Refactor SessionHandler and BrowserChannelClient to allow other OOPHM clients than HtmlUnit

There's a TODO(amitmanjhi) in HtmlUnitSessionHandler to refactor SessionHandler,
and the current situation makes it impossible for me to implement OOPHM in Adobe
AIR (well, unless I write a full-fledged OOPHM client).

So here's a patch that factors out a SessionHandlerClient from
HtmlUnitSessionHandler (breaking all references between BrowserChannelClient and
HtmlUnit), and pushing down a bunch of methods from SessionHandler into a new
SessionHandlerServer (all those methods that were implemented in
HtmlUnitSessionHandler as throwing an UnsupportedOperationException).

This patch does not do any tidying of no-op or "silly" methods (sendFreeValues
is a no-op, as javaObjectsToFree is always empty; and invokeSpecial should never
be called on the client side), it's just straightforward refactoring, thanks to
Eclipse (extract superclass, pull up, push down).

I've renamed HtmlUnitSessionHandler's getHtmlPage to getSynchronizationObject,
'cause it really doesn't matter which type of object it is, getHtmlPage() was
only used as the synchronization object for synchronized blocks.

I checked that it compiles OK and both BrowserChannelTest and
BrowserChannelServerTest are green.

[jat]

I really think this is too big a change to make between the RC and the final
release.  I would rather wait until after we ship 2.0 and spend a bit more time
on it to clean up some of the other rough edges before getting it into trunk.

[jat]

Ok, I think we can work on getting this into trunk now.

To do that, we need to do the cleanups you mention, such as getting rid of
methods that only throw UOE from SessionHandler.

[tbroyer]

On 2009/12/10 14:49:27, jat wrote:
> Ok, I think we can work on getting this into trunk now.
> 
> To do that, we need to do the cleanups you mention, such as getting rid of
> methods that only throw UOE from SessionHandler.

Any chance of having it into 2.1?

[jat]

On 2010/02/09 10:04:05, tbroyer wrote:
> Any chance of having it into 2.1?

Yes -- we still need the cleanups mentioned above, but to get this in it
probably needs to be done by the end of the quarter.

Amit also needs to be involved, as he is working on HtmlUnit changes right now
so there might need to be some coordination there.

[amitmanjhi]

I will take a look at this in a few weeks, after I am done with my immediate
HtmlUnit related changes. It would certainly be useful to refactor this code.

On 2010/02/09 16:11:02, jat wrote:
> On 2010/02/09 10:04:05, tbroyer wrote:
> > Any chance of having it into 2.1?
> 
> Yes -- we still need the cleanups mentioned above, but to get this in it
> probably needs to be done by the end of the quarter.
> 
> Amit also needs to be involved, as he is working on HtmlUnit changes right now
> so there might need to be some coordination there.

[amitmanjhi]

I  quickly looked at the changes and they look okay. We can get this patch into
trunk sooner than a few weeks.

@tbroyer: Can you update the patch to address John's concerns? Basically, just
push methods, as appropriate, from SessionHandler to SessionHandlerClient and
SessionHandlerServer that gets rid of any UnsupportedOperationExceptions.

[tbroyer]

On 2010/02/10 22:40:22, amitmanjhi wrote:
> 
> @tbroyer: Can you update the patch to address John's concerns? Basically, just
> push methods, as appropriate, from SessionHandler to SessionHandlerClient and
> SessionHandlerServer that gets rid of any UnsupportedOperationExceptions.

I got rid of all the UnsupportedOperationExceptions, which means that an
INVOKE_SPECIAL message type received on client will be treated as an error and
disconnect the plug-in.

I went a bit further and made SessionHandler generic to remove the need for
casts to SessionHandlerClient (resp. SessionHandlerServer) from within
BrowserChannelClient (resp. BrowserChannelServer) and derived classes.

[jat]

LGTM with some formatting issues.

I assume you have run tests, both using HtmlUnit and real browsers in DevMode?

http://gwt-code-reviews.appspot.com/114801/diff/5003/4005
File dev/core/src/com/google/gwt/dev/shell/BrowserChannelClient.java (right):

http://gwt-code-reviews.appspot.com/114801/diff/5003/4005#newcode34
Line 34: public abstract static class SessionHandlerClient extends
SessionHandler<BrowserChannelClient> {
Longer than 80-columns, as well as elsewhere.

[amitmanjhi]

LGTM.  Thanks for the patch.

I tried to apply the patch to a not-too-old trunk (r7385) and the patch
to BrowserListener.java did not apply cleanly. So you may want to double-check
that to see if the file has changed recently or if you have a local change in
your file.

Plus, I have the same question as John regarding tests and formatting.

[tbroyer]

On 2010/02/18 00:56:27, amitmanjhi wrote:
> LGTM.  Thanks for the patch.
> 
> I tried to apply the patch to a not-too-old trunk (r7385) and the patch
> to BrowserListener.java did not apply cleanly. So you may want to double-check
> that to see if the file has changed recently or if you have a local change in
> your file.

The second patch set was made against r7585, where BrowserListener was changed
to accept a bindAddress.
It applies OK against the current trunk too (r7592)

> Plus, I have the same question as John regarding tests and formatting.

I just ran UISuite in both prod and dev mode, and applied the same patch to a
GWT 2.0.2 project (well, actually copying the patched files to my project and
ensuring they were earlier on the classpath than gwt-dev.jar; I can confirm it
uses those classes as I had an error because of the change from SessionHandler
to SessionHandlerServer in the BrowserListener ctor; DevModeBase was looking for
a ctor with a SessionHandler argument, I copied DevModeBase.java to my project
so it was recompiled againts the "new" BrowserListener and DevMode launched
without error) which ran OK in dev mode in Chrome.
Most tests from the UISuite are green which, to me, means that the patch is OK
(others are errors caused by some local modifications to gwt-user, two
"JavaScriptException: (null): null" in RadioButtonTest's testGrouping and
testGroupingDeprecated, and then a timeout followed by a bunch of
UnableToCompleteException, but I'm experiencing some troubles with my network
card and the tests run on my "public" IP, not 127.0.0.1)

As for the formatting, I'm confused, as Checkstyle ("ant dev/build.xml
checkstyle") doesn't report any error.

[amitmanjhi]

The patch does apply cleanly against the current trunk. I just applied the patch
and ran all the tests. All tests passed. 

The checkstyle test actually doesn't check for all formatting related things. We
currently rely on a combination of formatting rules in eclipse and checkstyle.
After I did the formatting, there were minor changes to
BrowserChannelClient.java and BrowserListener.java, which I have posted at the
end (posting the patch inline probably screws up formatting, plus the whitespace
differences are not visible). 

If you are okay with the minor formatting changes, I can commit the patch on
your behalf. 

@@ -31,16 +31,18 @@ public class BrowserChannelClient extends BrowserChannel {
   /**
    * Hook interface for responding to messages from the server.
    */
-  public abstract static class SessionHandlerClient extends
SessionHandler<BrowserChannelClient> {
+  public abstract static class SessionHandlerClient extends
+      SessionHandler<BrowserChannelClient> {
 
     public abstract Object getSynchronizationObject();
-    
+
     public abstract String getUserAgent();
 
-    public abstract ExceptionOrReturnValue invoke(BrowserChannelClient channel,
Value thisObj, String methodName,
-        Value[] args);
+    public abstract ExceptionOrReturnValue invoke(BrowserChannelClient channel,
+        Value thisObj, String methodName, Value[] args);
 
-    public abstract void loadJsni(BrowserChannelClient channel, String
jsniString);
+    public abstract void loadJsni(BrowserChannelClient channel,
+        String jsniString);
   }
 
   private static class ClientObjectRefFactory implements ObjectRefFactory {
@@ -111,7 +113,7 @@ public class BrowserChannelClient extends BrowserChannel {
 
   /**
    * @return the negotiated protocol version -- only valid after {@link #init}
-   * has returned.
+   *         has returned.
    */
   public int getProtocolVersion() {
     return protocolVersion;
@@ -145,8 +147,7 @@ public class BrowserChannelClient extends BrowserChannel {
   }
 
   public ReturnMessage reactToMessagesWhileWaitingForReturn(
-      SessionHandlerClient handler) throws IOException,
-      BrowserChannelException {
+      SessionHandlerClient handler) throws IOException, BrowserChannelException
{
     ReturnMessage returnMessage = reactToMessages(handler, true);
     return returnMessage;
   }
@@ -181,9 +182,8 @@ public class BrowserChannelClient extends BrowserChannel {
     return true;
   }
 
-  private ReturnMessage reactToMessages(
-      SessionHandlerClient handler, boolean expectReturn)
-      throws IOException, BrowserChannelException {
+  private ReturnMessage reactToMessages(SessionHandlerClient handler,
+      boolean expectReturn) throws IOException, BrowserChannelException {
     while (true) {
       ExceptionOrReturnValue returnValue;
       MessageType type = Message.readMessageType(getStreamFromOtherSide());
@@ -193,11 +193,10 @@ public class BrowserChannelClient extends BrowserChannel {
         switch (type) {
           case INVOKE:
             InvokeOnClientMessage invokeMessage =
InvokeOnClientMessage.receive(this);
-            returnValue = handler.invoke(this,
-                invokeMessage.getThis(), invokeMessage.getMethodName(),
-                invokeMessage.getArgs());
+            returnValue = handler.invoke(this, invokeMessage.getThis(),
+                invokeMessage.getMethodName(), invokeMessage.getArgs());
             new ReturnMessage(this, returnValue.isException(),
-                returnValue.getReturnValue()).send();           
+                returnValue.getReturnValue()).send();
             break;
           case FREE_VALUE:
             FreeMessage freeMessage = FreeMessage.receive(this);
@@ -214,7 +213,7 @@ public class BrowserChannelClient extends BrowserChannel {
             break;
           case REQUEST_ICON:
             RequestIconMessage.receive(this);
-            // no need for icon here 
+            // no need for icon here
             UserAgentIconMessage.send(this, null);
             break;
           case RETURN:
@@ -32,7 +32,6 @@ import java.net.SocketException;
  */
 public class BrowserListener {
 
-
   /**
    * Get a query parameter to be added to the URL that specifies the address
    * of this listener.

[jat]

On 2010/02/19 00:40:57, amitmanjhi wrote:
> The patch does apply cleanly against the current trunk. I just applied the
patch
> and ran all the tests. All tests passed. 

Did you also run with a real browser in DevMode?

[amitmanjhi]

Just tested it with FF 3.6 and a test app created using webAppCreator.

On 2010/02/19 00:48:04, jat wrote:
> On 2010/02/19 00:40:57, amitmanjhi wrote:
> > The patch does apply cleanly against the current trunk. I just applied the
> patch
> > and ran all the tests. All tests passed. 
> 
> Did you also run with a real browser in DevMode?

[tbroyer]

On 2010/02/19 00:40:57, amitmanjhi wrote:
> 
> If you are okay with the minor formatting changes, I can commit the patch on
> your behalf. 

LGTM