Thanks for the review. On the javadoc comments, there are a couple
of small spec changes that will probably happen after feature freeze anyway.
So, that might be the best time to address the other javadoc issues.
I agree with your other comments. On the synchronized method in
MessageHeader,
I don't believe it needs to be synchronized since the method is not
relying on
consistency between object fields, and the returned object can be
modified before, during or after the method is called anyway.
Michael
On 12/05/13 08:13, Alan Bateman wrote:
On 10/05/2013 12:34, Michael McMahon wrote:
Hi,
This is the webrev for the HttpURLPermission addition.
As well as the new permission class, the change
includes the use of the permission in java.net.HttpURLConnection.
The code basically checks for a HttpURLPermission in plainConnect(),
getInputStream() and getOutputStream() for the request and if
the caller has permission the request is executed in a doPrivileged()
block. When the limited doPrivileged feature is integrated, I will
change the doPrivileged() call to limit the privilege elevation to a
single
SocketPermission (as shown in the code comments).
The webrev is at http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
A partial review, focusing mostly on the spec as we've been through a
few rounds on that part already. Overall I think the javadoc looks
quite good. I realize someone suggested using lowercase "url" in the
javadoc but as the usage is as an acronym then it might be clearer if
it were in uppercase, maybe "URL string" to avoid any confusion with
java.net.URL.
I assume you'll add a copyright header to HttpURLPermission before
pushing this.
A minor comment on the javadoc tags is that you probably should use
@throws instead of @exception.
At a high-level it would be nice if the fields were final but I guess
the parsing of actions and being serialized complicates this.
setURI - this parses the URI rather than "sets" it so maybe it should
be renamed. If you use URI.create then it would avoid needing to catch
the URISyntaxException.
normalizeMethods/normalizeHeaders- I assume these could use an ArrayList.
HttpURLConnection - "if a security manager is installed", should this
be "set"?
MessageHeader - some of the methods are synchronized, some are not. I
can't quite tell if getHeaderNames needs to be synchronized. Also is
there any reason why this can't use Arrays.asList?
HttpURLConnection.setRequestMethod - "connection being open" ->
"connect in progress"?
That's all I have for now but I think there is further review work
required on HttpURLConnection as some of that is tricky.
-Alan.
