Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2297073002:
[Remoting Android] Move feedback type to radius logic out of DesktopView (Closed)
Description[Remoting Android] Move feedback type to radius logic out of DesktopView
The feedback-type-to-radius logic should be renderer specific so it doesn't
quite make sense to define the conversion function inside the view. This CL
creates a standalone class for handling feedback type to radius conversion.
BUG=641123
Committed: https://crrev.com/60961fad23f2bab7372211fd971af813df74650d
Cr-Commit-Position: refs/heads/master@{#415810}
Patch Set 1 #Patch Set 2 : Add mapper #
Total comments: 10
Patch Set 3 : Reviewer's Feedback #
Total comments: 4
Patch Set 4 : Reviewer's Feedback #Patch Set 5 : Merge ToT #
Messages
Total messages: 22 (8 generated)
yuweih@chromium.org changed reviewers: + joedow@chromium.org, zijiehe@chromium.org
This change is less controversial than the change of RenderStub so I just make this a separate CL. PTAL. Thanks!
https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:28: private float mScaleFactor = 0; I believe usually you do not need to actively initialize a float to 0. Java itself will do it for you. https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:15: public InputFeedbackRadiusMapper(DesktopView view) { DesktopView -> View https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:30: public float getFeedbackRadius(DesktopView.InputFeedbackType feedbackToShow, Won't InputFeedbackType be in RenderStub? I suppose you will modify the RenderStub change, right?
https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:28: private float mScaleFactor = 0; On 2016/08/31 01:31:59, Hzj_jie wrote: > I believe usually you do not need to actively initialize a float to 0. Java > itself will do it for you. I think you are right... Remove this? https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:30: public float getFeedbackRadius(DesktopView.InputFeedbackType feedbackToShow, On 2016/08/31 01:31:59, Hzj_jie wrote: > Won't InputFeedbackType be in RenderStub? Probably not. I think it makes more sense to be a standalone class. > I suppose you will modify the RenderStub change, right? Yes. It will be merged with this change.
https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:28: private float mScaleFactor = 0; On 2016/08/31 01:36:13, Yuwei wrote: > On 2016/08/31 01:31:59, Hzj_jie wrote: > > I believe usually you do not need to actively initialize a float to 0. Java > > itself will do it for you. > > I think you are right... Remove this? Yes, I suggest to do so to keep consistent. And I believe usually we put final fields before others. So you may want to move mMapper before mOnHostSizeChangedListenerKey. https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:30: public float getFeedbackRadius(DesktopView.InputFeedbackType feedbackToShow, On 2016/08/31 01:36:13, Yuwei wrote: > On 2016/08/31 01:31:59, Hzj_jie wrote: > > Won't InputFeedbackType be in RenderStub? > > Probably not. I think it makes more sense to be a standalone class. > > > I suppose you will modify the RenderStub change, right? > > Yes. It will be merged with this change. No, I mean DesktopView.InputFeedbackType -> RenderStub.InputFeedbackType, right?
Patchset #4 (id:60001) has been deleted
ptal https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:28: private float mScaleFactor = 0; On 2016/08/31 01:38:43, Hzj_jie wrote: > On 2016/08/31 01:36:13, Yuwei wrote: > > On 2016/08/31 01:31:59, Hzj_jie wrote: > > > I believe usually you do not need to actively initialize a float to 0. Java > > > itself will do it for you. > > > > I think you are right... Remove this? > > Yes, I suggest to do so to keep consistent. And I believe usually we put final > fields before others. So you may want to move mMapper before > mOnHostSizeChangedListenerKey. Done. https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java (right): https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:15: public InputFeedbackRadiusMapper(DesktopView view) { On 2016/08/31 01:31:59, Hzj_jie wrote: > DesktopView -> View Done. https://codereview.chromium.org/2297073002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:30: public float getFeedbackRadius(DesktopView.InputFeedbackType feedbackToShow, On 2016/08/31 01:38:43, Hzj_jie wrote: > On 2016/08/31 01:36:13, Yuwei wrote: > > On 2016/08/31 01:31:59, Hzj_jie wrote: > > > Won't InputFeedbackType be in RenderStub? > > > > Probably not. I think it makes more sense to be a standalone class. > > > > > I suppose you will modify the RenderStub change, right? > > > > Yes. It will be merged with this change. > > No, I mean DesktopView.InputFeedbackType -> RenderStub.InputFeedbackType, right? Yep.
ping
lgtm
lgtm once feedback is addressed. https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:41: mMapper = new InputFeedbackRadiusMapper(this); mMapper isn't very descriptive. Perhaps mInputFeedbackMapper or similar? https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java (right): https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:30: * 0.0f will be returned if no feedback should be shown. I'd either remove this line or use javadoc style for input and return values. As it is, I don't think this comment is helpful.
https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:41: mMapper = new InputFeedbackRadiusMapper(this); On 2016/08/31 22:14:12, joedow wrote: > mMapper isn't very descriptive. Perhaps mInputFeedbackMapper or similar? Done. https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java (right): https://codereview.chromium.org/2297073002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/InputFeedbackRadiusMapper.java:30: * 0.0f will be returned if no feedback should be shown. On 2016/08/31 22:14:12, joedow wrote: > I'd either remove this line or use javadoc style for input and return values. > As it is, I don't think this comment is helpful. Done. Use javadoc style and give more useful comment.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zijiehe@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2297073002/#ps80001 (title: "Reviewer's Feedback")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zijiehe@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2297073002/#ps100001 (title: "Merge ToT")
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.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Move feedback type to radius logic out of DesktopView The feedback-type-to-radius logic should be renderer specific so it doesn't quite make sense to define the conversion function inside the view. This CL creates a standalone class for handling feedback type to radius conversion. BUG=641123 ========== to ========== [Remoting Android] Move feedback type to radius logic out of DesktopView The feedback-type-to-radius logic should be renderer specific so it doesn't quite make sense to define the conversion function inside the view. This CL creates a standalone class for handling feedback type to radius conversion. BUG=641123 Committed: https://crrev.com/60961fad23f2bab7372211fd971af813df74650d Cr-Commit-Position: refs/heads/master@{#415810} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/60961fad23f2bab7372211fd971af813df74650d Cr-Commit-Position: refs/heads/master@{#415810} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
