/hg/icedtea-web: Properly disconnect all connected http connecti...
Omair Majid
omajid at redhat.com
Tue May 20 15:40:01 UTC 2014
* Jiri Vanek <jvanek at redhat.com> [2014-05-15 11:17]:
> I have one more opinion to the final I have added. If somebody will
> suddenly try to assign to final value, he will be warned by compiler. And
> thats good thing. At least it will force him to double check what he is
> doing.
Fair enough. I prefer to make fields final only if all of them can be.
It's my hint for immutable (and hence thread-safe) object.
> I have also followed your recommendation to move few util classes to
> UrlUtils. And write tests for them.
Awesome!
> There is one change in code then. The original notNullUrlEquals ignored
> port! I added port check to new code. (this is actually code logic and to
> not belongs here..)
>
> I will NOT push this line (unless it is approved directly).
Nice!
> +++ b/netx/net/sourceforge/jnlp/cache/CacheEntry.java Thu May 15 17:09:28 2014 +0200
> @@ -75,6 +74,7 @@
>
> /**
> * Returns the remote location this entry caches.
> + * @return URL same, as the one on which this entry was created
No comma needed here. But I must ask: why are you duplicating the
same thing in two places right next to each other here?
> /**
> * Returns the time in the local system clock that the file was
> * most recently checked for an update.
> - * @return
> + * @return when the item was updated in ms
> */
> public long getLastUpdated() {
> * Sets the time in the local system clock that the file was
> * most recently checked for an update.
> - * @param updatedTime
> + * @param updatedTime the time to be set as last updated time
> */
> public void setLastUpdated(long updatedTime) {
Can you add the 'ms' reference to both methods?
> - * @param lastModified
> + * @param lastModified - current time detail as get from server
I cant really understand the explanation in the comment. What is 'time
detail'? Would it better to say 'the value of "Last-Modified" http
header'?
>
> - try {
> - long remoteModified = lastModified;
> - long cachedModified = Long.parseLong(properties.getProperty(KEY_LAST_MODIFIED));
> + long remoteModified = lastModified;
> + long cachedModified = Long.parseLong(properties.getProperty(KEY_LAST_MODIFIED));
>
> - if (remoteModified > 0 && remoteModified <= cachedModified)
> - return true;
> - else
> - return false;
> - } catch (Exception ex) {
> - OutputController.getLogger().log(ex);;
> + return remoteModified > 0 && remoteModified <= cachedModified;
There is one semantic change here: if properites.getProperty() returns
null, Long.parseLong throws NumberFormatException. It would have been
caught before, but isn't now. Is it safe to assume that this never
happens?
> +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java Thu May 15 17:09:28 2014 +0200
> - /**
> * Returns the Permission object necessary to access the
> * resource, or {@code null} if no permission is needed.
> - * @param location
> - * @param version
> - * @return
> + * @param location location of the resource
> + * @param version the version, or {@code null}
You know, if you named the variables resourceLocation and
resourceVersion, you wouldn't need these comments :(
> + * @return permissions of the location
This really doesn't explain anything. It just restates (in less detail!)
the name/return type of the method. Please just remove it.
> * Returns the parent directory of the cached resource.
> + * @return parent directory of cached resource
This is just duplicating text :(
> + * @return the stream towrite to resource
There's a typo here, but it is also really awkward to read. Can you
rephrase it to like "the stream to use when writing the resource to
disk"?
> + * @throws java.io.IOException if it appear
Uh.. what does this mean?
> */
> public static OutputStream getOutputStream(URL source, Version version) throws IOException {
> File localFile = getCacheFile(source, version);
> @@ -456,6 +405,9 @@
> * Copies from an input stream to an output stream. On
> * completion, both streams will be closed. Streams are
> * buffered automatically.
> + * @param is stream to read from
> + * @param os stream to write to
Maybe rename variables to 'source' and 'dest' and there won't be a need
for comments?
> + * @throws java.io.IOException if copy fails
I would just leave it out. It really doesn't say anything more than the
method signature:
> public static void streamCopy(InputStream is, OutputStream os) throws IOException {
> - List<URL> urlList = new ArrayList<URL>();
> + List<URL> urlList = new ArrayList<>();
Please don't mix diamond changes in this patch. Okay as a separate
patch, though.
> +++ b/netx/net/sourceforge/jnlp/cache/Resource.java Thu May 15 17:09:28 2014 +0200
> /**
> * Return a shared Resource object representing the given
> * location and version.
> + * @param location final location of resource
> + * @param requestVersion final version of resource
> + * @param updatePolicy final policy for updating
What does 'final' here mean?
> @@ -290,6 +297,7 @@
> /**
> * Removes the tracker to the list of trackers monitoring this
> * resource.
> + * @param tracker
Please don't do this. At this point, it's just noise.
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Thu May 15 17:09:28 2014 +0200
> @@ -308,6 +309,7 @@
> * on each tracker that is monitoring the resource. Do not call
> * this method with any locks because the listeners may call
> * back to this ResourceTracker.
> + * @param resource resource which event is fired
Maybe rephrase it to 'resource on which event is fired'.
> /**
> * Open a URL connection and get the content length and other
> * fields.
> + *
> + * @param resource the resource to download
s/download/initialize/
> @@ -863,21 +873,28 @@
> /**
> * testing wrapper
> *
> - * @param url
> - * @param requestProperties
> - * @param requestMethod
> - * @return
> + * @param url url of to be checked
> + * @param requestProperties properties map for connection
> + * @param requestMethod method of request - HEAD or GET
This method should really be encapsulated in an enum.
> + * @return the repsonse code from server
> * @throws IOException
> */
For a method documented as 'testing wrapper', I don't think it makes any
sense to document the parameters.
> +++ b/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Thu May 15 17:09:28 2014 +0200
> /**
> * Add an Application listener
> + * @param listener listener to be added
> */
> public void addApplicationListener(ApplicationListener listener) {
Why not rename the argument `listener` to `toAdd`? They you wont need
the @param.
> /**
> * Remove an Application Listener
> + * @param listener to be removed
> */
> public void removeApplicationListener(ApplicationListener listener) {
Same thing here.
> @@ -268,6 +277,7 @@
>
> /**
> * Returns whether the application is running.
> + * @return state of application
No, please describe the return value. As in '@return true if the
application is still running'.
But, this comment is just repeating the method signature. There's no
need for it.
> @@ -349,11 +362,16 @@
>
> /**
> * Returns whether or not this jar is signed.
> + * @return whether jar is signed.
This is duplicating (incorrect) text. It really should be the
application, not the jar, that's signed.
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/UrlUtilsTest.java Thu May 15 17:09:28 2014 +0200
>
> + @Test
> + public void testUrlEquals() throws Exception {
> + final URL n1 = null, n2 = null, u1 = new URL("http://example.com"), u2 = u1, u3 = new URL("http://example.com");
> + Assert.assertTrue("Two nulls should be equal", UrlUtils.urlEquals(n1, n2));
> + Assert.assertFalse("Null URL should not equal a non-null", UrlUtils.urlEquals(n1, u1));
> + Assert.assertTrue("URL should equal itself (same reference)", UrlUtils.urlEquals(u1, u2));
> + Assert.assertTrue("URLs should be equal when different reference but the same URL", UrlUtils.urlEquals(u1, u3));
You know, this really should be split into multiple tests:
testUrlEqualsSameUrls
testUrlEqualsNullUrls
testUrlEqualsDifferentUrls
And so on. If the tests are independent, moving them to different tests
makes a lot of sense.
> + public void notNullUrlEquals_nulls1() throws Exception {
Please rename it to something like:
verifyNotNullUrlEqualsThrowsExceptionWhenBothArgumentsAreNull()
> + public void notNullUrlEquals_nulls2() throws Exception {
How about:
verifyNotNullUrlEqualsThrowsExceptionWhenFirstArgumentIsNull()
> +
> + public void notNullUrlEquals_nulls3() throws Exception {
Missing @Test
Please rename this method to:
verifyNotNullUrlEqualsThrowsExceptionWhenSecondArgumentIsNull()
Please apply similar changes to the rest of the code in this file.
> +++ b/tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java Thu May 15 17:09:28 2014 +0200
> @@ -134,6 +123,8 @@
> * param "port" prints out the port
> * nothing or number will run server on random(or on number specified)
> * port in -Dtest.server.dir
> + * @param args paramas from commandline. recognized are port and randomport
s/paramas/params/
I am not sure I understand "recognized are port and randomport". Do you
mean the first argument is port and the second is randomport?
> + * @throws Exception
> + * @throws Exception
> + * @throws Exception
Please, just leave these out.
Thanks,
Omair
--
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681
More information about the distro-pkg-dev
mailing list