[Remoting Android] Placeholder for DesktopView

[Remoting Android] Placeholder for DesktopView

Preparation for using OpenGL ES rendering component on Android. This CL
leaves a FrameLayout placeholder for the DesktopView and creates the
implementation specific DesktopView in runtime.

BUG=385924
Committed: https://crrev.com/0822e24c49deda76b49076fe67f002ea8c66fc04
Cr-Commit-Position: refs/heads/master@{#404555}

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

yuweih@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Yuwei, 2016-07-08 00:30:01]

PTAL

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (left):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:166: DesktopView
desktopView = (DesktopView) findViewById(R.id.desktop_view);
This logic is unnecessary since attachRedrawCallback() is already called inside
DesktopView.surfaceCreated(). Removed this since there is no more
R.id.desktop_view.

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (left):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:78: public
DesktopView(Context context, AttributeSet attributes) {
Attributes are now being set in runtime.

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

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:109:
mClient.getDisplay().createDesktopView(getApplicationContext());
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.

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:112: ((ViewGroup)
findViewById(R.id.desktop_view_placeholder)).addView((View) remoteHostDesktop);
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.

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/DesktopViewInterface.java
(right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/DesktopViewInterface.java:71:
void setLayoutParams(ViewGroup.LayoutParams params);
Please add JavaDoc for each interface method.

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:28: private
DesktopViewFactory mDisplay;
Can you fix the naming inconsistency somehow?
Either mDisplayFactory / DisplayFactory
Or mDesktopViewFactory / DesktopViewFactory.
Or maybe DisplayViewFactory?

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

ptal

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:109:
mClient.getDisplay().createDesktopView(getApplicationContext());
On 2016/07/08 02:13:41, Lambros wrote:
> 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.

Done

> 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.

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

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:112: ((ViewGroup)
findViewById(R.id.desktop_view_placeholder)).addView((View) remoteHostDesktop);
On 2016/07/08 02:13:41, Lambros wrote:
> 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.

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...

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:28: private
DesktopViewFactory mDisplay;
On 2016/07/08 02:13:41, Lambros wrote:
> Can you fix the naming inconsistency somehow?
> Either mDisplayFactory / DisplayFactory
> Or mDesktopViewFactory / DesktopViewFactory.
> Or maybe DisplayViewFactory?
> 

Logically speaking, this interface is just for creating a DesktopView, so it
should be called as DesktopViewFactory. But it also represents the
implementation specific display handler.

Just separated the DesktopViewFactory from the idea of display object and made
the display object a private detail of the desktop view.

This breaks the cardboard code since now Display is tightly bundled with the
DesktopView and the cardboard renderer can no longer get the reference to
Display from Client.

Seems like we don't care too much about the cardboard feature. I just pass in
null for the Display to fix the build and will file a bug for this as a reminder
of this issue.

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

lgtm

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:112: ((ViewGroup)
findViewById(R.id.desktop_view_placeholder)).addView((View) remoteHostDesktop);
On 2016/07/08 19:58:56, Yuwei wrote:
> On 2016/07/08 02:13:41, Lambros wrote:
> > 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.
> 
> 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...

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.

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java
(right):

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:31:
public abstract void showActionBar();
Do showActionBar() and showKeyboard() need to be part of this interface? I'm not
sure why the View should care about these?
Doesn't matter for this CL, but maybe these can be refactored out somehow?

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:72:
/** An {@link Event} which is triggered when user touchs the screen. */
s/touchs/touches/

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
File
remoting/android/java/src/org/chromium/chromoting/cardboard/DesktopActivity.java
(right):

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/cardboard/DesktopActivity.java:50:
// it into the constructor.
As we discussed offline, you could store it as a global to pass it between
Activities, and call Display.getInstance() here to retrieve it. But it doesn't
matter for now.

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:48: * @param
factory the factory to create implementation-dependent desktop view.
s/the/The/

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:56: if
(mDesktopViewFactory == null) {
Do we need a null-check here? Maybe use Preconditions.notNull()?

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

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:112: ((ViewGroup)
findViewById(R.id.desktop_view_placeholder)).addView((View) remoteHostDesktop);
On 2016/07/08 22:02:36, Lambros wrote:
> On 2016/07/08 19:58:56, Yuwei wrote:
> > On 2016/07/08 02:13:41, Lambros wrote:
> > > 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.
> > 
> > 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...
> 
> 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.
> 

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?

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java
(right):

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:31:
public abstract void showActionBar();
On 2016/07/08 22:02:36, Lambros wrote:
> Do showActionBar() and showKeyboard() need to be part of this interface? I'm
not
> sure why the View should care about these?
> Doesn't matter for this CL, but maybe these can be refactored out somehow?

Agreed. These functions are only being used by TouchInputHandler. showKeyboard
can be easily moved into TouchInputHandler while showActionBar needs reference
to the desktop activity. May need to pass the activity from DesktopView to the
constructor of TouchInputHandler.

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

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/2132883002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:112: ((ViewGroup)
findViewById(R.id.desktop_view_placeholder)).addView((View) remoteHostDesktop);
On 2016/07/08 23:07:54, Yuwei wrote:
> On 2016/07/08 22:02:36, Lambros wrote:
> > On 2016/07/08 19:58:56, Yuwei wrote:
> > > On 2016/07/08 02:13:41, Lambros wrote:
> > > > 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.
> > > 
> > > 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...
> > 
> > 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.
> > 
> 
> 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?

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?

[Yuwei, 2016-07-08 23:46:01]

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java
(right):

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:72:
/** An {@link Event} which is triggered when user touchs the screen. */
On 2016/07/08 22:02:36, Lambros wrote:
> s/touchs/touches/

Done.

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
File
remoting/android/java/src/org/chromium/chromoting/cardboard/DesktopActivity.java
(right):

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/cardboard/DesktopActivity.java:50:
// it into the constructor.
On 2016/07/08 22:02:36, Lambros wrote:
> As we discussed offline, you could store it as a global to pass it between
> Activities, and call Display.getInstance() here to retrieve it. But it doesn't
> matter for now.

Acknowledged.

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:48: * @param
factory the factory to create implementation-dependent desktop view.
On 2016/07/08 22:02:36, Lambros wrote:
> s/the/The/

Done.

https://codereview.chromium.org/2132883002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:56: if
(mDesktopViewFactory == null) {
On 2016/07/08 22:02:36, Lambros wrote:
> Do we need a null-check here? Maybe use Preconditions.notNull()?

Done.

[Yuwei, 2016-07-08 23:56:30]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-07-08 23:56:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/2132883002/#ps60001
(title: "Reviewer's Feedback")

[commit-bot: I haz the power, 2016-07-08 23:56:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuwei, 2016-07-09 00:01:50]

The CQ bit was unchecked by yuweih@chromium.org

[Yuwei, 2016-07-09 00:14:59]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-07-09 00:14:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/2132883002/#ps80001
(title: "Rebase ToT and do same fix for GlDisplay")

[commit-bot: I haz the power, 2016-07-09 00:15:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-09 01:13:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-09 01:13:20]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-09 01:15:22]

Description was changed from

==========
[Remoting Android] Placeholder for DesktopView

Preparation for using OpenGL ES rendering component on Android. This CL
leaves a FrameLayout placeholder for the DesktopView and creates the
implementation specific DesktopView in runtime.

BUG=385924
==========

to

==========
[Remoting Android] Placeholder for DesktopView

Preparation for using OpenGL ES rendering component on Android. This CL
leaves a FrameLayout placeholder for the DesktopView and creates the
implementation specific DesktopView in runtime.

BUG=385924

Committed: https://crrev.com/0822e24c49deda76b49076fe67f002ea8c66fc04
Cr-Commit-Position: refs/heads/master@{#404555}
==========

[commit-bot: I haz the power, 2016-07-09 01:15:23]

Patchset 5 (id:??) landed as
https://crrev.com/0822e24c49deda76b49076fe67f002ea8c66fc04
Cr-Commit-Position: refs/heads/master@{#404555}

[Hzj_jie, 2016-07-10 19:23:58]

zijiehe@chromium.org changed reviewers:
+ zijiehe@chromium.org

[Hzj_jie, 2016-07-10 19:23:59]

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:75: public
DesktopView(Context context, Display display) {
Since we are not using Android internal way to create AbstractDesktopView
anymore, I would suggest to include Client in the constructor parameter, and
remove init function.

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:52: private
void setDesktopViewFactory(DesktopViewFactory factory) {
Do we really need to have a DesktopViewFactory? It seems DesktopView would only
be created once. So you may be able to have a
private void setDesktopView(AbstractDesktopView view)
instead of
private void setDesktopViewFactory(DesktopViewFactory factory)

Is my understanding correctly?

[Hzj_jie, 2016-07-10 19:25:44]

Sorry, I have missed this code review on Friday. I have two comments
regarding to the constructor of DesktopView and DesktopViewFactory.

On Sun, Jul 10, 2016 at 12:23 PM, <zijiehe@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
> File remoting/android/java/src/org/chromium/chromoting/DesktopView.java
> (right):
>
>
>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
> remoting/android/java/src/org/chromium/chromoting/DesktopView.java:75:
> public DesktopView(Context context, Display display) {
> Since we are not using Android internal way to create
> AbstractDesktopView anymore, I would suggest to include Client in the
> constructor parameter, and remove init function.
>
>
>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
> File remoting/android/java/src/org/chromium/chromoting/jni/Client.java
> (right):
>
>
>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
> remoting/android/java/src/org/chromium/chromoting/jni/Client.java:52:
> private void setDesktopViewFactory(DesktopViewFactory factory) {
> Do we really need to have a DesktopViewFactory? It seems DesktopView
> would only be created once. So you may be able to have a
> private void setDesktopView(AbstractDesktopView view)
> instead of
> private void setDesktopViewFactory(DesktopViewFactory factory)
>
> Is my understanding correctly?
>
> https://codereview.chromium.org/2132883002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Hzj_jie, 2016-07-10 19:59:07]

And I cannot quite understand why you need to send the DesktopViewFactory
through native code.

On Sun, Jul 10, 2016 at 12:25 PM, Zijie He <zijiehe@chromium.org> wrote:

> Sorry, I have missed this code review on Friday. I have two comments
> regarding to the constructor of DesktopView and DesktopViewFactory.
>
> On Sun, Jul 10, 2016 at 12:23 PM, <zijiehe@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
>> File remoting/android/java/src/org/chromium/chromoting/DesktopView.java
>> (right):
>>
>>
>>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
>> remoting/android/java/src/org/chromium/chromoting/DesktopView.java:75:
>> public DesktopView(Context context, Display display) {
>> Since we are not using Android internal way to create
>> AbstractDesktopView anymore, I would suggest to include Client in the
>> constructor parameter, and remove init function.
>>
>>
>>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
>> File remoting/android/java/src/org/chromium/chromoting/jni/Client.java
>> (right):
>>
>>
>>
https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
>> remoting/android/java/src/org/chromium/chromoting/jni/Client.java:52:
>> private void setDesktopViewFactory(DesktopViewFactory factory) {
>> Do we really need to have a DesktopViewFactory? It seems DesktopView
>> would only be created once. So you may be able to have a
>> private void setDesktopView(AbstractDesktopView view)
>> instead of
>> private void setDesktopViewFactory(DesktopViewFactory factory)
>>
>> Is my understanding correctly?
>>
>> https://codereview.chromium.org/2132883002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yuwei, 2016-07-11 00:17:09]

For passing DesktopViewFactory to JNI, I think it can be simplified by passing
Client to createJavaDisplay and set up the DesktopViewFactory there.

I'll resolve your comments in a different CL. Thanks for reviewing this CL for
me :)

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:75: public
DesktopView(Context context, Display display) {
On 2016/07/10 19:23:59, Hzj_jie wrote:
> Since we are not using Android internal way to create AbstractDesktopView
> anymore, I would suggest to include Client in the constructor parameter, and
> remove init function.

Sure

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2132883002/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:52: private
void setDesktopViewFactory(DesktopViewFactory factory) {
On 2016/07/10 19:23:59, Hzj_jie wrote:
> Do we really need to have a DesktopViewFactory? It seems DesktopView would
only
> be created once. So you may be able to have a
> private void setDesktopView(AbstractDesktopView view)
> instead of
> private void setDesktopViewFactory(DesktopViewFactory factory)
> 
> Is my understanding correctly?

Activity will be destroyed and recreated when you rotate the screen, in which
case the view will be created for multiple times.

Also it needs an activity context when creating the view, which doesn't exist
when creating the Display.