[rfc][icedtea-web] TinyHttpdImpl refactor

[Adam Domurad]

On 08/01/2013 03:46 PM, Andrew Azores wrote:
> On 08/01/2013 03:25 PM, Adam Domurad wrote:
>> On 08/01/2013 11:15 AM, Andrew Azores wrote:
>>> On 07/30/2013 01:25 AM, Jiri Vanek wrote:
>>>> On 07/26/2013 05:34 PM, Andrew Azores wrote:
>>>>> Changelog:
>>>>> * tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
>>>>> (TinyHttpdImpl): BufferedReader and DataOutputStream become member
>>>>> variables rather than local to run. More informative/mnemonic
>>>>> variable names
>>>>> (writeErrorToPipe): new private method.
>>>>> (writeToPipe): new private method.
>>>>> (run): refactored to use writeToPipe and writeErrorToPipe, and
>>>>> ensuring that streams are closed when no longer in use. More
>>>>> informative/mnemonic variable names
>>>>>
>>>>> The patch looks big (relative to the size of the file) but it's
>>>>> really almost entirely variable renaming and method extractions. :)
>>>> Does not, I'm dealyed due to packaging stuff ;( Sorry
>>>> Practicaly no objections to the code:
>>>> * The unittests for this are however a must - for new method,
>>>> constructors and especially for run - to ensure they behave as they
>>>> behaved before. Not that the backward compatibility is important in
>>>> tests extensions, but because it worked pretty well.
>>>> I would rather this patch to be pushed to head only together with
>>>> unittests, but if you swear, you can push in two pushes.
>>>>
>>>>   Thanx for refactoring!
>>>>     J.
>>>
>>> Round two.
>>>
>>> Changelog:
>>> *
>>> tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java:
>>> moved TinyHttpdImpl tests into new class
>>> *
>>> tests/test-extensions-tests/net/sourceforge/jnlp/TinyHttpdImplTest.java:
>>> new class for testing TinyHttpdImpl specifically
>>> * tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java:
>>> refactored. "port" field removed as it was unused. Server now continues
>>> to run after an HTTP 404.
>>> * tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java:
>>> removed "port" argument from TinyHttpdImpl constructor call
>>>
>>> I decided to undo some of my deeper changes to TinyHttpdImpl since they
>>> were really done in the first place just to make the code look cleaner
>>> (not good reasoning...), but I found better ways to clean it up without
>>> making the reader/writer variables into fields.
>>>
>>> Andrew A
>>
>> I believe you forgot a patch
>>
>> Cheers,
>> -Adam
>
> D'oh, this happens to me too frequently. Thanks.
>
> Andrew A

Comments inline, patch first:

> diff --git a/tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java b/tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java
> --- a/tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java
> +++ b/tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java
> @@ -113,7 +113,7 @@ public class ServerLauncher implements R
>          try {
>              serverSocket = new ServerSocket(port);
>              while (running) {
> -                TinyHttpdImpl server = new TinyHttpdImpl(serverSocket.accept(), dir, port,false);
> +                TinyHttpdImpl server = new TinyHttpdImpl(serverSocket.accept(), dir, false);
>                  server.setSupportingHeadRequest(isSupportingHeadRequest());
>                  server.start();
>              }
> diff --git a/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java b/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> --- a/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> +++ b/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> @@ -59,120 +59,133 @@ import java.util.StringTokenizer;
>   */
>  public class TinyHttpdImpl extends Thread {
>
> -    Socket c;
> -    private final File dir;
> -    private final int port;
> -    private boolean canRun = true;
> +    private static final String CRLF = "\r\n";
> +    private static final String HTTP_BAD_REQUEST = "HTTP/1.0 " + HttpURLConnection.HTTP_BAD_REQUEST + " Bad Request" + CRLF;
> +    private static final String HTTP_NOT_IMPLEMENTED = "HTTP/1.0 " + HttpURLConnection.HTTP_NOT_IMPLEMENTED + " Not Implemented" + CRLF;
> +    private static final String HTTP_NOT_FOUND = "HTTP/1.0 " + HttpURLConnection.HTTP_NOT_FOUND + " Not Found" + CRLF;
> +    private static final String HTTP_OK = "HTTP/1.0 " + HttpURLConnection.HTTP_OK + " OK" + CRLF;
>      private static final String XSX = "/XslowX";
> -    private boolean supportingHeadRequest = true;
> -
> -    public TinyHttpdImpl(Socket s, File f, int port) {
> -        this(s, f, port, true);
> +
> +    private Socket mSocket;

As discussed on IRC, this prefixing of 'm' before an instance variable 
is not used in icedtea-web. Please consider using 'this.variable' 
wherever you feel it is unclear. Personally for me IDE colouring renders 
scope differences obsolete.

[.. snip ..]

No qualms otherwise with the changes.


Tests look good.

I think you can push if you drop the mSomething naming convention.

Cheers,
-Adam