Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2322623002:
[Remoting Android] Refactor GlDesktopView (Closed)
Description[Remoting Android] Refactor GlDesktopView
This CL further refactors the DesktopView design. Now DesktopView will have no
rendering related logic and there is no need to have placeholder for
implementation specific DesktopView.
BUG=641123
Committed: https://crrev.com/56ba6ed7f8898ea9b6fbc7a573a3adaaa404e591
Cr-Commit-Position: refs/heads/master@{#418337}
Patch Set 1 #Patch Set 2 : Add RenderController #
Total comments: 25
Patch Set 3 : Reviewer's Feedback #
Total comments: 2
Patch Set 4 : Fix processAnimation bug. Abort animation when detach #
Total comments: 35
Patch Set 5 : Reviewer's Feedback #
Total comments: 13
Patch Set 6 : Reviewer's Feedback #
Total comments: 10
Patch Set 7 : Reviewer's Feedback #
Total comments: 4
Patch Set 8 : Reviewer's Feedback #Messages
Total messages: 34 (10 generated)
Description was changed from ========== [Remoting Android] Refactor GlDesktopView ========== to ========== [Remoting Android] Refactor GlDesktopView This CL further refactors the DesktopView design. Now DesktopView will have no rendering related logic and there is no need to have placeholder for implementation specific DesktopView. BUG=641123 ==========
yuweih@chromium.org changed reviewers: + joedow@chromium.org, zijiehe@chromium.org
PTAL thanks! https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:34: public void init(Desktop desktop) { Maybe we can get rid of this function by casting the context to Desktop in the constructor? https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderController.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:44: mInputHandler.invalidate(); 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.
https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:17: * The abstract class for viewing and interacting with a specific remote host. Handles logic Change the comment here too since it is no longer abstract? https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:34: public void init(Desktop desktop) { On 2016/09/07 20:45:40, Yuwei wrote: > Maybe we can get rid of this function by casting the context to Desktop in the > constructor? Some good reasons to separate c'tor and init is if you have heavy lifting/complicated logic in the init method and want to return success/failure values, you want to do work in a base class but not in derived classes, you want to be able to inject/alters behaviors for testing, or the objects you want to pass in aren't available at construction time. My assumption is that these generally don't apply so simplifying it would be good if possible (and not hacky). https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderController.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:26: * @param stub The render stub. Must not be null. Is "Must not be null." required in the comment? I don't know that it is very helpful. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:37: * null. Remove the comments about null params and document the params as above (javadoc style) https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:44: mInputHandler.invalidate(); On 2016/09/07 20:45:40, Yuwei wrote: > 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. 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. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:254: mValid = true; Can't you just detach the event handlers / remove queued events when you want to stop processing? I think that would be cleaner than relying on a flag here.
https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:34: public void init(Desktop desktop) { On 2016/09/08 18:32:47, joedow wrote: > On 2016/09/07 20:45:40, Yuwei wrote: > > Maybe we can get rid of this function by casting the context to Desktop in the > > constructor? > > Some good reasons to separate c'tor and init is if you have heavy > lifting/complicated logic in the init method and want to return success/failure > values, you want to do work in a base class but not in derived classes, you want > to be able to inject/alters behaviors for testing, or the objects you want to > pass in aren't available at construction time. > > My assumption is that these generally don't apply so simplifying it would be > good if possible (and not hacky). So maybe just keep the init function? Looks like the documentation doesn't specify what Context should be when calling the c'tor. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:254: mValid = true; On 2016/09/08 18:32:47, joedow wrote: > Can't you just detach the event handlers / remove queued events when you want to > stop processing? I think that would be cleaner than relying on a flag here. The reason was that I didn't want to have a lot of private Objects for removing events. I think I can avoid this by using a HashMap.
https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:34: public void init(Desktop desktop) { On 2016/09/08 19:12:16, Yuwei wrote: > On 2016/09/08 18:32:47, joedow wrote: > > On 2016/09/07 20:45:40, Yuwei wrote: > > > Maybe we can get rid of this function by casting the context to Desktop in > the > > > constructor? > > > > Some good reasons to separate c'tor and init is if you have heavy > > lifting/complicated logic in the init method and want to return > success/failure > > values, you want to do work in a base class but not in derived classes, you > want > > to be able to inject/alters behaviors for testing, or the objects you want to > > pass in aren't available at construction time. > > > > My assumption is that these generally don't apply so simplifying it would be > > good if possible (and not hacky). > > So maybe just keep the init function? Looks like the documentation doesn't > specify what Context should be when calling the c'tor. If you need to use Desktop instead of Context, you need to use a secondary init function. The input Context is a ContextWrapper of the Activity, but the implementation may be changed later. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderController.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:13: public class RenderController { 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. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:48: mInputHandler.init(desktop, mRenderStub, new InputEventSender(mClient)); 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. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:27: private DesktopCanvas mDesktopCanvas; Once init function has been removed, these fields can be final. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:254: mValid = true; On 2016/09/08 19:12:16, Yuwei wrote: > On 2016/09/08 18:32:47, joedow wrote: > > Can't you just detach the event handlers / remove queued events when you want > to > > stop processing? I think that would be cleaner than relying on a flag here. > > The reason was that I didn't want to have a lot of private Objects for removing > events. I think I can avoid this by using a HashMap. Emmm ... Seems you are writing more codes by using addSelfRemovable here.
ptal https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:17: * The abstract class for viewing and interacting with a specific remote host. Handles logic On 2016/09/08 18:32:47, joedow wrote: > Change the comment here too since it is no longer abstract? Done. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:34: public void init(Desktop desktop) { On 2016/09/08 19:12:16, Yuwei wrote: > On 2016/09/08 18:32:47, joedow wrote: > > On 2016/09/07 20:45:40, Yuwei wrote: > > > Maybe we can get rid of this function by casting the context to Desktop in > the > > > constructor? > > > > Some good reasons to separate c'tor and init is if you have heavy > > lifting/complicated logic in the init method and want to return > success/failure > > values, you want to do work in a base class but not in derived classes, you > want > > to be able to inject/alters behaviors for testing, or the objects you want to > > pass in aren't available at construction time. > > > > My assumption is that these generally don't apply so simplifying it would be > > good if possible (and not hacky). > > So maybe just keep the init function? Looks like the documentation doesn't > specify what Context should be when calling the c'tor. Acknowledged. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderController.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:13: public class RenderController { On 2016/09/08 21:37:33, Hzj_jie wrote: > I cannot quite tell the functionality of this class. 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. > Does it just create mInputHandler each time a new DesktopView created? Yes > If so, why cannot we just make InputHandler outlive? 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. > And it seems this class has no relationship with Render, it just forwards > objects to mInputHandler. I also have concern about its name. Obsolete. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:26: * @param stub The render stub. Must not be null. On 2016/09/08 18:32:47, joedow wrote: > Is "Must not be null." required in the comment? I don't know that it is very > helpful. Obsolete. Class removed. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:37: * null. On 2016/09/08 18:32:47, joedow wrote: > Remove the comments about null params and document the params as above (javadoc > style) Obsolete. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:44: mInputHandler.invalidate(); On 2016/09/08 18:32:47, joedow wrote: > On 2016/09/07 20:45:40, Yuwei wrote: > > 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. > > 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. > Done. Added attach/detach interface to DesktopView and TouchInputHandler becomes part of DesktopView again. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderController.java:48: mInputHandler.init(desktop, mRenderStub, new InputEventSender(mClient)); On 2016/09/08 21:37:33, Hzj_jie wrote: > If you really want to keep current design, Not keeping :P > you may want to merge InputHandler.ctor and init function. > It does not need a secondary initialization. Done. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:27: private DesktopCanvas mDesktopCanvas; On 2016/09/08 21:37:33, Hzj_jie wrote: > Once init function has been removed, these fields can be final. Done. Made everything final-able final. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:254: mValid = true; On 2016/09/08 21:37:33, Hzj_jie wrote: > On 2016/09/08 19:12:16, Yuwei wrote: > > On 2016/09/08 18:32:47, joedow wrote: > > > Can't you just detach the event handlers / remove queued events when you > want > > to > > > stop processing? I think that would be cleaner than relying on a flag here. > > > > The reason was that I didn't want to have a lot of private Objects for > removing > > events. I think I can avoid this by using a HashMap. > > Emmm ... Seems you are writing more codes by using addSelfRemovable here. Simplified. https://codereview.chromium.org/2322623002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:254: mValid = true; On 2016/09/08 19:12:16, Yuwei wrote: > I think I can avoid this by using a HashMap. s/HashMap/list of listener-event pairs/ https://codereview.chromium.org/2322623002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:238: attachEvent(mViewer.onTouch(), new Event.ParameterRunnable<TouchEventParameter>() { So annoying that the N SDK still hasn't been included. These code can be simplified a lot using the lambda syntax...
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
https://codereview.chromium.org/2322623002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:238: attachEvent(mViewer.onTouch(), new Event.ParameterRunnable<TouchEventParameter>() { On 2016/09/08 23:09:15, Yuwei wrote: > So annoying that the N SDK still hasn't been included. These code can be > simplified a lot using the lambda syntax... Yes, unfortunately. () -> {} looks extremely better. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Desktop.java:148: super.onDestroy(); Though there should be zero impact, super.onDestroy() at the end is a best practice. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:43: Preconditions.notNull(renderStub); You do not need to notNull renderStub, as the it will be dereferred soon. You do not really need to notNull client, as it will be covered by InputEventSender, but up to you. You may want to check whether mDesktop is not null, otherwise, something must be wrong. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:44: private final FlingAnimationJob mCursorAnimationJob, mScrollAnimationJob; IMO, placing two variables in a same definition line is not good. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:192: } Functions should be placed after constructors. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:53: mRenderStub = stub; Add Preconditions here. If I have made a mistake, this is an one-off function. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:56: public RenderStub getRenderStub() { And here. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:61: public static Client getInstance() { Suggest to add a Preconditions.notNull here, and update the comment. Consumer (Desktop activity) always assumes it returns a valid instance. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; Is this volatile useful? I have not seen this pointer been changed somewhere. And you can make this final. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private InputFeedbackRadiusMapper mFeedbackRadiusMapper; Move regular fields after final fields. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:82: if (mNativeJniGlDisplay != 0) { Is there any chance mNativeJniGlDisplay == 0 / nullptr? I have not found a native caller will send nullptr for this variable.
https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:43: Preconditions.notNull(renderStub); On 2016/09/10 02:06:32, Hzj_jie wrote: > You do not need to notNull renderStub, as the it will be dereferred soon. > > You do not really need to notNull client, as it will be covered by > InputEventSender, but up to you. > > You may want to check whether mDesktop is not null, otherwise, something must be > wrong. You mean checking whether mDesktop is null so that attach won't be called twice? https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/10 02:06:32, Hzj_jie wrote: > Is this volatile useful? I have not seen this pointer been changed somewhere. > And you can make this final. There is a small design issue with JniGlDisplayHandler. invalidate() is called by the native code from the display thread. volatile in Java can guarantee assignment and referencing are atomic. This isn't very thread-safe though if invalidate() is called between the nullptr check and calling the native function. I'm considering making JniGlDisplayHandler call invalidate() from UI. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:82: if (mNativeJniGlDisplay != 0) { On 2016/09/10 02:06:32, Hzj_jie wrote: > Is there any chance mNativeJniGlDisplay == 0 / nullptr? I have not found a > native caller will send nullptr for this variable. invalidate()
ptal https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Desktop.java:148: super.onDestroy(); On 2016/09/10 02:06:32, Hzj_jie wrote: > Though there should be zero impact, super.onDestroy() at the end is a best > practice. Done. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:43: Preconditions.notNull(renderStub); On 2016/09/10 02:06:32, Hzj_jie wrote: > You do not need to notNull renderStub, as the it will be dereferred soon. > > You do not really need to notNull client, as it will be covered by > InputEventSender, but up to you. > > You may want to check whether mDesktop is not null, otherwise, something must be > wrong. Done. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:44: private final FlingAnimationJob mCursorAnimationJob, mScrollAnimationJob; On 2016/09/10 02:06:32, Hzj_jie wrote: > IMO, placing two variables in a same definition line is not good. Done. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:192: } On 2016/09/10 02:06:32, Hzj_jie wrote: > Functions should be placed after constructors. Done. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:53: mRenderStub = stub; On 2016/09/10 02:06:32, Hzj_jie wrote: > Add Preconditions here. If I have made a mistake, this is an one-off function. Done. BTW why is the Precondition implemented by throwing exceptions instead of doing asserts? https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:56: public RenderStub getRenderStub() { On 2016/09/10 02:06:32, Hzj_jie wrote: > And here. Done. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:61: public static Client getInstance() { On 2016/09/10 02:06:32, Hzj_jie wrote: > Suggest to add a Preconditions.notNull here, and update the comment. Consumer > (Desktop activity) always assumes it returns a valid instance. I think it may make more sense to check it in Desktop, since this function might be useful in the future for checking whether the instance has been created. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/10 02:06:32, Hzj_jie wrote: > Is this volatile useful? I have not seen this pointer been changed somewhere. > And you can make this final. I'll consider fixing this problem after this CL is submitted. I can either: * Make JniGlDisplay destroy on the UI thread and call invalidate() on UI * Or reverse the ownership direction and control the lifetime of the native class from Java like Client does. IIRC the reason we made the native Display control the lifetime of the Java counterpart was to make it easier for the native code to decide which renderer instance to create, but now it's unnecessary since we only have one renderer. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private InputFeedbackRadiusMapper mFeedbackRadiusMapper; On 2016/09/10 02:06:32, Hzj_jie wrote: > Move regular fields after final fields. Done. Just moving mNativeJniGlDisplay below?
https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/12 18:56:14, Yuwei wrote: > On 2016/09/10 02:06:32, Hzj_jie wrote: > > Is this volatile useful? I have not seen this pointer been changed somewhere. > > And you can make this final. > > I'll consider fixing this problem after this CL is submitted. I can either: > > * Make JniGlDisplay destroy on the UI thread and call invalidate() on UI > * Or reverse the ownership direction and control the lifetime of the native > class from Java like Client does. IIRC the reason we made the native Display > control the lifetime of the Java counterpart was to make it easier for the > native code to decide which renderer instance to create, but now it's > unnecessary since we only have one renderer. Looks like I was wrong with #2. The reason was to avoid passing native pointer of JniGlDisplay back and forth between GlDisplay and Client.
https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:19: * The class for viewing and interacting with a specific remote host. IIUC the lifetime for this class is longer than the current activity, if so then I think a comment should be added to describe the lifetime of an instance of this class as well as the expectations that users of it should have (i.e. attach when the view is created and call detach when the view is destroyed). https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:52: * when calling this function. nit: s/when/prior to https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:60: Preconditions.notNull(mRenderStub); Is this precondition needed? I'm wondering if someone would ever want to call getRenderStub() and check against null to see if it has been set. https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private volatile long mNativeJniGlDisplay; Can you initialize |mNativeJniGlDisplay| to a reasonable default?
https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:19: * The class for viewing and interacting with a specific remote host. On 2016/09/12 19:55:08, joedow wrote: > IIUC the lifetime for this class is longer than the current activity, I think it should be roughly the same as the activity? https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private volatile long mNativeJniGlDisplay; On 2016/09/12 19:55:08, joedow wrote: > Can you initialize |mNativeJniGlDisplay| to a reasonable default? It used to be 0 but Zijie thought it is unnecessary since long will be initialized to 0 by default...
https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:19: * The class for viewing and interacting with a specific remote host. On 2016/09/12 20:57:18, Yuwei wrote: > On 2016/09/12 19:55:08, joedow wrote: > > IIUC the lifetime for this class is longer than the current activity, > > I think it should be roughly the same as the activity? It would be good to confirm and document. I had assumed it would outlive the activity (which is destroyed for events such as rotation changes) since the view attaches/detaches when it is created/destroyed. https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private volatile long mNativeJniGlDisplay; On 2016/09/12 20:57:18, Yuwei wrote: > On 2016/09/12 19:55:08, joedow wrote: > > Can you initialize |mNativeJniGlDisplay| to a reasonable default? > > It used to be 0 but Zijie thought it is unnecessary since long will be > initialized to 0 by default... It isn't necessary but it documents intent. Probably just a style preference. You can ignore this if you want.
https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:43: Preconditions.notNull(renderStub); On 2016/09/12 18:56:13, Yuwei wrote: > On 2016/09/10 02:06:32, Hzj_jie wrote: > > You do not need to notNull renderStub, as the it will be dereferred soon. > > > > You do not really need to notNull client, as it will be covered by > > InputEventSender, but up to you. > > > > You may want to check whether mDesktop is not null, otherwise, something must > be > > wrong. > > Done. Yes, I think so. Is my understanding correct? https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:53: mRenderStub = stub; On 2016/09/12 18:56:14, Yuwei wrote: > On 2016/09/10 02:06:32, Hzj_jie wrote: > > Add Preconditions here. If I have made a mistake, this is an one-off function. > > Done. > > BTW why is the Precondition implemented by throwing exceptions instead of doing > asserts? In Java, assertions take effect only with -ea option on java command, or debug.assert == 1 in Android. Otherwise it won't evaluate the expression. http://stackoverflow.com/questions/2364910/can-i-use-assert-on-android-devices But throwing exceptions is a safer way to ensure the expression always being evaluated. This implementation is copied from guava, a Google java library, @ https://github.com/google/guava/blob/master/guava/src/com/google/common/base/.... https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:61: public static Client getInstance() { On 2016/09/12 18:56:13, Yuwei wrote: > On 2016/09/10 02:06:32, Hzj_jie wrote: > > Suggest to add a Preconditions.notNull here, and update the comment. Consumer > > (Desktop activity) always assumes it returns a valid instance. > > I think it may make more sense to check it in Desktop, since this function might > be useful in the future for checking whether the instance has been created. Yes, both OK. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/12 19:13:14, Yuwei wrote: > On 2016/09/12 18:56:14, Yuwei wrote: > > On 2016/09/10 02:06:32, Hzj_jie wrote: > > > Is this volatile useful? I have not seen this pointer been changed > somewhere. > > > And you can make this final. > > > > I'll consider fixing this problem after this CL is submitted. I can either: > > > > * Make JniGlDisplay destroy on the UI thread and call invalidate() on UI > > * Or reverse the ownership direction and control the lifetime of the native > > class from Java like Client does. IIRC the reason we made the native Display > > control the lifetime of the Java counterpart was to make it easier for the > > native code to decide which renderer instance to create, but now it's > > unnecessary since we only have one renderer. > > Looks like I was wrong with #2. The reason was to avoid passing native pointer > of JniGlDisplay back and forth between GlDisplay and Client. Emmm, interesting, code search won't display all references when searching mNativeJniGlDisplay, so I lost two references. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/12 18:56:14, Yuwei wrote: > On 2016/09/10 02:06:32, Hzj_jie wrote: > > Is this volatile useful? I have not seen this pointer been changed somewhere. > > And you can make this final. > > I'll consider fixing this problem after this CL is submitted. I can either: > > * Make JniGlDisplay destroy on the UI thread and call invalidate() on UI > * Or reverse the ownership direction and control the lifetime of the native > class from Java like Client does. IIRC the reason we made the native Display > control the lifetime of the Java counterpart was to make it easier for the > native code to decide which renderer instance to create, but now it's > unnecessary since we only have one renderer. I have had a look at JniGlDisplayHandler, and have several questions, 1. JniGlDisplayHandler should only be destroyed on display thread, otherwise all other On* functions are not safe. Is that correct? 2. Even you invalidate the pointer in UI thread, it's still not safe. As the invalidate function may be executed after the destructor. i.e. You may execute some renderer_.GetWeakPtr() after renderer_ has been released. 3. jni.Client.destroy() should call Desktop.onDestroy() and DesktopView.onDestroy(), JniClient.DisconnectFromHost(), JniGlDisplayHandler.~JniGlDisplayHandler sequentially. So you probably be safe without need to have the invalidate function in GlDisplay. i.e. Nobody is able to call GlDisplay functions after JniGlDisplayHandler's destructor. Would you mind to confirm it? https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private InputFeedbackRadiusMapper mFeedbackRadiusMapper; On 2016/09/12 18:56:14, Yuwei wrote: > On 2016/09/10 02:06:32, Hzj_jie wrote: > > Move regular fields after final fields. > > Done. Just moving mNativeJniGlDisplay below? Since mNativeJniGlDisplay needs to be mutable, this comment is out-of-date. https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private volatile long mNativeJniGlDisplay; On 2016/09/12 21:01:56, joedow wrote: > On 2016/09/12 20:57:18, Yuwei wrote: > > On 2016/09/12 19:55:08, joedow wrote: > > > Can you initialize |mNativeJniGlDisplay| to a reasonable default? > > > > It used to be 0 but Zijie thought it is unnecessary since long will be > > initialized to 0 by default... > > It isn't necessary but it documents intent. Probably just a style preference. > You can ignore this if you want. Oh, I saw the initialization and definition of most of variables are separately in our source code. So I added this comment before. But I do suggest to initialize variables in constructor instead of definitions, to avoid any potential reinitialization or initialization ordering issues. I do not have strong opinion on this. Feel free to add or ignore it.
https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:43: Preconditions.notNull(renderStub); On 2016/09/12 21:28:04, Hzj_jie wrote: > On 2016/09/12 18:56:13, Yuwei wrote: > > On 2016/09/10 02:06:32, Hzj_jie wrote: > > > You do not need to notNull renderStub, as the it will be dereferred soon. > > > > > > You do not really need to notNull client, as it will be covered by > > > InputEventSender, but up to you. > > > > > > You may want to check whether mDesktop is not null, otherwise, something > must > > be > > > wrong. > > > > Done. > > Yes, I think so. Is my understanding correct? Asserting mDesktop is null? Yes. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:53: mRenderStub = stub; On 2016/09/12 21:28:04, Hzj_jie wrote: > On 2016/09/12 18:56:14, Yuwei wrote: > > On 2016/09/10 02:06:32, Hzj_jie wrote: > > > Add Preconditions here. If I have made a mistake, this is an one-off > function. > > > > Done. > > > > BTW why is the Precondition implemented by throwing exceptions instead of > doing > > asserts? > > In Java, assertions take effect only with -ea option on java command, or > debug.assert == 1 in Android. Otherwise it won't evaluate the expression. > http://stackoverflow.com/questions/2364910/can-i-use-assert-on-android-devices > But throwing exceptions is a safer way to ensure the expression always being > evaluated. > This implementation is copied from guava, a Google java library, @ > https://github.com/google/guava/blob/master/guava/src/com/google/common/base/.... Acknowledged. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/12 21:28:04, Hzj_jie wrote: > On 2016/09/12 18:56:14, Yuwei wrote: > > On 2016/09/10 02:06:32, Hzj_jie wrote: > > > Is this volatile useful? I have not seen this pointer been changed > somewhere. > > > And you can make this final. > > > > I'll consider fixing this problem after this CL is submitted. I can either: > > > > * Make JniGlDisplay destroy on the UI thread and call invalidate() on UI > > * Or reverse the ownership direction and control the lifetime of the native > > class from Java like Client does. IIRC the reason we made the native Display > > control the lifetime of the Java counterpart was to make it easier for the > > native code to decide which renderer instance to create, but now it's > > unnecessary since we only have one renderer. > > I have had a look at JniGlDisplayHandler, and have several questions, > 1. JniGlDisplayHandler should only be destroyed on display thread, otherwise all > other On* functions are not safe. Is that correct? Something like that. Basically its WeakFactory lives on the display thread and therefore must be destroyed on destroyed on the display thread. > 2. Even you invalidate the pointer in UI thread, it's still not safe. As the > invalidate function may be executed after the destructor. i.e. You may execute > some renderer_.GetWeakPtr() after renderer_ has been released. You may check my fix: https://codereview.chromium.org/2338473002/ Destroy() is called directly on UI and it calls the destructor. The destructor is marked as private. > 3. jni.Client.destroy() should call Desktop.onDestroy() and > DesktopView.onDestroy(), JniClient.DisconnectFromHost(), > JniGlDisplayHandler.~JniGlDisplayHandler sequentially. So you probably be safe > without need to have the invalidate function in GlDisplay. i.e. Nobody is able > to call GlDisplay functions after JniGlDisplayHandler's destructor. Would you > mind to confirm it? Desktop.onDestroy() is called by android and may be called after invalidate(). Also from the standpoint of GlDisplay it is better to design it that way to prevent possible mistake. https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private InputFeedbackRadiusMapper mFeedbackRadiusMapper; On 2016/09/12 21:28:04, Hzj_jie wrote: > On 2016/09/12 18:56:14, Yuwei wrote: > > On 2016/09/10 02:06:32, Hzj_jie wrote: > > > Move regular fields after final fields. > > > > Done. Just moving mNativeJniGlDisplay below? > > Since mNativeJniGlDisplay needs to be mutable, this comment is out-of-date. Acknowledged.
ptal https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:19: * The class for viewing and interacting with a specific remote host. On 2016/09/12 21:01:56, joedow wrote: > On 2016/09/12 20:57:18, Yuwei wrote: > > On 2016/09/12 19:55:08, joedow wrote: > > > IIUC the lifetime for this class is longer than the current activity, > > > > I think it should be roughly the same as the activity? > > It would be good to confirm and document. Confirm that it's recreated when the activity is recreated. Added some comments about the lifetime. > I had assumed it would outlive the activity (which is destroyed for events > such as rotation changes) since the view attaches/detaches when it is > created/destroyed. It was basically like "attaching the view to the RenderStub" and "detaching the view from the RenderStub". I've changed the design a little bit. I override the view's attach/detach function to do these things. https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:52: * when calling this function. On 2016/09/12 19:55:08, joedow wrote: > nit: s/when/prior to Obsolete. https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/Client.java:60: Preconditions.notNull(mRenderStub); On 2016/09/12 19:55:08, joedow wrote: > Is this precondition needed? I'm wondering if someone would ever want to call > getRenderStub() and check against null to see if it has been set. Removed. https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:36: private volatile long mNativeJniGlDisplay; On 2016/09/12 21:28:04, Hzj_jie wrote: > On 2016/09/12 21:01:56, joedow wrote: > > On 2016/09/12 20:57:18, Yuwei wrote: > > > On 2016/09/12 19:55:08, joedow wrote: > > > > Can you initialize |mNativeJniGlDisplay| to a reasonable default? > > > > > > It used to be 0 but Zijie thought it is unnecessary since long will be > > > initialized to 0 by default... > > > > It isn't necessary but it documents intent. Probably just a style preference. > > > You can ignore this if you want. > > Oh, I saw the initialization and definition of most of variables are separately > in our source code. So I added this comment before. > But I do suggest to initialize variables in constructor instead of definitions, > to avoid any potential reinitialization or initialization ordering issues. > > I do not have strong opinion on this. Feel free to add or ignore it. ``` private GlDisplay(long nativeJniGlDisplay) { mNativeJniGlDisplay = nativeJniGlDisplay; } ``` Given this ctor probably we don't need to change anything...
lgtm once my comments are addressed. https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right): https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Desktop.java:32: * background. Since you removed the attach/detach methods and this class has the same lifetime as the activity, I don't think the additional comment is needed now. https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:20: * is roughly the same as the Desktop activity. What does 'roughly' mean? What should be documented is whether an instance is tied to the activity (i.e. there is a 1:1 mapping between class and activity) or if it lives past that. If it lives about the same amount of time as the other activity bound classes, then you don't need this comment. https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:65: Preconditions.notNull(mInputHandler); I think the check on line 57 is fine (to prevent multiple attaches) however if there are multiple detaches, you are going to detect it really fast (when the app crashes on the next line). I'm not sure this check is useful here. https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:193: Preconditions.notNull(renderStub); Why only check if the last two params are non-null?
https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:20: * is roughly the same as the Desktop activity. On 2016/09/13 00:08:51, joedow wrote: > What does 'roughly' mean? What should be documented is whether an instance is > tied to the activity (i.e. there is a 1:1 mapping between class and activity) or > if it lives past that. Sorry for the vagueness. Here are the details: * The constructor is called inside Desktop.onCreate() by setContentView(), and init() is called later also inside onCreate. * onAttachedToWindow is called slightly after onCreate() by a task runner. * onDetachedFromWindow is called slightly after onDestroy() by a task runner, and before constructing the new Desktop activity. https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:65: Preconditions.notNull(mInputHandler); On 2016/09/13 00:08:51, joedow wrote: > I think the check on line 57 is fine (to prevent multiple attaches) however if > there are multiple detaches, you are going to detect it really fast (when the > app crashes on the next line). I'm not sure this check is useful here. detachEventListeners() doesn't check whether it has already called since what it does is just clearing the list... Shall I also add assert that the event list is not empty? Arguably we may not need to add any check here since onAttachedToWindow and onDetachedFromWindow are called by the Android library.
Thanks! And @zijiehe PTAL https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right): https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Desktop.java:32: * background. On 2016/09/13 00:08:50, joedow wrote: > Since you removed the attach/detach methods and this class has the same lifetime > as the activity, I don't think the additional comment is needed now. Removed. Actually this is the activity class... https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:20: * is roughly the same as the Desktop activity. On 2016/09/13 00:08:51, joedow wrote: > If it lives about the same amount of time as the other > activity bound classes, then you don't need this comment. Removed. https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:65: Preconditions.notNull(mInputHandler); On 2016/09/13 00:08:51, joedow wrote: > I think the check on line 57 is fine (to prevent multiple attaches) however if > there are multiple detaches, you are going to detect it really fast (when the > app crashes on the next line). I'm not sure this check is useful here. Removed this check and added emptiness check in detachEventListeners(). https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java (right): https://codereview.chromium.org/2322623002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:193: Preconditions.notNull(renderStub); On 2016/09/13 00:08:51, joedow wrote: > Why only check if the last two params are non-null? Added checks for the first two. Probably because they were first added in the init() function which was later merged into the ctor.
lgtm https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/12 21:50:04, Yuwei wrote: > On 2016/09/12 21:28:04, Hzj_jie wrote: > > On 2016/09/12 18:56:14, Yuwei wrote: > > > On 2016/09/10 02:06:32, Hzj_jie wrote: > > > > Is this volatile useful? I have not seen this pointer been changed > > somewhere. > > > > And you can make this final. > > > > > > I'll consider fixing this problem after this CL is submitted. I can either: > > > > > > * Make JniGlDisplay destroy on the UI thread and call invalidate() on UI > > > * Or reverse the ownership direction and control the lifetime of the native > > > class from Java like Client does. IIRC the reason we made the native Display > > > control the lifetime of the Java counterpart was to make it easier for the > > > native code to decide which renderer instance to create, but now it's > > > unnecessary since we only have one renderer. > > > > I have had a look at JniGlDisplayHandler, and have several questions, > > 1. JniGlDisplayHandler should only be destroyed on display thread, otherwise > all > > other On* functions are not safe. Is that correct? > > Something like that. Basically its WeakFactory lives on the display thread and > therefore must be destroyed on destroyed on the display thread. > > > 2. Even you invalidate the pointer in UI thread, it's still not safe. As the > > invalidate function may be executed after the destructor. i.e. You may execute > > some renderer_.GetWeakPtr() after renderer_ has been released. > > You may check my fix: https://codereview.chromium.org/2338473002/ > > Destroy() is called directly on UI and it calls the destructor. The destructor > is marked as private. > > > 3. jni.Client.destroy() should call Desktop.onDestroy() and > > DesktopView.onDestroy(), JniClient.DisconnectFromHost(), > > JniGlDisplayHandler.~JniGlDisplayHandler sequentially. So you probably be safe > > without need to have the invalidate function in GlDisplay. i.e. Nobody is able > > to call GlDisplay functions after JniGlDisplayHandler's destructor. Would you > > mind to confirm it? > > Desktop.onDestroy() is called by android and may be called after invalidate(). > Also from the standpoint of GlDisplay it is better to design it that way to > prevent possible mistake. I just guessed Activity.onDestroy() and View.onDestroy() should be called in the same thread without interruption. But since you have implemented a safer solution without this assumption, I am good with it. https://codereview.chromium.org/2322623002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:21: public class DesktopView extends SurfaceView { Will this class be final? https://codereview.chromium.org/2322623002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:68: // TODO(yuweih): move showActionBar and showKeyboard out of this abstract class. Remove 'abstract'
Thanks! https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2322623002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:30: private volatile long mNativeJniGlDisplay; On 2016/09/13 18:00:13, Hzj_jie wrote: > On 2016/09/12 21:50:04, Yuwei wrote: > > On 2016/09/12 21:28:04, Hzj_jie wrote: > > > On 2016/09/12 18:56:14, Yuwei wrote: > > > > On 2016/09/10 02:06:32, Hzj_jie wrote: > > > > > Is this volatile useful? I have not seen this pointer been changed > > > somewhere. > > > > > And you can make this final. > > > > > > > > I'll consider fixing this problem after this CL is submitted. I can > either: > > > > > > > > * Make JniGlDisplay destroy on the UI thread and call invalidate() on UI > > > > * Or reverse the ownership direction and control the lifetime of the > native > > > > class from Java like Client does. IIRC the reason we made the native > Display > > > > control the lifetime of the Java counterpart was to make it easier for the > > > > native code to decide which renderer instance to create, but now it's > > > > unnecessary since we only have one renderer. > > > > > > I have had a look at JniGlDisplayHandler, and have several questions, > > > 1. JniGlDisplayHandler should only be destroyed on display thread, otherwise > > all > > > other On* functions are not safe. Is that correct? > > > > Something like that. Basically its WeakFactory lives on the display thread and > > therefore must be destroyed on destroyed on the display thread. > > > > > 2. Even you invalidate the pointer in UI thread, it's still not safe. As the > > > invalidate function may be executed after the destructor. i.e. You may > execute > > > some renderer_.GetWeakPtr() after renderer_ has been released. > > > > You may check my fix: https://codereview.chromium.org/2338473002/ > > > > Destroy() is called directly on UI and it calls the destructor. The destructor > > is marked as private. > > > > > 3. jni.Client.destroy() should call Desktop.onDestroy() and > > > DesktopView.onDestroy(), JniClient.DisconnectFromHost(), > > > JniGlDisplayHandler.~JniGlDisplayHandler sequentially. So you probably be > safe > > > without need to have the invalidate function in GlDisplay. i.e. Nobody is > able > > > to call GlDisplay functions after JniGlDisplayHandler's destructor. Would > you > > > mind to confirm it? > > > > Desktop.onDestroy() is called by android and may be called after invalidate(). > > Also from the standpoint of GlDisplay it is better to design it that way to > > prevent possible mistake. > > I just guessed Activity.onDestroy() and View.onDestroy() should be called in the same thread without interruption. Should be now GlDisplay.invalidate() vs View.onDetachedFromWindow(). They don't need to have causality. > But since you have implemented a safer > solution without this assumption, I am good with it. Acknowledged. https://codereview.chromium.org/2322623002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2322623002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:21: public class DesktopView extends SurfaceView { On 2016/09/13 18:00:13, Hzj_jie wrote: > Will this class be final? Done. Marked as final. https://codereview.chromium.org/2322623002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:68: // TODO(yuweih): move showActionBar and showKeyboard out of this abstract class. On 2016/09/13 18:00:13, Hzj_jie wrote: > Remove 'abstract' Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org, zijiehe@chromium.org Link to the patchset: https://codereview.chromium.org/2322623002/#ps140001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Refactor GlDesktopView This CL further refactors the DesktopView design. Now DesktopView will have no rendering related logic and there is no need to have placeholder for implementation specific DesktopView. BUG=641123 ========== to ========== [Remoting Android] Refactor GlDesktopView This CL further refactors the DesktopView design. Now DesktopView will have no rendering related logic and there is no need to have placeholder for implementation specific DesktopView. BUG=641123 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Refactor GlDesktopView This CL further refactors the DesktopView design. Now DesktopView will have no rendering related logic and there is no need to have placeholder for implementation specific DesktopView. BUG=641123 ========== to ========== [Remoting Android] Refactor GlDesktopView This CL further refactors the DesktopView design. Now DesktopView will have no rendering related logic and there is no need to have placeholder for implementation specific DesktopView. BUG=641123 Committed: https://crrev.com/56ba6ed7f8898ea9b6fbc7a573a3adaaa404e591 Cr-Commit-Position: refs/heads/master@{#418337} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/56ba6ed7f8898ea9b6fbc7a573a3adaaa404e591 Cr-Commit-Position: refs/heads/master@{#418337} |
