[Remoting Android] Placeholder for DesktopView

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

Index: remoting/android/java/src/org/chromium/chromoting/Desktop.java
diff --git a/remoting/android/java/src/org/chromium/chromoting/Desktop.java b/remoting/android/java/src/org/chromium/chromoting/Desktop.java
index 4ad4324d816408f6fb351b04a96647f7a5e4ec69..62a84f7bef3ce51c1de1bdf3f4a4d3982fefebf2 100644
--- a/remoting/android/java/src/org/chromium/chromoting/Desktop.java
+++ b/remoting/android/java/src/org/chromium/chromoting/Desktop.java
@@ -24,6 +24,7 @@ import android.view.MotionEvent;
 import android.view.View;
 import android.view.View.OnLayoutChangeListener;
 import android.view.View.OnTouchListener;
+import android.view.ViewGroup;
 import android.view.inputmethod.InputMethodManager;
 
 import org.chromium.chromoting.cardboard.DesktopActivity;
@@ -104,7 +105,11 @@ public class Desktop
         mToolbar = (Toolbar) findViewById(R.id.toolbar);
         setSupportActionBar(mToolbar);
 
-        DesktopView remoteHostDesktop = (DesktopView) findViewById(R.id.desktop_view);
+        DesktopViewInterface remoteHostDesktop =
+                mClient.getDisplay().createDesktopView(getApplicationContext());

[Lambros, 2016/07/08 02:13:41]

When creating Views, it's important to use the activity context, not the app
context (I expect you'll see some crashes). Pass 'this' here instead.

When the Activity goes away, after onDestroy(), the View will no longer be
valid. If JNI or anything else can call into the View and modify any UI, you'll
have to add something to onDestroy() to stop that from happening.

[Yuwei, 2016/07/08 19:58:56]

Done
I think that's fine. Both desktop views override SurfaceDestroyed() and do
cleanup there.

+        remoteHostDesktop.setLayoutParams(new ViewGroup.LayoutParams(
+                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT));
+        ((ViewGroup) findViewById(R.id.desktop_view_placeholder)).addView((View) remoteHostDesktop);

[Lambros, 2016/07/08 02:13:41]

The cast to (View) is ugly, maybe indicates a design weakness?

It seems like you want to force any DesktopViewInterface implementations to be
View-derived objects? Maybe an abstract View-derived class would be a better
choice than a pure interface?

Alternatively, have the View object and DesktopViewInterface impl as separate
objects, where one is a member of the other?

I don't feel too strongly if you have reasons for wanting this design.

[Yuwei, 2016/07/08 19:58:56]

Actually I have considered making it an abstract class extending View but the
problem is both desktop views extend from SurfaceView and Java doesn't allow
multi-class inheritance. It will work if I extend the desktop view "interface"
from SurfaceView but I'm not sure whether this is a flexible design...

Maybe just make it extend from SurfaceView for now? It's unlikely we will have
the third rendering implementation in foreseeable future...

[Lambros, 2016/07/08 22:02:36]

I think this is fine. As long as you are rendering in another thread, you are
going to need a SurfaceView anyway.

If you like, you could make your own SurfaceView-derived class, and have any
differences in implementation delegated to a separate object which can be
created/switched at run-time. If you had

class MySurfaceView extends SurfaceView {
public MySurfaceView(Context context, AttributeSet attributes) {...}
}

You could add <org.chromium.chromoting.MySurfaceView>.. directly into your
layout XML without needing any placeholder View.

[Yuwei, 2016/07/08 23:07:54]

This sounds like a better approach, but also requires a lot of refactorizations
O_o

This may sound a little bit hacky. I'm not sure whether a surface that had been
used to draw Java canvas can be reused for rendering OpenGL buffers when
switching impl. at run-time...

I don't have strong opinion about this. Maybe check in this CL and consider this
approach later?

[Yuwei, 2016/07/08 23:35:53]

CC zijiehe@. There is a discussion about making DesktopView a pure drawing
canvas and pulling out the drawing logic. Do you have any opinion about this
change or is it something already in your refactoring plan?

         remoteHostDesktop.init(this, mClient);
         mSwitchToCardboardDesktopActivity = false;
 
@@ -163,8 +168,6 @@ public class Desktop
         super.onStart();
         mActivityLifecycleListener.onActivityStarted(this);
         mClient.enableVideoChannel(true);
-        DesktopView desktopView = (DesktopView) findViewById(R.id.desktop_view);

[Yuwei, 2016/07/08 00:30:00]

This logic is unnecessary since attachRedrawCallback() is already called inside
DesktopView.surfaceCreated(). Removed this since there is no more
R.id.desktop_view.

-        desktopView.attachRedrawCallback();
         mClient.getCapabilityManager().addListener(this);
     }