On 04/10/2018 22:04, Sean Mullan wrote:
Excellent comments, Stuart. Thanks for taking the time to review this.
I have posted another review that should address most of your
comments, but also responded inline with replies below.
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/
I also posted the javadoc so you can see what it looks like, esp. the
table:
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/SecurityManager.html
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/System.html
The update to the javadoc, to describe how the security manager is
located etc., goes beyond the original scope of the change but it's a
welcome update as this is something that was always missing from the docs.
Overall I think it looks good. The only issue I see is the statement
"The class is loaded by the system class loader ..." as it's actually
the built-in application class loader. This is necessary to avoid some
tricky bootstrapping issues, also to ensure the security manager is set
before the custom system class loader executes.
I agree with Stuart's comment that the quotes around
java.security.manager are a bit of a distraction.
A minor nit with the webrev is that the lines with the table
descriptions are way too long. You can put line breaks in those and it
will not impact the generated javadoc.
-Alan.
