Comments inline below:
On 5/31/2013 11:53 AM, Mandy Chung wrote:
Jeff,
The limited doPrivilege is very useful and I'm hoping the libraries
can begin using the limited doPrivileged once this is in jdk8. It's
non-trivial stuff and I know that you're working on the
unit tests and I can look at them when they are available.
It looks fine to me and I don't see anything obviously wrong.
This extends the wrapper implementation for the combiner
and the new comments you added make it easier to understand.
I think it'd be good to extend the javadoc of package-private
AccessControlContext constructor about "wrapped" ACC
and some summary on how they will be used in determining limited
privilege scope.
I don't spot anything obviously wrong. Just a few minor comments:
AccessController.java
You fixed up a couple of @see #doPrivileged in existing
methods. Looks like L393-394, 445-446 were copied before
the change. I'm not sure how these @see tags were
decided to be included and it seems worth taking a pass
and clean that up if appropriate.
Yes, I need to make a pass over the @see listings in the javadoc.
There was one for the PrivilegedExceptionAction version of the basic
doPrivileged() that was inconsistent with the pattern set by the javadoc
on the other methods so I changed it (it looked like a copy & paste
oversight from the original code).
You can call java.util.Objects.requireNonNull(perms) to
replacethe null argument check (e.g. L403-405). Might
be good to move the null argument check at the beginning?
Good point. I forgot about the new java.util.Object added in Java 7.
AccessControlContext.java
L214-231: I wonder if it might be better to move this
validation to AccessController.createWrapperbefore constructing
the ACC wrapper.
I wanted to keep this code in AccessControlContext.java since it is
part of the implementation that is spread over the rest of the class.
Moving it to AccessController.java would be awkward and would
leak the implementation outside of AccessControlContext.java.
The comments in the combine method e.g. L618-196, 622-623
need update after the refactoring.
Good point.
L740-744: FYI this can be replaced by a simple call to:
Objects.equals(this.combiner, that.combiner)
Same comment can apply to the equality check for this.permission
and this.parent.
True, although the implementation of Object.equals() looks somewhat
non-optimized. I'm curious why there is a long chain of
if/else tests before falling back to obj.equals() rather than a simple
"if obj.getClass().isArray() else". I'll look into that a bit more.
checkPermission2 is checking the limited privilege scope.
Maybe good to rename it to something like checkLimitedPrivilegeScope
to make it explicit.
Yes, perhaps.
Jeff
On 5/28/2013 10:56 PM, Jeff Nisewanger wrote:
This is the implementation of the JEP 140 feature for a limited
privilege form of doPrivileged(). A test will be added in an
updated webrev within the next day.
The JEP is: http://openjdk.java.net/jeps/140
The webrev is: http://cr.openjdk.java.net/~jdn/8014097/webrev.0/
