Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2032883002:
[Chromoting] Minor changes to TapGestureDetector (Closed)
Description[Chromoting] Minor changes to TapGestureDetector
This change finals several fields in TapGestureDetector, and fixes potential
issue which impacts long press position, and causing it not to be updated.
BUG=615277
Committed: https://crrev.com/852fb89c23feb6a6d6cc24d1acf87fac0eeb8ceb
Cr-Commit-Position: refs/heads/master@{#397545}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Resolve review comments #Messages
Total messages: 14 (7 generated)
Description was changed from ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and cause it won't update. BUG=615277 ========== to ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and cause it won't update. BUG=615277 ==========
zijiehe@chromium.org changed reviewers: + lambroslambrou@chromium.org
lgtm In the description: "and cause it won't update" I think you mean "causing it not to be updated" ? https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java (right): https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:45: private final OnTapListener mListener; Are you adding 'final' simply because this implementation doesn't modify it? This isn't *logically* final. You could easily add a setListener() method without breaking anything. There is a small cost to adding 'final' (not just the extra typing and clutter). The problem is that 'final' can discourage other developers from removing it, which is why I prefer not to use it unnecessarily. But I don't feel strongly, there are some benefits as well, and I'm not aware of any style guidance. https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:60: private SparseArray<PointF> mInitialPositions = new SparseArray<PointF>(); This could be final as well? https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:66: private int mTouchSlopSquare; And this?
Description was changed from ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and cause it won't update. BUG=615277 ========== to ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and causing it not to be updated. BUG=615277 ==========
On 2016/06/02 18:11:31, Lambros wrote: > lgtm > > In the description: "and cause it won't update" > I think you mean > "causing it not to be updated" > ? > > https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... > File remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java > (right): > > https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... > remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:45: > private final OnTapListener mListener; > Are you adding 'final' simply because this implementation doesn't modify it? > > This isn't *logically* final. You could easily add a setListener() method > without breaking anything. > > There is a small cost to adding 'final' (not just the extra typing and clutter). > The problem is that 'final' can discourage other developers from removing it, > which is why I prefer not to use it unnecessarily. > > But I don't feel strongly, there are some benefits as well, and I'm not aware of > any style guidance. > > https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... > remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:60: > private SparseArray<PointF> mInitialPositions = new SparseArray<PointF>(); > This could be final as well? > > https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... > remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:66: > private int mTouchSlopSquare; > And this? Sorry about my poor English ... I am addressing your comments.
https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java (right): https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:45: private final OnTapListener mListener; On 2016/06/02 18:11:30, Lambros wrote: > Are you adding 'final' simply because this implementation doesn't modify it? > > This isn't *logically* final. You could easily add a setListener() method > without breaking anything. > > There is a small cost to adding 'final' (not just the extra typing and clutter). > The problem is that 'final' can discourage other developers from removing it, > which is why I prefer not to use it unnecessarily. > > But I don't feel strongly, there are some benefits as well, and I'm not aware of > any style guidance. > In my opinion -- it's similar as the 'final' of SelfRemovableParameterRunnable -- it would be better to 'final' it unless we need to make it not 'final'. For these fields, finalizing them can help the readers to understand their references won't be changed during the lifetime of this instance. https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:60: private SparseArray<PointF> mInitialPositions = new SparseArray<PointF>(); On 2016/06/02 18:11:30, Lambros wrote: > This could be final as well? Done. https://codereview.chromium.org/2032883002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:66: private int mTouchSlopSquare; On 2016/06/02 18:11:30, Lambros wrote: > And this? Done.
The CQ bit was checked by zijiehe@chromium.org
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/2032883002/#ps20001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2032883002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2032883002/20001
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and causing it not to be updated. BUG=615277 ========== to ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and causing it not to be updated. BUG=615277 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and causing it not to be updated. BUG=615277 ========== to ========== [Chromoting] Minor changes to TapGestureDetector This change finals several fields in TapGestureDetector, and fixes potential issue which impacts long press position, and causing it not to be updated. BUG=615277 Committed: https://crrev.com/852fb89c23feb6a6d6cc24d1acf87fac0eeb8ceb Cr-Commit-Position: refs/heads/master@{#397545} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/852fb89c23feb6a6d6cc24d1acf87fac0eeb8ceb Cr-Commit-Position: refs/heads/master@{#397545} |
