/hg/icedtea-web: Properly disconnect all connected http connecti...
Jiri Vanek
jvanek at redhat.com
Wed May 21 09:48:38 UTC 2014
On 05/20/2014 05:40 PM, Omair Majid wrote:
> * 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?
will be done
>
>> - * @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'?
+ * @param lastModified - current time as get from server. Mostly 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 :(
see lower
>
>> + * @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.
>
I would say it does. It i snot clear which permissions those are.
>> * Returns the parent directory of the cached resource.
>
>> + * @return parent directory of cached resource
>
> This is just duplicating text :(
see lower
>
>> + * @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?
>
Well one of the most dummy comments I come with :)
>> */
>> 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?
nope. Those are is for input stream and os for output stream Well and excelent comment expalining
whatshappenng with them :) See lower again.
>
>> + * @throws java.io.IOException if copy fails
>
> I would just leave it out. It really doesn't say anything more than the
> method signature:
see lower
>
>> 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.
Ugh.. ok.. I iwll try to push this as separate comment, but
- I do not wont to bother with it.
- replace all with <> is too big and unnecessary patch
- so I replaced those minors I stumbled across... Is it so bad?
>
>> +++ 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?
Tha` it is set and never changed later.
>
>> @@ -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.
see lower
>
>> +++ 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'.
ok!
>
>> /**
>> * 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.
>
Worthy idea. Can I include it inside this patch?
>> + * @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.
>
+- deppends.. se lower
>> +++ 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.
May be. But stil there wil be an comment...
>
>> @@ -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.
ok. Thats why even this noise is good.
>
>> +++ 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:
Nope.
On one side, test doing one thing is greate, but to have 100000 of test which are doing neraly the
same is not so greate. I tried to separate those tests by behavior. And it seems to be right.
>
> testUrlEqualsSameUrls
> testUrlEqualsNullUrls
> testUrlEqualsDifferentUrls
>
> And so on. If the tests are independent, moving them to different tests
> makes a lot of sense.
as above. Does not.
>
>> + public void notNullUrlEquals_nulls1() throws Exception {
>
> Please rename it to something like:
>
> verifyNotNullUrlEqualsThrowsExceptionWhenBothArgumentsAreNull()
ok ;)
>
>> + public void notNullUrlEquals_nulls2() throws Exception {
>
> How about:
>
> verifyNotNullUrlEqualsThrowsExceptionWhenFirstArgumentIsNull()
ok ;)
/you should start to learn Finish :)
>
>
>> +
>> + public void notNullUrlEquals_nulls3() throws Exception {
>
> Missing @Test
oh good catch. Will be fixed.
>
> Please rename this method to:
>
> verifyNotNullUrlEqualsThrowsExceptionWhenSecondArgumentIsNull()
ok
>
> Please apply similar changes to the rest of the code in this file.
ok :(
>
>> +++ 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.
hmmm
This is the "see lower" part
We are a bit deadlocking around usability of comments. Well from where you are looking on it? I got
an impression you are looking on them from *code*. I think most of your arguments become invalid,
when you look to them as on final html file. ANd Imean pure html file without the code. If tose
which you wont me to remove will be removed (or when you wont some infomratin to be removed in
behalf of somthing else...) then this information is really missing in final html.
Is this what you wont?
J.
More information about the distro-pkg-dev
mailing list