> On Oct 8, 2015, at 5:26 AM, Daniel Fuchs <[email protected]> wrote:
>
> webrev:
> http://cr.openjdk.java.net/~dfuchs/8046565/proto.01/webrev.01/
System.Logger
Several log methods throws NPE if the level is legible and the parameter is
null. E.g.
* @throws NullPointerException if {@code level} is {@code null}, or if the level
* is loggable and {@code msgSupplier} is {@code null}.
Should it throw NPE if the input parameter is null regardless of whether the
level is loggable?
RuntimePermission(“loggerFinder”) is required when
1. a LoggerFinder provider is instantiated
2. LoggerFinder::getLoggerFinder() is called
3. LoggerFinder::getLogger is called
This is very specific to finding system logger (more than just the provider
construction check). It does seem worth defining a specific Permission type
for it e.g. LoggerPermission. Since LoggerFinder::getLogger requires
permission check, does LoggerFinder::getLoggerFinder() need the explicit
permission check? We will need to consult with the security experts.
LazyLoggers::getLazyLogger(String name, Class<?> caller)
- does it need permission check? it’s currently commented out
public class LoggerWrapper<L extends Logger> extends AbstractLoggerWrapper<L>
implements Logger, PlatformLoggerBridge
- AbstractLoggerWrapper implements Logger, PlatformLoggerBridge,
ConfigurableLoggerBridgem. Is "implements Logger, PlatformLoggerBridge” needed
at all?
Are all *Logger and *LoggerWrapper types implementing Logger,
PlatformLoggerBridge, ConfigurableLoggerBridge? I might be missing it - I
don’t see any non-configurable logger type implements only Logger,
PlatformLoggerBridge.
SimpleConsoleLogger.Formatting
static final String FORMAT_PROP_KEY =
"java.util.logging.SimpleFormatter.format";
- is this used when java.logging is not present? It’s for the simple console
logger formatting. If so, the property name should be renamed to
idk.logger.xxx?
Can you explain the difference of sun.util.logging and sun.util.logger package?
I think the reason to have two different packages is to make sure that
sun.util.logger not used by other modules. What other issue to put the classes
in sun.util.logging - the existing package? I don’t have any issue to create a
new package but the name is hard to differentiate. Maybe rename
sun.util.logger to jdk.internal.logger if not in sun.util.logging?
Similarly, sun.util.logging.internal.JdkLoggingProvider and
sun.util.logger.JdkLoggerProvider - the package names are too close and they
are in two different module. Probably good to rename the classname - what
about
s/JdkLoggerProvider/SystemLoggerProvider/
s/JdkLoggingProvider/LoggingProvider/
I would have assumed that sun.util.logger.JdkLoggerProvider should be
LoggerFinder. Is there any reason why it can’t extend LoggerFinder? I think
that would be cleaner and getJdkLogger is not needed. Maybe
DefaultLoggerFinder can be simplified.
Have you considered moving out LoggerFinderLoader to a separate class? This
change adds many lines in System.java.
PlatformLogger - main reason to retain it is to minimize changes. I wonder if
changing it to an interface would help or not. I’m still studying the
sun.util.logger.* classes.
jdk/test/java/lang/System/Logger/custom/AccessSystemLogger.java
jdk/test/java/lang/System/Logger/default/AccessSystemLogger.java
jdk/test/java/lang/System/LoggerFinder/public/BaseLoggerFinderTest/AccessSystemLogger.java
- they seem to be duplicated code to setup boot directory. Worth putting them
in logging test library?
- what does the directory name "public" mean? maybe just take that directory
out?
jdk/test/java/lang/System/LoggerFinder/internal/backend/SystemClassLoader.java
- copyright header
That’s all for today.
Mandy
