[Remoting Android] Refactor GlDesktopView

remoting/android/java/src/org/chromium/chromoting/RenderController.java

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chromoting;
+
+import org.chromium.chromoting.jni.Client;
+
+/**
+ * This class manages resources related to the rendering process. Its lifetime should be the same
+ * as the JNI client.
+ */
+public class RenderController {

[Hzj_jie, 2016/09/08 21:37:33]

I cannot quite tell the functionality of this class. Does it just create
mInputHandler each time a new DesktopView created? If so, why cannot we just
make InputHandler outlive?

And it seems this class has no relationship with Render, it just forwards
objects to mInputHandler. I also have concern about its name.

[Yuwei, 2016/09/08 23:09:14]

Removed. The original intention was to have a chance to invalidate the old
TouchInputHandler when the new one is registered. If we use the
attach/detach+onCreate/onDestroy pattern then this class is unnecessary.
Yes
May see Joe's comments. TouchInputHandler is useful only when the view exists so
it makes sense to scope it to a view's lifetime. I personally don't have a
strong opinion though. There is no need to remove listeners if InputHandler
outlives.
Obsolete.

+    private final Client mClient;
+    private RenderStub mRenderStub;
+    private TouchInputHandler mInputHandler;
+
+    public RenderController(Client client) {
+        Preconditions.notNull(client);
+        mClient = client;
+    }
+
+    /**
+     * Sets the render stub. This must be called exactly once before
+     * {@link RenderController#reset(Desktop, DesktopView)} is called.
+     * @param stub The render stub. Must not be null.

[joedow, 2016/09/08 18:32:47]

Is "Must not be null." required in the comment?  I don't know that it is very
helpful.

[Yuwei, 2016/09/08 23:09:15]

Obsolete. Class removed.

+     */
+    public void setRenderStub(RenderStub stub) {
+        Preconditions.isNull(mRenderStub);
+        Preconditions.notNull(stub);
+        mRenderStub = stub;
+    }
+
+    /**
+     * Resets the RenderController with the current activity and desktop view. Must be called after
+     * {@link RenderController#setRenderStub(RenderStub)} is called. Both arguments must not be
+     * null.

[joedow, 2016/09/08 18:32:47]

Remove the comments about null params and document the params as above (javadoc
style)

[Yuwei, 2016/09/08 23:09:15]

Obsolete.

+     */
+    public void reset(Desktop desktop, DesktopView view) {
+        Preconditions.notNull(desktop);
+        Preconditions.notNull(view);
+        Preconditions.notNull(mRenderStub);
+        if (mInputHandler != null) {
+            mInputHandler.invalidate();

[Yuwei, 2016/09/07 20:45:40]

Maybe we can make TouchInputHandler's lifetime as long as the session, then we
don't need to "invalidate" the old input handler every time the activity is
reset.

[joedow, 2016/09/08 18:32:47]

I would think of lifetime/scoping with respect to whether it is needed.  The
native classes live through activity lifecycle events (such as screen
orientation changes) but the java view classes are destroyed.  I would think the
TouchInputHandler would also be destroyed since it is only useful when receiving
input from a view (which is destroyed).

Another way to think about this is to use something like attach/detach instead
of init/reset.  When the activity starts, the class(es) with activity scope will
attach to the RenderController and when they are paused/stopped they can detach.

[Yuwei, 2016/09/08 23:09:15]

Done. Added attach/detach interface to DesktopView and TouchInputHandler becomes
part of DesktopView again.

+        }
+        mRenderStub.setDesktopView(view);
+        mInputHandler = new TouchInputHandler(view, desktop);
+        mInputHandler.init(desktop, mRenderStub, new InputEventSender(mClient));

[Hzj_jie, 2016/09/08 21:37:33]

If you really want to keep current design, you may want to merge
InputHandler.ctor and init function. It does not need a secondary
initialization.

[Yuwei, 2016/09/08 23:09:15]

Not keeping :P
Done.

+    }
+}