Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
481683003:
Support for Adaptive Handle Orientation (Closed)
|
Created:
6 years, 4 months ago by AviD Modified:
5 years, 2 months ago CC:
AKVT, chromium-reviews, darin-cc_chromium.org, jam, jdduke+watch_chromium.org, mohsen, raghu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport for Adaptive Handle Orientation
It would be easier in edge screen cases to move the selection
handles if it is not hidden. Adding support for Adaptive
Handle Orientation.
BUG=402824
Committed: https://crrev.com/348be68968d377e8a3cb55af747784936aa56185
Cr-Commit-Position: refs/heads/master@{#352319}
Patch Set 1 #Patch Set 2 : cleaning up #Patch Set 3 : some more cleaning up #
Total comments: 6
Patch Set 4 : Initial patchset #
Total comments: 2
Patch Set 5 : Separated viewport changed notification #
Total comments: 6
Patch Set 6 : using mirror_x and mirror_y in handles #
Total comments: 4
Patch Set 7 : #
Total comments: 8
Patch Set 8 : Review comments #Patch Set 9 : #
Total comments: 6
Patch Set 10 : Code refactoring #Patch Set 11 : Cleanup #
Total comments: 1
Patch Set 12 : #Patch Set 13 : cleanup #
Total comments: 49
Patch Set 14 : Rebase and refactoring #Patch Set 15 : Fixed nit added TODO for creation of new inverted handle. #Patch Set 16 : Fixed few inverting issues checked on Chrome Public apk #
Total comments: 6
Patch Set 17 : Review comments #
Total comments: 12
Patch Set 18 : Added drawable whitespace offset value setter #Patch Set 19 : unittests #
Total comments: 16
Patch Set 20 : Refactor + Unittests #Patch Set 21 : fixed nits #
Total comments: 14
Patch Set 22 : Fixed nits #
Total comments: 8
Patch Set 23 : Fixed nits. #
Total comments: 22
Patch Set 24 : Fixed as per jdduke's comments #Patch Set 25 : nits #
Total comments: 14
Patch Set 26 : changed as per jdduke comments #
Total comments: 4
Patch Set 27 : removed enabling flag #
Total comments: 8
Patch Set 28 : fixed nits #Patch Set 29 : Added corresponding methods in Aura #
Total comments: 8
Patch Set 30 : Changes in aura handles #
Total comments: 4
Patch Set 31 : Update handles for explicit handle visibility calls #
Total comments: 6
Patch Set 32 : incorporated changes as per review comments #Patch Set 33 : rebased #Patch Set 34 : rebase fix #Patch Set 35 : rebase fix #
Total comments: 8
Patch Set 36 : review comments #
Total comments: 2
Patch Set 37 : changed switch name #Patch Set 38 : rebased #Messages
Total messages: 114 (15 generated)
@jdduke: Could you please take a look? This is only a basic patch which adds support for inverted bitmap. I plan to add more unit tests going further. Could you also please guide for the best approach for enabling inverted handles? I was thinking compile/runtime flags for enabling and disabling.
I'm not sure this is the best approach. If we made the drawables viewport-aware, this might become a lot simpler (the drawable could then orient itself according to the collision of its drawable and the viewport area). https://codereview.chromium.org/481683003/diff/40001/content/browser/android/... File content/browser/android/popup_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/40001/content/browser/android/... content/browser/android/popup_touch_handle_drawable.cc:47: // synchronous (WebView) compositor case. WebView is probably the *most* useful case for inverting handles, so this is a little confusing. https://codereview.chromium.org/481683003/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_handle.h:25: TOUCH_HANDLE_LEFT_INVERTED, I don't think we should add these until we actually start using them; otherwise it's confusing. Also, INVERTED and FLIPPED is somewhat confusing terminology. I almost feel like the drawables could be responsible for "automatically" inverting/flipping themselves to stay visible, rather than having that be explicitly controlled, which could greatly increase complexity. https://codereview.chromium.org/481683003/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:84: Bitmap scaledBitmap = Bitmap.createScaledBitmap(source, source.getWidth(), Why do we need to invert the bitmap explicitly when we can otherwise just change the transform?
jdduke@ Thanks for your review comments. I was thinking of the below approach for having inverted handles: -> RenderWidgetHostViewAndroid::SelectionBoundsChanged() -- Here I check if for the position of the anchor_rect and focus_rect, and accordingly modify the anchor_orientation and focus_orientation. -> touch_selection_controller -- based on the orientation and the support given for positioning the handles based on the orientation, draw the handles. Could you please suggest a better approach? https://codereview.chromium.org/481683003/diff/40001/content/browser/android/... File content/browser/android/popup_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/40001/content/browser/android/... content/browser/android/popup_touch_handle_drawable.cc:47: // synchronous (WebView) compositor case. On 2014/08/20 15:15:01, jdduke wrote: > WebView is probably the *most* useful case for inverting handles, so this is a > little confusing. If the handles were to be drawn on top of Selected text(as the bug description), then it would be a round about approach to get the top of selection (start_rect and end_rect) on java(PopuptouchHandleDrawable) side. But if we anchor the handles to the bottom of selection(as per your suggestion), then it can be implemented without having to consider the selection height. I'll work on bringing this on WebView also. https://codereview.chromium.org/481683003/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:84: Bitmap scaledBitmap = Bitmap.createScaledBitmap(source, source.getWidth(), On 2014/08/20 15:15:01, jdduke wrote: > Why do we need to invert the bitmap explicitly when we can otherwise just change > the transform? Please correct if my understanding is wrong. Does it mean that we can check for the position and transform the drawable in composited_touch_handle_drawable itself?
So, my suggestion here would be to wait until we enable composited position updates (https://codereview.chromium.org/359033002/). That will give us tighter, precise information on how the positions relate to the current viewport. https://codereview.chromium.org/481683003/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:84: Bitmap scaledBitmap = Bitmap.createScaledBitmap(source, source.getWidth(), On 2014/08/21 16:15:20, AviD wrote: > On 2014/08/20 15:15:01, jdduke wrote: > > Why do we need to invert the bitmap explicitly when we can otherwise just > change > > the transform? > > Please correct if my understanding is wrong. Does it mean that we can check for > the position and transform the drawable in composited_touch_handle_drawable > itself? Right, we can use the same bitmap/drawable, but simply change its transform.
@jdduke: This is only a general idea of the inverted handles for text selection. I will refine it further based on your comments. Please take a look.
I may not be able to review this thoroughly until Wednesday. +mfomitchev, +mohsen for FYI. https://codereview.chromium.org/481683003/diff/60001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/60001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:72: const gfx::SizeF& root_layer_size, The size/offet should probably be in a separate method. Ideally we'd just have one call |OnViewportChanged(gfx::Rect())|.
@mfomitchev, @mohsen, @jdduke Could you PTAL? https://codereview.chromium.org/481683003/diff/60001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/60001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:72: const gfx::SizeF& root_layer_size, On 2015/03/16 22:01:45, jdduke wrote: > The size/offet should probably be in a separate method. Ideally we'd just have > one call |OnViewportChanged(gfx::Rect())|. Done.
https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:444: start_orientation_ == TouchHandleOrientation::LEFT_INVERTED || Why does the position change depending on the orientation? The position should be the "focus" of the selection handle, which should be invariant with respect to orientation, even if we're inverted, right? https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:494: if (start_handle_state == 2) { Flipped vs inverted is confusing. What if, instead, we had a mirror_x + mirror_y argument. We can pass that to the drawable along with the orientation, which should be all we need to position the handle (given an anchor position), right? Then we don't need all these enum combinations.
I think we should discuss this with Android ProdM. Inverting the handles sounds like a good idea, but if it's not consistent across the platform, it may create confusion which will make this a net negative change. I'll start a thread. Also, if we decide to proceed with this, I think it would be good to get some input from UX on the positioning of inverted handles. It looks like this patch positions them so that the tip is always aligned to the baseline? This obscures the text we are selecting, so it might be better to position the inverted handle so that the tip is aligned to the top of the selection highlight instead.
On 2015/03/19 20:01:39, mfomitchev wrote: > I think we should discuss this with Android ProdM. Inverting the handles sounds > like a good idea, but if it's not consistent across the platform, it may create > confusion which will make this a net negative change. I'll start a thread. > Also, if we decide to proceed with this, I think it would be good to get some > input from UX on the positioning of inverted handles. It looks like this patch > positions them so that the tip is always aligned to the baseline? This obscures > the text we are selecting, so it might be better to position the inverted handle > so that the tip is aligned to the top of the selection highlight instead. I don't entirely agree with that last statement, but yes we should ensure an aligned vision with the platform.
mfomitchev@chromium.org changed reviewers: + mfomitchev@chromium.org
After some discussions, the consensus is that we want this for Chrome on both Android and Aura, and we've settled on aligning handles to the top when they are inverted. I've added some details to crbug.com/402824. @AviD - can you please update the code for handles positioning?
https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:444: start_orientation_ == TouchHandleOrientation::LEFT_INVERTED || On 2015/03/19 18:56:39, jdduke wrote: > Why does the position change depending on the orientation? The position should > be the "focus" of the selection handle, which should be invariant with respect > to orientation, even if we're inverted, right? Nevermind, it sounds like if we mirror the handle about the x-axis (flip it vertically), we'll want to position it at the top of the text. https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:494: if (start_handle_state == 2) { On 2015/03/19 18:56:39, jdduke wrote: > Flipped vs inverted is confusing. > > What if, instead, we had a mirror_x + mirror_y argument. We can pass that to the > drawable along with the orientation, which should be all we need to position the > handle (given an anchor position), right? Then we don't need all these enum > combinations. I wonder if it's worth having the TouchHandle be responsible for mirroring itself?
On 2015/03/23 21:01:06, mfomitchev wrote: > After some discussions, the consensus is that we want this for Chrome on both > Android and Aura, and we've settled on aligning handles to the top when they are > inverted. I've added some details to crbug.com/402824. @AviD - can you please > update the code for handles positioning? Thanks mfomitchev@, jdduke@ I have attached a video in crbug.com/402824 with the behavior in Chrome Shell with the current patch. Will refine the patch as per jdduke's suggestion.
On 2015/03/24 11:08:22, AviD wrote: > On 2015/03/23 21:01:06, mfomitchev wrote: > > After some discussions, the consensus is that we want this for Chrome on both > > Android and Aura, and we've settled on aligning handles to the top when they > are > > inverted. I've added some details to crbug.com/402824. @AviD - can you please > > update the code for handles positioning? > > Thanks mfomitchev@, jdduke@ > > I have attached a video in crbug.com/402824 with the behavior in Chrome Shell > with the current patch. > Will refine the patch as per jdduke's suggestion. Sounds good! The video looks great, and I believe all the orientations we care about were properly handled.
Added patch with mirror_x and mirror_y params. https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:444: start_orientation_ == TouchHandleOrientation::LEFT_INVERTED || On 2015/03/23 21:05:31, jdduke wrote: > On 2015/03/19 18:56:39, jdduke wrote: > > Why does the position change depending on the orientation? The position should > > be the "focus" of the selection handle, which should be invariant with respect > > to orientation, even if we're inverted, right? > > Nevermind, it sounds like if we mirror the handle about the x-axis (flip it > vertically), we'll want to position it at the top of the text. Right, it is to position the handles above the text in case of inverted/x-axis mirrored condition. https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:494: if (start_handle_state == 2) { On 2015/03/23 21:05:31, jdduke wrote: > On 2015/03/19 18:56:39, jdduke wrote: > > Flipped vs inverted is confusing. > > > > What if, instead, we had a mirror_x + mirror_y argument. We can pass that to > the > > drawable along with the orientation, which should be all we need to position > the > > handle (given an anchor position), right? Then we don't need all these enum > > combinations. > > I wonder if it's worth having the TouchHandle be responsible for mirroring > itself? I will try this approach with the next patch.
On 2015/03/24 20:19:57, jdduke wrote: > On 2015/03/24 11:08:22, AviD wrote: > > On 2015/03/23 21:01:06, mfomitchev wrote: > > > After some discussions, the consensus is that we want this for Chrome on > both > > > Android and Aura, and we've settled on aligning handles to the top when they > > are > > > inverted. I've added some details to crbug.com/402824. @AviD - can you > please > > > update the code for handles positioning? > > > > Thanks mfomitchev@, jdduke@ > > > > I have attached a video in crbug.com/402824 with the behavior in Chrome Shell > > with the current patch. > > Will refine the patch as per jdduke's suggestion. > > Sounds good! The video looks great, and I believe all the orientations we care > about were properly handled. Thanks. Regarding deferring the orientation while dragging: With the mirror_x, mirror_y approach, i was thinking if we could send the selection height or both top and end positions, when we call TouchHandle::SetPosition(). This would give TouchHandle class a bit more flexibility in terms of positioning the handles especially at the end of drag, rather than having to shuttle between controller and TouchHandle for drag handling. With multiple orientation enums, deferring will be handled with the existing code itself.
https://codereview.chromium.org/481683003/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:159: // Flipped Inverted Case Flipped/inverted can mean the same thing so I'm not sure these comments add additional insights. https://codereview.chromium.org/481683003/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:208: mHotspotY = mirrorX ? mDrawable.getIntrinsicHeight() : 0; Hmm, I guess |mirrorX| can be confusing. It could mean either mirror the x-coordinates or mirror about the x-axis. I interpreted it as the former, and you the latter. So this should either be |mirrorHorizontal| (for mirroring x coordinates) or |mirrorAcrossXAxis| (for mirroring about the x axis). I somewhat prefer |mirrorHorizontal/Vertical|.
https://codereview.chromium.org/481683003/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:159: // Flipped Inverted Case On 2015/04/06 18:58:42, jdduke wrote: > Flipped/inverted can mean the same thing so I'm not sure these comments add > additional insights. Done. https://codereview.chromium.org/481683003/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:208: mHotspotY = mirrorX ? mDrawable.getIntrinsicHeight() : 0; On 2015/04/06 18:58:41, jdduke wrote: > Hmm, I guess |mirrorX| can be confusing. It could mean either mirror the > x-coordinates or mirror about the x-axis. I interpreted it as the former, and > you the latter. > > So this should either be |mirrorHorizontal| (for mirroring x coordinates) or > |mirrorAcrossXAxis| (for mirroring about the x axis). I somewhat prefer > |mirrorHorizontal/Vertical|. Changed to mirrorHorizontal
I haven't forgotten about this! Just working through a few release blocking issues, I'm hoping to get to this tomorrow or Monday.
https://codereview.chromium.org/481683003/diff/120001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/120001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:48: left_inverted_bitmap_ = SkBitmapOperations::Rotate( There's no need to create new assets for a transformed layer. We should just update the layer transforms directly. https://codereview.chromium.org/481683003/diff/120001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:158: const int bitmap_height = bitmap.height(); To set the layer transform, you can use an approach simililar to that found here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an... You simply set the transform origin, then scale the transform based on mirror_vertical and mirror_horizontal. https://codereview.chromium.org/481683003/diff/120001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:65: static Drawable invertDrawable(final Drawable drawable, final int angle) { Can we move this code in to the PopupTouchHandleDrawable class? It might be easier to understand if it lives near the code that consumes it. https://codereview.chromium.org/481683003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:71: canvas.rotate( Hmm, inverting is not the same as a 180 degree rotation. Shouldn't this be something like canvas.scale(1.f, -1.f, w/2, h/2)?
https://codereview.chromium.org/481683003/diff/120001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/120001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:48: left_inverted_bitmap_ = SkBitmapOperations::Rotate( On 2015/04/14 00:00:32, jdduke wrote: > There's no need to create new assets for a transformed layer. We should just > update the layer transforms directly. Done. https://codereview.chromium.org/481683003/diff/120001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:158: const int bitmap_height = bitmap.height(); On 2015/04/14 00:00:32, jdduke wrote: > To set the layer transform, you can use an approach simililar to that found > here: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an... > > You simply set the transform origin, then scale the transform based on > mirror_vertical and mirror_horizontal. Done. https://codereview.chromium.org/481683003/diff/120001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:65: static Drawable invertDrawable(final Drawable drawable, final int angle) { On 2015/04/14 00:00:32, jdduke wrote: > Can we move this code in to the PopupTouchHandleDrawable class? It might be > easier to understand if it lives near the code that consumes it. Done. https://codereview.chromium.org/481683003/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:71: canvas.rotate( On 2015/04/14 00:00:32, jdduke wrote: > Hmm, inverting is not the same as a 180 degree rotation. Shouldn't this be > something like canvas.scale(1.f, -1.f, w/2, h/2)? Done.
Getting closer! Eventually we'll want some tests, but let's leave them out until the implementation has been more or less finalized. https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:74: void SetPosition(const gfx::PointF& position); Would it be any simpler if, instead of |SetPosition()|, we had both a |SetFocus(top, bottom)|, and a |SetViewport(viewport_rect)|, and the handle itself would be responsible for picking the right logical position given the viewport rect? Then I think we'd have almost all of the viewport-related logic nicely contained here in TouchHandle, rather than partially here and partially in TouchSelectionController. Sorry for the back and forth here, but I think this is shaping up very nicely, and will be a great addition. Thanks! https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:92: gfx::RectF GetHandleBounds(); Hmm, where is this method used? Can we remove it? https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:95: bool is_inverted() const { return mirror_vertical_; } These getter names should reflect the member variable names.
I will add tests once the approach has been finalized. https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:74: void SetPosition(const gfx::PointF& position); On 2015/04/23 15:22:39, jdduke wrote: > Would it be any simpler if, instead of |SetPosition()|, we had both a > > |SetFocus(top, bottom)|, and a > |SetViewport(viewport_rect)|, It wouldn't be possible to get rid if SetPosition() entirely. I have renamed it to UpdatePosition() and used |SetFocus(top, bottom)| and |SetViewport(viewport_rect)| to set the values in TouchHandle. It would be helpful to have it this way for repositioning the handles on DRAG_END condition. > > and the handle itself would be responsible for picking the right logical > position given the viewport rect? Then I think we'd have almost all of the > viewport-related logic nicely contained here in TouchHandle, rather than > partially here and partially in TouchSelectionController. I have tried to contain most of the logic in TouchHandle in the new patch. > > Sorry for the back and forth here, but I think this is shaping up very nicely, > and will be a great addition. Thanks! All's well that ends well :) Thanks for your review comments. Helps me learn better about contribution to chromium development and keep the code clean. https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:92: gfx::RectF GetHandleBounds(); On 2015/04/23 15:22:39, jdduke wrote: > Hmm, where is this method used? Can we remove it? Done. https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:95: bool is_inverted() const { return mirror_vertical_; } On 2015/04/23 15:22:39, jdduke wrote: > These getter names should reflect the member variable names. Removed.
https://codereview.chromium.org/481683003/diff/200001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/200001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:141: focus_bottom_ = bottom; OK, so there are 3 inputs here that can impact the orientation layout: viewport, position and orientation. Any of those three can be updated at any time, so we need to "flush" the right orientation after each update. So we'll have something like public: SetFocus() { ... UpdateLayout(); } SetOrientation() { ... UpdateLayout(); } SetViewport(); { ... UpdateLayout(); } wherein each we early return if the bits aren't dirty (as we do now), then we have: private: UpdateLayout() { bool mirror_x = ...; bool mirror_y = ...; gfx::PointF position = ...; if (visible) drawable_->SetLayout(position, orientation_, mirror_x, mirror_y); } Would that work? I think we can replace the use of |position_| when draggign/animating with |focus_bottom_|, get rid of the position() getter on this class entirely, and use GetStart/GetEndPosition in TSC::OnHandleDragBegin. Then we no longer need a |position_| member variable at all, or |mirror| member variables, and we can just call |UpdateLayout()| after we regain visibility.
Any updates here? Do you want to keep going with this or do you need one of us to resume the work?
On 2015/04/30 19:41:51, jdduke wrote: > Any updates here? Do you want to keep going with this or do you need one of us > to resume the work? I am working on this. There are a few side effects which I am trying to fix with the new approach. Will upload the new patch by tomorrow.
jdduke@ is this approach Ok? I will upload the modified test cases if you're Ok with this change.
A few nits, but at a high level this looks pretty good! Let's wait until mfomitchev@ is back and can review before adding tests. https://codereview.chromium.org/481683003/diff/240001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/240001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:120: layer_->SetBitmap(bitmap); Hmm, it looks like UIResourceLayer doesn't cache the bitmap reference, so I think we should only perform this update when the orientation changes (might have to cache the orientation). https://codereview.chromium.org/481683003/diff/240001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:133: switch (orientation_) { Hmm, to save on code can you do: focal_offset_x = 0; switch (...) { ... focal_offset_x = ... } focal_offset_from_origin_ = gfx::Vector2df(focal_offset_x, focal_offset_y); https://codereview.chromium.org/481683003/diff/240001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:180: layer_->SetPosition(focal_position_ - focal_offset_from_origin_); Since this is only called in one place now, I'd be OK moving the logic up to where it is called and removing this function. https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:214: } I'd be OK early returning if mirrorHorizontal and mirrorVertical are both false. https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { It's a shame to create a new Drawable each time, not sure if there's any way around this? https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:127: UpdateLayout(); Nice, I think this is a bit easier to follow. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (left): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:350: start_selection_handle_->SetPosition(GetStartPosition()); Hmm, I think you need to rebase. We now only conditionally call ActivateSelection(), so I think the focus/viewport updates should come here. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:343: insertion_handle_->SetFocus(start_.edge_top(), start_.edge_bottom()); Hmm, shouldn't the visibility update come first?
>A few nits, but at a high level this looks pretty good! Let's wait until mfomitchev@ is back and can review before adding tests. I will review this tomorrow or Wednesday.
https://codereview.chromium.org/481683003/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/481683003/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1457: gfx::SizeF view_size(frame_metadata.scrollable_viewport_size); Nit: Rename this to viewport_size for consistency? https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:244: gfx::RectF handle_bounds = drawable_->GetVisibleBounds(); move this inside the if https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:257: focus_bottom_.x() - handle_bounds.width() < viewport_rect_.x()) { This assumes that handle is drawn without any offset to the focus position, but in reality there's focal_offset_from_origin_ equal to 1/4 of the handle width, so we will be a bit too eager to flip horizontally. By the way, I don't understand why do we have that focal_offset_from_origin_? Is there exactly 1/4 whitespace padding on handle images? jdduke, perhaps you know? https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:261: if (orientation_ == TouchHandleOrientation::RIGHT && else if https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:296: UpdateLayout(); // Handle layout may is deferred while the handle is dragged. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:75: void SetFocus(const gfx::PointF& top, const gfx::PointF& bottom); We always call both SetFocus and SetViewportRect, and we end up doing the layout twice. Can we combine these methods into one like we did for CompositedTouchHandleDrawable? https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:94: const gfx::PointF& position() const { return focus_bottom_; } rename to focus_bottom()? https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.h (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:52: // To be called when the viewport size has been changed. This is used for size -> rect https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:156: bool viewport_size_changed_; size -> rect
https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:214: } I think we should, since we can return the drawable as is instead of creating a new one in this case. https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/11 15:40:15, jdduke wrote: > It's a shame to create a new Drawable each time, not sure if there's any way > around this? Can we cache the current one? Since ActivateSelection is called conditionally now I guess it doesn't matter much in practice, but would be nice to have this layer of protection so that we don't end up creating garbage every frame in the future.
https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/13 22:33:01, mfomitchev wrote: > On 2015/05/11 15:40:15, jdduke wrote: > > It's a shame to create a new Drawable each time, not sure if there's any way > > around this? > > Can we cache the current one? Since ActivateSelection is called conditionally > now I guess it doesn't matter much in practice, but would be nice to have this > layer of protection so that we don't end up creating garbage every frame in the > future. It won't be per-frame, it would only be when the mirroring/orientation change. Still too frequent for my liking. Instead, we can always reference mMirrorHorizontal/mMirrorVertical, so then we only create a new drawable if the orientation changes, which should be (relatively) infrequent. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:257: focus_bottom_.x() - handle_bounds.width() < viewport_rect_.x()) { On 2015/05/13 22:19:14, mfomitchev wrote: > This assumes that handle is drawn without any offset to the focus position, but > in reality there's focal_offset_from_origin_ equal to 1/4 of the handle width, > so we will be a bit too eager to flip horizontally. > > By the way, I don't understand why do we have that focal_offset_from_origin_? Is > there exactly 1/4 whitespace padding on handle images? jdduke, perhaps you know? I believe so, yes. The |handle_bounds| should give us the bounds in the right (viewport) coordinates, so as you say we should be using those coordinates instead of just the width/height. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:257: focus_bottom_.x() - handle_bounds.width() < viewport_rect_.x()) { On 2015/05/13 22:19:14, mfomitchev wrote: > This assumes that handle is drawn without any offset to the focus position, but > in reality there's focal_offset_from_origin_ equal to 1/4 of the handle width, > so we will be a bit too eager to flip horizontally. > Good catch, |handle_bounds| should already be in viewport coordinates, but it's using coordinates from the previous frame, so we have a bit of a chicken and egg here. Also, I think the |GetVisibleBounds()| call may now be incorrect for mirrored handles... hmm... > By the way, I don't understand why do we have that focal_offset_from_origin_? Is > there exactly 1/4 whitespace padding on handle images? jdduke, perhaps you know? I believe so, yes. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:75: void SetFocus(const gfx::PointF& top, const gfx::PointF& bottom); On 2015/05/13 22:19:14, mfomitchev wrote: > We always call both SetFocus and SetViewportRect, and we end up doing the layout > twice. Can we combine these methods into one like we did for > CompositedTouchHandleDrawable? Hmm, but we would only do the layout twice if the viewport size changes (hopefully infrequent?). Don't have a problem merging the two calls, not sure what the best name would be.
https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/14 15:20:10, jdduke wrote: > On 2015/05/13 22:33:01, mfomitchev wrote: > > On 2015/05/11 15:40:15, jdduke wrote: > > > It's a shame to create a new Drawable each time, not sure if there's any way > > > around this? > > > > Can we cache the current one? Since ActivateSelection is called conditionally > > now I guess it doesn't matter much in practice, but would be nice to have this > > layer of protection so that we don't end up creating garbage every frame in > the > > future. > > It won't be per-frame, it would only be when the mirroring/orientation change. > Still too frequent for my liking. > > Instead, we can always reference mMirrorHorizontal/mMirrorVertical, so then we > only create a new drawable if the orientation changes, which should be > (relatively) infrequent. It's called every time PopupTouchHandleDrawable::SetLayout is called, which is called every time TouchHandle::UpdateLayout() is called, which is called every time TouchHandle::SetFocus is called and top or bottom changes, which I understand can happen every frame if the user is scrolling while the handles are shown. Am I missing something? https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:257: focus_bottom_.x() - handle_bounds.width() < viewport_rect_.x()) { On 2015/05/14 15:20:10, jdduke wrote: > On 2015/05/13 22:19:14, mfomitchev wrote: > > This assumes that handle is drawn without any offset to the focus position, > but > > in reality there's focal_offset_from_origin_ equal to 1/4 of the handle width, > > so we will be a bit too eager to flip horizontally. > > > > Good catch, |handle_bounds| should already be in viewport coordinates, but it's > using coordinates from the previous frame, so we have a bit of a chicken and egg > here. Yeah, and the coordinates from the previous frame may already reflect values for a mirrored handle, so we can't use them. Perhaps we can expose the value of focal_offset_from_origin for a non-mirrored handle, and then we can offset the width by that amount? > Also, I think the |GetVisibleBounds()| call may now be incorrect for > mirrored handles... hmm... It's correct for the previous frame, but not necessarily after we update the layout, right?
https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/14 15:52:39, mfomitchev wrote: > On 2015/05/14 15:20:10, jdduke wrote: > > On 2015/05/13 22:33:01, mfomitchev wrote: > > > On 2015/05/11 15:40:15, jdduke wrote: > > > > It's a shame to create a new Drawable each time, not sure if there's any > way > > > > around this? > > > > > > Can we cache the current one? Since ActivateSelection is called > conditionally > > > now I guess it doesn't matter much in practice, but would be nice to have > this > > > layer of protection so that we don't end up creating garbage every frame in > > the > > > future. > > > > It won't be per-frame, it would only be when the mirroring/orientation change. > > Still too frequent for my liking. > > > > Instead, we can always reference mMirrorHorizontal/mMirrorVertical, so then we > > only create a new drawable if the orientation changes, which should be > > (relatively) infrequent. > > It's called every time PopupTouchHandleDrawable::SetLayout is called, which is > called every time TouchHandle::UpdateLayout() is called, which is called every > time TouchHandle::SetFocus is called and top or bottom changes, which I > understand can happen every frame if the user is scrolling while the handles are > shown. Am I missing something? Should have been more clear, I meant in the event that we actually do the right thing and cache the values.
https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/14 15:55:57, jdduke wrote: > On 2015/05/14 15:52:39, mfomitchev wrote: > > On 2015/05/14 15:20:10, jdduke wrote: > > > On 2015/05/13 22:33:01, mfomitchev wrote: > > > > On 2015/05/11 15:40:15, jdduke wrote: > > > > > It's a shame to create a new Drawable each time, not sure if there's any > > way > > > > > around this? > > > > > > > > Can we cache the current one? Since ActivateSelection is called > > conditionally > > > > now I guess it doesn't matter much in practice, but would be nice to have > > this > > > > layer of protection so that we don't end up creating garbage every frame > in > > > the > > > > future. > > > > > > It won't be per-frame, it would only be when the mirroring/orientation > change. > > > Still too frequent for my liking. > > > > > > Instead, we can always reference mMirrorHorizontal/mMirrorVertical, so then > we > > > only create a new drawable if the orientation changes, which should be > > > (relatively) infrequent. > > > > It's called every time PopupTouchHandleDrawable::SetLayout is called, which is > > called every time TouchHandle::UpdateLayout() is called, which is called every > > time TouchHandle::SetFocus is called and top or bottom changes, which I > > understand can happen every frame if the user is scrolling while the handles > are > > shown. Am I missing something? > > Should have been more clear, I meant in the event that we actually do the right > thing and cache the values. Acknowledged.
I will upload the new rebased patch tomorrow https://codereview.chromium.org/481683003/diff/240001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/240001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:120: layer_->SetBitmap(bitmap); On 2015/05/11 15:40:15, jdduke wrote: > Hmm, it looks like UIResourceLayer doesn't cache the bitmap reference, so I > think we should only perform this update when the orientation changes (might > have to cache the orientation). Done. We save orientation_ here, I have used that to check if orientation is changed. https://codereview.chromium.org/481683003/diff/240001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:133: switch (orientation_) { On 2015/05/11 15:40:15, jdduke wrote: > Hmm, to save on code can you do: > > focal_offset_x = 0; > switch (...) { > ... focal_offset_x = ... > } > focal_offset_from_origin_ = gfx::Vector2df(focal_offset_x, focal_offset_y); Done. https://codereview.chromium.org/481683003/diff/240001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:180: layer_->SetPosition(focal_position_ - focal_offset_from_origin_); On 2015/05/11 15:40:15, jdduke wrote: > Since this is only called in one place now, I'd be OK moving the logic up to > where it is called and removing this function. It is currently being called from SetEnabled() too. https://codereview.chromium.org/481683003/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/481683003/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1457: gfx::SizeF view_size(frame_metadata.scrollable_viewport_size); On 2015/05/13 22:19:14, mfomitchev wrote: > Nit: Rename this to viewport_size for consistency? Done. https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:214: } On 2015/05/13 22:33:00, mfomitchev wrote: > I think we should, since we can return the drawable as is instead of creating a > new one in this case. Done. https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/14 15:59:40, mfomitchev wrote: > On 2015/05/14 15:55:57, jdduke wrote: > > On 2015/05/14 15:52:39, mfomitchev wrote: > > > On 2015/05/14 15:20:10, jdduke wrote: > > > > On 2015/05/13 22:33:01, mfomitchev wrote: > > > > > On 2015/05/11 15:40:15, jdduke wrote: > > > > > > It's a shame to create a new Drawable each time, not sure if there's > any > > > way > > > > > > around this? > > > > > > > > > > Can we cache the current one? Since ActivateSelection is called > > > conditionally > > > > > now I guess it doesn't matter much in practice, but would be nice to > have > > > this > > > > > layer of protection so that we don't end up creating garbage every frame > > in > > > > the > > > > > future. > > > > > > > > It won't be per-frame, it would only be when the mirroring/orientation > > change. > > > > Still too frequent for my liking. > > > > > > > > Instead, we can always reference mMirrorHorizontal/mMirrorVertical, so > then > > we > > > > only create a new drawable if the orientation changes, which should be > > > > (relatively) infrequent. > > > > > > It's called every time PopupTouchHandleDrawable::SetLayout is called, which > is > > > called every time TouchHandle::UpdateLayout() is called, which is called > every > > > time TouchHandle::SetFocus is called and top or bottom changes, which I > > > understand can happen every frame if the user is scrolling while the handles > > are > > > shown. Am I missing something? > > > > Should have been more clear, I meant in the event that we actually do the > right > > thing and cache the values. > > Acknowledged. Would it be possible to fetch the mirrored handles directly from Android framework, similar to left, right and center handles? (android.R.attr.textSelectHandleRightInverted/android.R.attr.textSelectHandleLeftInverted) For now, I have cached the values and will create a new LayerDrawable only if orientation changes. Also added TODO for a better approach(if any). https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:127: UpdateLayout(); On 2015/05/11 15:40:15, jdduke wrote: > Nice, I think this is a bit easier to follow. Acknowledged. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:244: gfx::RectF handle_bounds = drawable_->GetVisibleBounds(); On 2015/05/13 22:19:14, mfomitchev wrote: > move this inside the if Done. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:257: focus_bottom_.x() - handle_bounds.width() < viewport_rect_.x()) { On 2015/05/14 15:52:39, mfomitchev wrote: > On 2015/05/14 15:20:10, jdduke wrote: > > On 2015/05/13 22:19:14, mfomitchev wrote: > > > This assumes that handle is drawn without any offset to the focus position, > > but > > > in reality there's focal_offset_from_origin_ equal to 1/4 of the handle > width, > > > so we will be a bit too eager to flip horizontally. > > > > > > > Good catch, |handle_bounds| should already be in viewport coordinates, but > it's > > using coordinates from the previous frame, so we have a bit of a chicken and > egg > > here. > > Yeah, and the coordinates from the previous frame may already reflect values for > a mirrored handle, so we can't use them. Perhaps we can expose the value of > focal_offset_from_origin for a non-mirrored handle, and then we can offset the > width by that amount? > > > Also, I think the |GetVisibleBounds()| call may now be incorrect for > > mirrored handles... hmm... > > It's correct for the previous frame, but not necessarily after we update the > layout, right? The focal_offset_from_origin is always 1/4th the width. But the handles sometimes have an extra transparent region after the handle image. For Samsung platform handles, the width is 1/4th the width of the image. eg: Left handle drawable is like: ______________ | /| | | /.| | | /..| | | /...| | | /....| | |____\____|___| <1/4> <1/2> <1/4> @jdduke: is this same for all handles? if it is so, then would it be okay if we check (handle_width*0.5) for checking mirror on the y axis condition? (or check for (handle_width*0.75) otherwise) https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:261: if (orientation_ == TouchHandleOrientation::RIGHT && On 2015/05/13 22:19:14, mfomitchev wrote: > else if Done. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:296: UpdateLayout(); On 2015/05/13 22:19:14, mfomitchev wrote: > // Handle layout may is deferred while the handle is dragged. Done. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:75: void SetFocus(const gfx::PointF& top, const gfx::PointF& bottom); On 2015/05/14 15:20:10, jdduke wrote: > On 2015/05/13 22:19:14, mfomitchev wrote: > > We always call both SetFocus and SetViewportRect, and we end up doing the > layout > > twice. Can we combine these methods into one like we did for > > CompositedTouchHandleDrawable? > > Hmm, but we would only do the layout twice if the viewport size changes > (hopefully infrequent?). Don't have a problem merging the two calls, not sure > what the best name would be. SetSelectionParams() ? https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:94: const gfx::PointF& position() const { return focus_bottom_; } On 2015/05/13 22:19:14, mfomitchev wrote: > rename to focus_bottom()? Done. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (left): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:350: start_selection_handle_->SetPosition(GetStartPosition()); On 2015/05/11 15:40:16, jdduke wrote: > Hmm, I think you need to rebase. We now only conditionally call > ActivateSelection(), so I think the focus/viewport updates should come here. I will rebase in the next patch. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:343: insertion_handle_->SetFocus(start_.edge_top(), start_.edge_bottom()); On 2015/05/11 15:40:16, jdduke wrote: > Hmm, shouldn't the visibility update come first? It wouldn't matter much here, as we call updateLayout() on SetVisible() also, where it would update the handles only once. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.h (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:52: // To be called when the viewport size has been changed. This is used for On 2015/05/13 22:19:14, mfomitchev wrote: > size -> rect Done. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:156: bool viewport_size_changed_; On 2015/05/13 22:19:14, mfomitchev wrote: > size -> rect Done.
Thanks! Once you have a chance to upload the latest patchset I'll try to play with this locally. https://codereview.chromium.org/481683003/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/19 16:26:27, AviD wrote: > Would it be possible to fetch the mirrored handles directly from Android > framework, similar to left, right and center handles? > (android.R.attr.textSelectHandleRightInverted/android.R.attr.textSelectHandleLeftInverted) > Ideally, but do they expose such values? Worst case we restrict the inversion/flipping functionality to L+, where the handles don't have shadows and are "flat" by design (so inverted/flipped doesn't actually change the style). > For now, I have cached the values and will create a new LayerDrawable only if > orientation changes. Also added TODO for a better approach(if any). Thanks. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:257: focus_bottom_.x() - handle_bounds.width() < viewport_rect_.x()) { On 2015/05/19 16:26:27, AviD wrote: > On 2015/05/14 15:52:39, mfomitchev wrote: > > On 2015/05/14 15:20:10, jdduke wrote: > > > On 2015/05/13 22:19:14, mfomitchev wrote: > > > > This assumes that handle is drawn without any offset to the focus > position, > > > but > > > > in reality there's focal_offset_from_origin_ equal to 1/4 of the handle > > width, > > > > so we will be a bit too eager to flip horizontally. > > > > > > > > > > Good catch, |handle_bounds| should already be in viewport coordinates, but > > it's > > > using coordinates from the previous frame, so we have a bit of a chicken and > > egg > > > here. > > > > Yeah, and the coordinates from the previous frame may already reflect values > for > > a mirrored handle, so we can't use them. Perhaps we can expose the value of > > focal_offset_from_origin for a non-mirrored handle, and then we can offset the > > width by that amount? > > > > > Also, I think the |GetVisibleBounds()| call may now be incorrect for > > > mirrored handles... hmm... > > > > It's correct for the previous frame, but not necessarily after we update the > > layout, right? > > The focal_offset_from_origin is always 1/4th the width. But the handles > sometimes have an extra transparent region after the handle image. For Samsung > platform handles, the width is 1/4th the width of the image. > > eg: Left handle drawable is like: > > ______________ > | /| | > | /.| | > | /..| | > | /...| | > | /....| | > |____\____|___| > > <1/4> <1/2> <1/4> > > > @jdduke: is this same for all handles? if it is so, then would it be okay if we > check (handle_width*0.5) for checking mirror on the y axis condition? (or check > for (handle_width*0.75) otherwise) We've made this assumption until now and it's worked. I think Android would have a hard time changing this fact, but we probably cannot assume this for other platforms. Worst case, we're slightly over aggressive in flipping the handles, but I don't necessarily think that's a bad thing? I'll have to try it out and see how it feels.
@jdduke, @mfomitchev : PTAL Created an inner private class for InvertedDrawable in PopupTouchHandleDrawable, so that we dont create a new object unless the orientation changes. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:257: focus_bottom_.x() - handle_bounds.width() < viewport_rect_.x()) { I have changed it to (handle_bounds.width() * 0.75f) to consider the focal offset only. The extra transparent padding would result in flipping the handles slightly earlier. On 2015/05/19 18:59:26, jdduke wrote: > On 2015/05/19 16:26:27, AviD wrote: > > On 2015/05/14 15:52:39, mfomitchev wrote: > > > On 2015/05/14 15:20:10, jdduke wrote: > > > > On 2015/05/13 22:19:14, mfomitchev wrote: > > > > > This assumes that handle is drawn without any offset to the focus > > position, > > > > but > > > > > in reality there's focal_offset_from_origin_ equal to 1/4 of the handle > > > width, > > > > > so we will be a bit too eager to flip horizontally. > > > > > > > > > > > > > Good catch, |handle_bounds| should already be in viewport coordinates, but > > > it's > > > > using coordinates from the previous frame, so we have a bit of a chicken > and > > > egg > > > > here. > > > > > > Yeah, and the coordinates from the previous frame may already reflect values > > for > > > a mirrored handle, so we can't use them. Perhaps we can expose the value of > > > focal_offset_from_origin for a non-mirrored handle, and then we can offset > the > > > width by that amount? > > > > > > > Also, I think the |GetVisibleBounds()| call may now be incorrect for > > > > mirrored handles... hmm... > > > > > > It's correct for the previous frame, but not necessarily after we update the > > > layout, right? > > > > The focal_offset_from_origin is always 1/4th the width. But the handles > > sometimes have an extra transparent region after the handle image. For Samsung > > platform handles, the width is 1/4th the width of the image. > > > > eg: Left handle drawable is like: > > > > ______________ > > | /| | > > | /.| | > > | /..| | > > | /...| | > > | /....| | > > |____\____|___| > > > > <1/4> <1/2> <1/4> > > > > > > @jdduke: is this same for all handles? if it is so, then would it be okay if > we > > check (handle_width*0.5) for checking mirror on the y axis condition? (or > check > > for (handle_width*0.75) otherwise) > > We've made this assumption until now and it's worked. I think Android would have > a hard time changing this fact, but we probably cannot assume this for other > platforms. > > Worst case, we're slightly over aggressive in flipping the handles, but I don't > necessarily think that's a bad thing? I'll have to try it out and see how it > feels. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:75: void SetFocus(const gfx::PointF& top, const gfx::PointF& bottom); On 2015/05/14 15:20:10, jdduke wrote: > On 2015/05/13 22:19:14, mfomitchev wrote: > > We always call both SetFocus and SetViewportRect, and we end up doing the > layout > > twice. Can we combine these methods into one like we did for > > CompositedTouchHandleDrawable? > > Hmm, but we would only do the layout twice if the viewport size changes > (hopefully infrequent?). Don't have a problem merging the two calls, not sure > what the best name would be. I have changed this to update the viewport rect only if it changes and active selection/insertion is present, instead of updating for every selectionChanged() event. https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (left): https://codereview.chromium.org/481683003/diff/240001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:350: start_selection_handle_->SetPosition(GetStartPosition()); On 2015/05/11 15:40:16, jdduke wrote: > Hmm, I think you need to rebase. We now only conditionally call > ActivateSelection(), so I think the focus/viewport updates should come here. Now that we have conditional calling of ActivateSelection() (which calls setOrientation()), the defered_orientation logic in TouchHandles::setOrientation would be obsolete? We would need the deferring logic to work so that the handles are updated properly on drag end. So, I have moved setOrientation here, along with setFocus() so that the handles orient properly on dragEnd.
jdduke@ PTAL?
https://codereview.chromium.org/481683003/diff/300001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/300001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:116: const SkBitmap& bitmap = g_selection_resources.Get().GetBitmap(orientation); Can we move the bitmap/bitmap_height/bitmap_width into the (orientation_changed) branch? Then below we can just do: gfx::Size bounds = layer_->bounds(); and use that where we otherwise used the bitmap. A minor detail, but it avoids fetching the bitmap every update. https://codereview.chromium.org/481683003/diff/300001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/300001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:331: mDrawable.setBounds(0, 0, getRight() - getLeft(), getBottom() - getTop()); Silly question, but I wonder if this works. What if you insert here: final boolean needsMirror = mMirrorHorizontal || mMirrorVertical; if (needsMirror) { c.save(); float scaleX = mMirrorHorizontal ? -1.f : 1.f; float scaleY = mMirrorVertical ? -1.f : 1.f; c.scale(scaleX, scaleY, getWidth() / 2.0f, getHeight() / 2.0f); } mDrawable.draw(c); if (needsMirror) c.restore(); Does that eliminate the need to create new drawables? https://codereview.chromium.org/481683003/diff/300001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/300001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:262: viewport_rect_.bottom()) { There's one scenario I wonder if we should handle differently. For WebView, the size of the viewport may be quite small, the size of the actual line of text. In that case, we would always invert the handles, but I wonder if we should avoid that? So we might change the logic to something like: float bottom_y_unmirrored = focus_bottom_.y() + handle_bounds.height() + viewport_rect_.y(); float top_y_mirrored = focus_top_.y() - handle_bounds.height() + viewport_rect_.y(); if (bottom_y_mirrored > viewport_rect_.bottom() && top_y_mirrored > viewport_rect_.top()) { mirror_vertical = true; } mirror_vertical = true; }
jdduke@ PTAL https://codereview.chromium.org/481683003/diff/300001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/300001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:116: const SkBitmap& bitmap = g_selection_resources.Get().GetBitmap(orientation); On 2015/06/03 18:59:00, jdduke wrote: > Can we move the bitmap/bitmap_height/bitmap_width into the (orientation_changed) > branch? > > Then below we can just do: > > gfx::Size bounds = layer_->bounds(); > > and use that where we otherwise used the bitmap. A minor detail, but it avoids > fetching the bitmap every update. Done. https://codereview.chromium.org/481683003/diff/300001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/300001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:331: mDrawable.setBounds(0, 0, getRight() - getLeft(), getBottom() - getTop()); On 2015/06/03 18:59:00, jdduke wrote: > Silly question, but I wonder if this works. What if you insert here: > > final boolean needsMirror = mMirrorHorizontal || mMirrorVertical; > if (needsMirror) { > c.save(); > float scaleX = mMirrorHorizontal ? -1.f : 1.f; > float scaleY = mMirrorVertical ? -1.f : 1.f; > c.scale(scaleX, scaleY, getWidth() / 2.0f, getHeight() / 2.0f); > } > mDrawable.draw(c); > if (needsMirror) c.restore(); > > Does that eliminate the need to create new drawables? This works. The mirror values have to be cached. I've retained the getHandleDrawable() call so that we can call it only when orientation is changed. https://codereview.chromium.org/481683003/diff/300001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/300001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:262: viewport_rect_.bottom()) { On 2015/06/03 18:59:00, jdduke(OOO_until_June_8) wrote: > There's one scenario I wonder if we should handle differently. For WebView, the > size of the viewport may be quite small, the size of the actual line of text. In > that case, we would always invert the handles, but I wonder if we should avoid > that? So we might change the logic to something like: > > float bottom_y_unmirrored = focus_bottom_.y() + handle_bounds.height() + > viewport_rect_.y(); > float top_y_mirrored = focus_top_.y() - handle_bounds.height() + > viewport_rect_.y(); > if (bottom_y_mirrored > viewport_rect_.bottom() && top_y_mirrored > > viewport_rect_.top()) { > mirror_vertical = true; > } > mirror_vertical = true; > } Done.
Looking pretty good, I think we're in a state where we can add some tests. Probably one for the viewport change to TouchSelectionController, and a couple for the TouchHandle changes? Thanks for being patient here! https://codereview.chromium.org/481683003/diff/320001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/320001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:150: boolean mirrorChanged) { Looks like we no longer need the "mirrorChanged" argument. https://codereview.chromium.org/481683003/diff/320001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:216: return HandleViewResources.getCenterHandleDrawable(mContext); Can you add "assert false;" here before returning? https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:133: if ((viewport_rect_ == viewport_rect)) Nit: No need for double parentheses.
https://codereview.chromium.org/481683003/diff/320001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/320001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:167: } nit: no need for brackets https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:269: mirror_vertical = true; Don't we also have vertical padding to account for here? https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:273: focus_bottom_.x() - (handle_bounds.width() * 0.75f) < This code will be used by Aura (desktop Chrome UI) as well once the unified text selection lands, and in Aura we don't have 25% padding in handle bitmaps. Instead of hard coding .75 here, can we get this value through an extensible interface? Perhaps something like // Returns the width of the whitespace padding on the handle image as a percentage of the total handle width. float TouchHandleDrawable::GetPaddingPercentage() I don't like the name, but can't think of anything better now. If we need to know the vertical p[adding as well, can make this return gfx::Size.
Added unittests. PTAL https://codereview.chromium.org/481683003/diff/320001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/320001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:150: boolean mirrorChanged) { On 2015/06/08 15:24:46, jdduke wrote: > Looks like we no longer need the "mirrorChanged" argument. This was being used in composited_touch_handle_drawable to do setTransform() only when mirror_changed is true. I have removed it for now, so the setTransform() would be done for every updateLayout() call. https://codereview.chromium.org/481683003/diff/320001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:167: } On 2015/06/08 18:06:09, mfomitchev wrote: > nit: no need for brackets Done. https://codereview.chromium.org/481683003/diff/320001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:216: return HandleViewResources.getCenterHandleDrawable(mContext); On 2015/06/08 15:24:46, jdduke wrote: > Can you add "assert false;" here before returning? Done. https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:133: if ((viewport_rect_ == viewport_rect)) On 2015/06/08 15:24:46, jdduke wrote: > Nit: No need for double parentheses. Done. https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:269: mirror_vertical = true; On 2015/06/08 18:06:09, mfomitchev wrote: > Don't we also have vertical padding to account for here? Added a common API to get both vertical and horizontal padding. https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:273: focus_bottom_.x() - (handle_bounds.width() * 0.75f) < On 2015/06/08 18:06:09, mfomitchev wrote: > This code will be used by Aura (desktop Chrome UI) as well once the unified text > selection lands, and in Aura we don't have 25% padding in handle bitmaps. > Instead of hard coding .75 here, can we get this value through an extensible > interface? Perhaps something like > > // Returns the width of the whitespace padding on the handle image as a > percentage of the total handle width. > float TouchHandleDrawable::GetPaddingPercentage() > > I don't like the name, but can't think of anything better now. If we need to > know the vertical p[adding as well, can make this return gfx::Size. I have added GetDrawablePadding() returning gfx::SizeF for the same. The values need to be set from a common location for Aura(if required) and Android. For Android, I have set the values from ContentViewCore. For Aura, since it doesn't have any whitespace/transparent padding, it wouldn't be required to set the values.
Haven't forgotten about this, but I won't be able to get to reviewing it until tomorrow or Friday, sorry!
https://codereview.chromium.org/481683003/diff/360001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:155: mirror_horizontal ? layer_width * 0.25f : layer_width * 0.75f; .25/.75 shouldn't be hard coded, but should be based on drawable_padding_ https://codereview.chromium.org/481683003/diff/360001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.h (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android... content/browser/android/composited_touch_handle_drawable.h:31: gfx::SizeF GetDrawablePadding() const override; Hmm.. These methods are a little hard to understand.. Perhaps instead of GetDrawablePadding() we could have GetBounds()? Then the padding could be calculated as the difference between the bounds and the visual bounds. I think this would make the semantics of these methods easier to grok. This would also work if the padding is not the same on top/bottom (unlike the current API). Also, I think it is not great that we are now essentially doing the layout math for the handle in two different classes: we do it in CompositedTouchHandleDrawable::SetLayout and in TouchHandle::UpdateLayout. One way to overcome this would be to replace TouchHandleDrawable::SetFocus with TouchHandleDrawable::SetOrigin, which would just specify the position where the handle would need to be drawn. Then we could remove all the layout/positioning math from TouchHandleDrawable, since it wouldn't need to calculate focal_offset_from_origin_. The responsibility breakdown would be cleaner: TouchHandleDrawable would tell TouchHandle about its geometry via GetBounds() and GetVisibleBounds, and TouchHandle would do all the positioning calculationsbased on this. What do you think? jdduke, does this sound reasonable to you? https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: HandleViewResources.setHandlePadding(0.0f, 0.25f); Is the vertical padding zero? I thought we may have some padding on the bottom of the handle images, no? https://codereview.chromium.org/481683003/diff/360001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_unittest.cc (right): https://codereview.chromium.org/481683003/diff/360001/ui/touch_selection/touc... ui/touch_selection/touch_handle_unittest.cc:60: gfx::SizeF GetDrawablePadding() const override { return gfx::SizeF(); } Once we have the implementation nailed down, we should probably add some unit tests for layout correctness with non-zero padding.
https://codereview.chromium.org/481683003/diff/360001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.h (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android... content/browser/android/composited_touch_handle_drawable.h:31: gfx::SizeF GetDrawablePadding() const override; On 2015/06/18 16:16:31, mfomitchev wrote: > Hmm.. These methods are a little hard to understand.. Perhaps instead of > GetDrawablePadding() we could have GetBounds()? Then the padding could be > calculated as the difference between the bounds and the visual bounds. I think > this would make the semantics of these methods easier to grok. This would also > work if the padding is not the same on top/bottom (unlike e current API). Considering that GetBounds() would return the bounds without the padding (i.e, (visible_bounds.width()*0.75, visible_bounds.height()), in case of android). It would still have to be calculated based on the padding_ratio/transparent_ratio set from ContentViewCore? > > Also, I think it is not great that we are now essentially doing the layout math > for the handle in two different classes: we do it in > CompositedTouchHandleDrawable::SetLayout and in TouchHandle::UpdateLayout. One > way to overcome this would be to replace TouchHandleDrawable::SetFocus with > TouchHandleDrawable::SetOrigin, which would just specify the position where the > handle would need to be drawn. Then we could remove all the layout/positioning > math from TouchHandleDrawable, since it wouldn't need to calculate > focal_offset_from_origin_. The responsibility breakdown would be cleaner: > TouchHandleDrawable would tell TouchHandle about its geometry via GetBounds() > and GetVisibleBounds, and TouchHandle would do all the positioning > calculationsbased on this. > > What do you think? This should be fine. This would allow setting of focal_offset_from_origin_ in CompositedTouchHandleDrawable, and mHotspotX and mHotspotY in PopupTouchHandleDrawable based on the calculations in TouchHandle. Will wait for updates from jdduke, and modify accordingly. > jdduke, does this sound reasonable to you? https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: HandleViewResources.setHandlePadding(0.0f, 0.25f); On 2015/06/18 16:16:31, mfomitchev wrote: > Is the vertical padding zero? I thought we may have some padding on the bottom > of the handle images, no? I have checked on Samsung Note4 and Galaxy S6, padding is not present on the bottom of handle images. https://codereview.chromium.org/481683003/diff/360001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_unittest.cc (right): https://codereview.chromium.org/481683003/diff/360001/ui/touch_selection/touc... ui/touch_selection/touch_handle_unittest.cc:60: gfx::SizeF GetDrawablePadding() const override { return gfx::SizeF(); } On 2015/06/18 16:16:31, mfomitchev wrote: > Once we have the implementation nailed down, we should probably add some unit > tests for layout correctness with non-zero padding. Acknowledged.
https://codereview.chromium.org/481683003/diff/360001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.h (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android... content/browser/android/composited_touch_handle_drawable.h:31: gfx::SizeF GetDrawablePadding() const override; On 2015/06/19 07:20:47, AviD wrote: > On 2015/06/18 16:16:31, mfomitchev wrote: > > Hmm.. These methods are a little hard to understand.. Perhaps instead of > > GetDrawablePadding() we could have GetBounds()? Then the padding could be > > calculated as the difference between the bounds and the visual bounds. I think > > this would make the semantics of these methods easier to grok. This would also > > work if the padding is not the same on top/bottom (unlike e current API). > > Considering that GetBounds() would return the bounds without the padding (i.e, > (visible_bounds.width()*0.75, visible_bounds.height()), in case of android). It > would still have to be calculated based on the padding_ratio/transparent_ratio > set from ContentViewCore? So, considering that we don't actually have vertical padding, perhaps we should just have GetHorizontalPadding() (returning .25 on Android)? Doesn't make much sense to write code to account for vertical padding if we don't actually have it on any platforms. Sorry for the confusion I caused with this.. > > > > > Also, I think it is not great that we are now essentially doing the layout > math > > for the handle in two different classes: we do it in > > CompositedTouchHandleDrawable::SetLayout and in TouchHandle::UpdateLayout. One > > way to overcome this would be to replace TouchHandleDrawable::SetFocus with > > TouchHandleDrawable::SetOrigin, which would just specify the position where > the > > handle would need to be drawn. Then we could remove all the layout/positioning > > math from TouchHandleDrawable, since it wouldn't need to calculate > > focal_offset_from_origin_. The responsibility breakdown would be cleaner: > > TouchHandleDrawable would tell TouchHandle about its geometry via GetBounds() > > and GetVisibleBounds, and TouchHandle would do all the positioning > > calculationsbased on this. > > > > What do you think? > > This should be fine. This would allow setting of focal_offset_from_origin_ in > CompositedTouchHandleDrawable, and mHotspotX and mHotspotY in > PopupTouchHandleDrawable based on the calculations in TouchHandle. Will wait for > updates from jdduke, and modify accordingly. We shouldn't need to calculate focal_offset_from_origin_ in CompositedTouchHandleDrawable.. It would just position the layer exactly at the origin specified in TouchHandleDrawable::SetOrigin(). PopupTouchHandleDrawable.java would also need to change to have SetFocus replaced with SetOrigin, and we shouldn't need mHotspotX/mHotspotY. https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: HandleViewResources.setHandlePadding(0.0f, 0.25f); On 2015/06/19 07:20:47, AviD wrote: > On 2015/06/18 16:16:31, mfomitchev wrote: > > Is the vertical padding zero? I thought we may have some padding on the bottom > > of the handle images, no? > > I have checked on Samsung Note4 and Galaxy S6, padding is not present on the > bottom of handle images. Ok, thanks for checking. I have dug up some emails - it seems we used to have vertical padding, but it was removed recently. I guess assuming 0 padding when there is a discrepancy is the best we can do.
https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:23: public class HandleViewResources { For now, I don't think there's any advantage to having this set dynamically, I'd be OK just initializing these values as constants "HANDLE_HORIZONTAL_PADDING = 0.25f;". https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:169: mHotspotX = mirrorHorizontal ? drawableWidth / 4f : (drawableWidth * 3) / 4f; Hmm, it's a bit odd to have the handle padding come from HandleViewResources, but to not use that value here?
Thanks Avi, I think the last major thing we want here is to add a flag to conditionally enable this feature. We're still deciding what platforms we'd like to roll it out to (L+), but in the meantime it would be good to land this so we can do some more testing/tweaking/experimentation. For now, you can add an Android-specific flag to content_switches (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...), perhaps "kEnableSmartSelectionHandleOrientation" or "kEnableAdaptiveSelectionHandleOrientation". You can then add that as a bit on TouchSelectionController::Config, populate it in render_widget_host_view_android.cc:CreateSelectionController, and add a new TouchHandleClient method that queries whether to enable the feature. Then you'll just shortciruit the re-orientation logic. Sound OK?
Thanks jdduke. I will add the enable/disable bit in the next patch. https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:23: public class HandleViewResources { On 2015/06/19 14:33:58, jdduke wrote: > For now, I don't think there's any advantage to having this set dynamically, I'd > be OK just initializing these values as constants "HANDLE_HORIZONTAL_PADDING = > 0.25f;". Acknowledged. https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:169: mHotspotX = mirrorHorizontal ? drawableWidth / 4f : (drawableWidth * 3) / 4f; On 2015/06/19 14:33:58, jdduke wrote: > Hmm, it's a bit odd to have the handle padding come from HandleViewResources, > but to not use that value here? mfomitchev suggested moving this logic of setting origin to TouchHandle. I can update the logic in TouchHandle to use the values from HandleViewResources. Shall I do it in this patch or as a follow-up patch?
On 2015/06/19 15:25:29, AviD wrote: > mfomitchev suggested moving this logic of setting origin to TouchHandle. > I can update the logic in TouchHandle to use the values from > HandleViewResources. > Shall I do it in this patch or as a follow-up patch? Either way, if it's not too much work it might be nice to unify the offset calculation, but I'd be OK doing that as a follow-up. mfomitchev@ might have some other ideas for further cleanup/refactoring as well after this patch lands.
Moved focal offset and hotspot logic to TouchHandle. Also added a flag based updating of handles, so that the handle is updated only once per frame update, even if many parameters are changed. PTAL. https://codereview.chromium.org/481683003/diff/360001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android... content/browser/android/composited_touch_handle_drawable.cc:155: mirror_horizontal ? layer_width * 0.25f : layer_width * 0.75f; On 2015/06/18 16:16:31, mfomitchev wrote: > .25/.75 shouldn't be hard coded, but should be based on drawable_padding_ Done. This logic is moved to TouchHandle https://codereview.chromium.org/481683003/diff/360001/content/browser/android... File content/browser/android/composited_touch_handle_drawable.h (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android... content/browser/android/composited_touch_handle_drawable.h:31: gfx::SizeF GetDrawablePadding() const override; On 2015/06/19 13:43:29, mfomitchev wrote: > On 2015/06/19 07:20:47, AviD wrote: > > On 2015/06/18 16:16:31, mfomitchev wrote: > > > Hmm.. These methods are a little hard to understand.. Perhaps instead of > > > GetDrawablePadding() we could have GetBounds()? Then the padding could be > > > calculated as the difference between the bounds and the visual bounds. I > think > > > this would make the semantics of these methods easier to grok. This would > also > > > work if the padding is not the same on top/bottom (unlike e current API). > > > > Considering that GetBounds() would return the bounds without the padding (i.e, > > (visible_bounds.width()*0.75, visible_bounds.height()), in case of android). > It > > would still have to be calculated based on the padding_ratio/transparent_ratio > > set from ContentViewCore? > > So, considering that we don't actually have vertical padding, perhaps we should > just have GetHorizontalPadding() (returning .25 on Android)? Doesn't make much > sense to write code to account for vertical padding if we don't actually have it > on any platforms. Sorry for the confusion I caused with this.. > > > > > > > > > Also, I think it is not great that we are now essentially doing the layout > > math > > > for the handle in two different classes: we do it in > > > CompositedTouchHandleDrawable::SetLayout and in TouchHandle::UpdateLayout. > One > > > way to overcome this would be to replace TouchHandleDrawable::SetFocus with > > > TouchHandleDrawable::SetOrigin, which would just specify the position where > > the > > > handle would need to be drawn. Then we could remove all the > layout/positioning > > > math from TouchHandleDrawable, since it wouldn't need to calculate > > > focal_offset_from_origin_. The responsibility breakdown would be cleaner: > > > TouchHandleDrawable would tell TouchHandle about its geometry via > GetBounds() > > > and GetVisibleBounds, and TouchHandle would do all the positioning > > > calculationsbased on this. > > > > > > What do you think? > > > > This should be fine. This would allow setting of focal_offset_from_origin_ in > > CompositedTouchHandleDrawable, and mHotspotX and mHotspotY in > > PopupTouchHandleDrawable based on the calculations in TouchHandle. Will wait > for > > updates from jdduke, and modify accordingly. > > We shouldn't need to calculate focal_offset_from_origin_ in > CompositedTouchHandleDrawable.. It would just position the layer exactly at the > origin specified in TouchHandleDrawable::SetOrigin(). > PopupTouchHandleDrawable.java would also need to change to have SetFocus > replaced with SetOrigin, and we shouldn't need mHotspotX/mHotspotY. Done. https://codereview.chromium.org/481683003/diff/360001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: HandleViewResources.setHandlePadding(0.0f, 0.25f); On 2015/06/19 13:43:29, mfomitchev wrote: > On 2015/06/19 07:20:47, AviD wrote: > > On 2015/06/18 16:16:31, mfomitchev wrote: > > > Is the vertical padding zero? I thought we may have some padding on the > bottom > > > of the handle images, no? > > > > I have checked on Samsung Note4 and Galaxy S6, padding is not present on the > > bottom of handle images. > > Ok, thanks for checking. I have dug up some emails - it seems we used to have > vertical padding, but it was removed recently. I guess assuming 0 padding when > there is a discrepancy is the best we can do. Acknowledged.
Looks great! Just minor comments from me. Thanks for doing this work! https://codereview.chromium.org/481683003/diff/400001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/400001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:24: private static final float sHandleHorizontalPadding = 0.25f; I am not too familiar with Chromium Java code conventions, but based on other examples, seems like constant names should be all uppercase? https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:112: SetUpdateLayoutRequired(); if (visible) ? https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:248: gfx::PointF position = mirror_vertical_ ? focus_top_ : focus_bottom_; Call this "focus" instead of position? IMO position is more ambiguous. https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:252: // Set the Focal offsets for the composited handle layer "focal offsets from origin"? https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:277: gfx::Vector2dF offset_from_origin_ = I'd just remove this variable and use the vector directly in the calc below. If you decide to leave it, please remove the trailing underscore. https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:31: virtual void SetOrigin(const gfx::PointF& origin) = 0; Could you please add some comments to SetOrigin, GetVisibleBounds, and GetDrawableHorizontalPaddin to establish what padding is and how these methods account for it (i.e. that visible bounds do not include padding, while SetOrigin sets the origin of the rect that does include padding)?
Thanks mfomitchev@ PTAL https://codereview.chromium.org/481683003/diff/400001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/400001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:24: private static final float sHandleHorizontalPadding = 0.25f; On 2015/08/06 14:33:02, mfomitchev wrote: > I am not too familiar with Chromium Java code conventions, but based on other > examples, seems like constant names should be all uppercase? I had added the s-prefix for static variable. I will change this to Uppercase like other constants. https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:112: SetUpdateLayoutRequired(); On 2015/08/06 14:33:02, mfomitchev wrote: > if (visible) ? There is a visibility check in SetUpdateLayoutRequired(). 287 if (!is_visible_ || !is_handle_layout_update_required_) 288 return; I thought it would be redundant here. Should I still add it? https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:248: gfx::PointF position = mirror_vertical_ ? focus_top_ : focus_bottom_; On 2015/08/06 14:33:02, mfomitchev wrote: > Call this "focus" instead of position? IMO position is more ambiguous. Done. https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:252: // Set the Focal offsets for the composited handle layer On 2015/08/06 14:33:02, mfomitchev wrote: > "focal offsets from origin"? Done. https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:277: gfx::Vector2dF offset_from_origin_ = On 2015/08/06 14:33:02, mfomitchev wrote: > I'd just remove this variable and use the vector directly in the calc below. If > you decide to leave it, please remove the trailing underscore. Done. https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:31: virtual void SetOrigin(const gfx::PointF& origin) = 0; On 2015/08/06 14:33:02, mfomitchev wrote: > Could you please add some comments to SetOrigin, GetVisibleBounds, and > GetDrawableHorizontalPaddin to establish what padding is and how these methods > account for it (i.e. that visible bounds do not include padding, while SetOrigin > sets the origin of the rect that does include padding)? Done.
LGTM with nits https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:112: SetUpdateLayoutRequired(); On 2015/08/09 13:55:41, AviD wrote: > On 2015/08/06 14:33:02, mfomitchev wrote: > > if (visible) ? > > There is a visibility check in SetUpdateLayoutRequired(). > > 287 if (!is_visible_ || !is_handle_layout_update_required_) > 288 return; > > > I thought it would be redundant here. > > Should I still add it? Yeah, I realize we check visibility in UpdateHandleLayout(). Personally I'd still add the check here - just so that it is crystal clear what is happening in this code without looking anywhere else. I don't feel strongly about this though, so if you feel it would make code harder to understand - feel free to leave it the way it is. https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:39: // |origin| takes care of positioning the handle drawable based on the I think if you say that it's based on handle's visible bounds, it would be more clear. https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:47: // The bounds includes the transparent horizontal padding, which is Nit: I'd drop the "which is" part - IMO generally it's better not to make claims about the behavior of specific implementing classes in the interface. https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:51: // Returns the Transparent horizontal if present in the handle drawable. "padding" missing https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:52: // The transparent padding is present in case of Android handle drawables. I'd drop the second sentence.
https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:112: SetUpdateLayoutRequired(); On 2015/08/10 13:23:45, mfomitchev wrote: > On 2015/08/09 13:55:41, AviD wrote: > > On 2015/08/06 14:33:02, mfomitchev wrote: > > > if (visible) ? > > > > There is a visibility check in SetUpdateLayoutRequired(). > > > > 287 if (!is_visible_ || !is_handle_layout_update_required_) > > 288 return; > > > > > > I thought it would be redundant here. > > > > Should I still add it? > > Yeah, I realize we check visibility in UpdateHandleLayout(). Personally I'd > still add the check here - just so that it is crystal clear what is happening in > this code without looking anywhere else. > > I don't feel strongly about this though, so if you feel it would make code > harder to understand - feel free to leave it the way it is. Added the visibility check here. https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:39: // |origin| takes care of positioning the handle drawable based on the On 2015/08/10 13:23:46, mfomitchev wrote: > I think if you say that it's based on handle's visible bounds, it would be more > clear. Done. https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:47: // The bounds includes the transparent horizontal padding, which is On 2015/08/10 13:23:45, mfomitchev wrote: > Nit: I'd drop the "which is" part - IMO generally it's better not to make claims > about the behavior of specific implementing classes in the interface. Done. https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:51: // Returns the Transparent horizontal if present in the handle drawable. On 2015/08/10 13:23:45, mfomitchev wrote: > "padding" missing Done. https://codereview.chromium.org/481683003/diff/420001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:52: // The transparent padding is present in case of Android handle drawables. On 2015/08/10 13:23:46, mfomitchev wrote: > I'd drop the second sentence. Done.
Sorry for the delayed review, I'd like to get to it this week but it may not happen until next (M45 blockers... =/). In any case, we probably ought to land this after branch anyhow (end of this week), thanks for your patience.
Thanks, haven't looked at the tests yet, mostly just a few more nits. https://codereview.chromium.org/481683003/diff/440001/content/browser/android... File content/browser/android/popup_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/440001/content/browser/android... content/browser/android/popup_touch_handle_drawable.cc:70: const float PopupTouchHandleDrawable::GetPaddingFromResources() { Nit: This is only use once, let's just inline it in the constructor. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:248: gfx::PointF TouchHandle::GetHandleOrigin() const { This definition should follow the declaration order, let's move below UpdateHandleLayout(). https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:254: // based on the orientation Nit: period at the end of comment. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:291: // inversion while dragging of handles Nit: period at end of comment. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:327: drawable_->SetOrigin(origin); Nit: Let's just inline this, drawable_->SetOrigin(GetHandleOrigin()); https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:125: gfx::PointF GetHandleOrigin() const; There's some work here, can we rename this to |ComputeHandleOrigin()|? https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:151: if (viewport_rect_ == viewport_rect) { I don't think we should rely on the |OnViewportChanged| signal being called every frame. I'd rather we trigger UpdateHandleLayoutIfRequired() in OnSelectionChanged/OnInsertionChanged after we finish pushing the properties. This might mean we get two updates when the viewport is changed, but I'd rather that than rely on this getting called every frame. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:175: void TouchSelectionController::UpdateHandleLayoutIfRequired() { Nit: Move the definition down into the same order as the declaration. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:615: UpdateHandleLayoutIfRequired(); I don't think this is necessary. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.h (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:157: bool IsEnabledAdaptiveHandleOrientation() const override; Let's rename this to |IsAdapativeHandleOrientationEnabled()|. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:178: void UpdateHandleLayoutIfRequired(); Keeping with phrasing, let's call use "IfNecessary" instead of "IfRequired".
Thanks jdduke, PTAL. https://codereview.chromium.org/481683003/diff/440001/content/browser/android... File content/browser/android/popup_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/440001/content/browser/android... content/browser/android/popup_touch_handle_drawable.cc:70: const float PopupTouchHandleDrawable::GetPaddingFromResources() { On 2015/08/17 17:20:50, jdduke wrote: > Nit: This is only use once, let's just inline it in the constructor. Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:248: gfx::PointF TouchHandle::GetHandleOrigin() const { On 2015/08/17 17:20:53, jdduke wrote: > This definition should follow the declaration order, let's move below > UpdateHandleLayout(). Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:254: // based on the orientation On 2015/08/17 17:20:53, jdduke wrote: > Nit: period at the end of comment. Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:291: // inversion while dragging of handles On 2015/08/17 17:20:53, jdduke wrote: > Nit: period at end of comment. Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:327: drawable_->SetOrigin(origin); On 2015/08/17 17:20:53, jdduke wrote: > Nit: Let's just inline this, drawable_->SetOrigin(GetHandleOrigin()); Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:125: gfx::PointF GetHandleOrigin() const; On 2015/08/17 17:20:53, jdduke wrote: > There's some work here, can we rename this to |ComputeHandleOrigin()|? Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:151: if (viewport_rect_ == viewport_rect) { On 2015/08/17 17:20:53, jdduke wrote: > I don't think we should rely on the |OnViewportChanged| signal being called > every frame. I'd rather we trigger UpdateHandleLayoutIfRequired() in > OnSelectionChanged/OnInsertionChanged after we finish pushing the properties. > > This might mean we get two updates when the viewport is changed, but I'd rather > that than rely on this getting called every frame. Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:175: void TouchSelectionController::UpdateHandleLayoutIfRequired() { On 2015/08/17 17:20:53, jdduke wrote: > Nit: Move the definition down into the same order as the declaration. Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:615: UpdateHandleLayoutIfRequired(); On 2015/08/17 17:20:53, jdduke wrote: > I don't think this is necessary. Right. If I move UpdateHandleLayoutIfRequired() to onSelectionChanged() and onInsertionChanged(), this would not be required. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.h (right): https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:157: bool IsEnabledAdaptiveHandleOrientation() const override; On 2015/08/17 17:20:53, jdduke wrote: > Let's rename this to |IsAdapativeHandleOrientationEnabled()|. Done. https://codereview.chromium.org/481683003/diff/440001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.h:178: void UpdateHandleLayoutIfRequired(); On 2015/08/17 17:20:53, jdduke wrote: > Keeping with phrasing, let's call use "IfNecessary" instead of "IfRequired". Done.
jdduke@ Are there any other changes that you would like me to do or any more test cases you would like me to add?
On 2015/08/25 12:57:30, AviD wrote: > jdduke@ Are there any other changes that you would like me to do or any more > test cases you would like me to add? Again sorry for the delay. My main concern now is to make sure this works well with WebView. Have you ever built WebView? There are some instructions here: https://www.chromium.org/developers/how-tos/build-instructions-android-webview, however, they're a bit involved. If that's too much, I'd be happy to do some manual testing locally, but I may not get to it until tomorrow.
https://codereview.chromium.org/481683003/diff/480001/content/browser/android... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/481683003/diff/480001/content/browser/android... content/browser/android/content_startup_flags.cc:69: parsed_command_line->AppendSwitch( I think we still need to address WebView behavior consistency. Let's go ahead and leave this off by default for now. That way we can land this and avoid bit rot, and work toward turning it on. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:136: void TouchHandle::SetViewportRect(const gfx::RectF viewport_rect) { Nit: Pass by const ref. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:33: // if required for Adaptive Handle Orientation. Nit: "Adaptive Handle Orientation" should be all lowercase. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:38: // Sets the Origin position of the touch handle. Nit: Origin shouldn't be capitalized. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:50: // Returns the Transparent horizontal padding of the handle drawable. Should we call this |GetDrawableHorizontalPaddingRatio|? I think this needs a little more explanation. Nit: Transparent shouldn't be capitalized. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:74: const gfx::RectF viewport_rect); Nit: Pass by const ref. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:100: void SetViewportRect(const gfx::RectF viewport_rect); Nit: Pass by const ref.
@jdduke I built webview and tried replacing the SystemWebView on my Note4 device, but that failed with an error :( I have disabled this for now. Will enable it after checking on Webview. https://codereview.chromium.org/481683003/diff/480001/content/browser/android... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/481683003/diff/480001/content/browser/android... content/browser/android/content_startup_flags.cc:69: parsed_command_line->AppendSwitch( On 2015/09/08 15:42:55, jdduke wrote: > I think we still need to address WebView behavior consistency. Let's go ahead > and leave this off by default for now. That way we can land this and avoid bit > rot, and work toward turning it on. I have disabled this from RenderWidgetHostViewAndroid::CreateSelectionController() for now. Will enable it after webview behavior consistency. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:136: void TouchHandle::SetViewportRect(const gfx::RectF viewport_rect) { On 2015/09/08 15:42:55, jdduke wrote: > Nit: Pass by const ref. Done. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:33: // if required for Adaptive Handle Orientation. On 2015/09/08 15:42:55, jdduke wrote: > Nit: "Adaptive Handle Orientation" should be all lowercase. Done. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:38: // Sets the Origin position of the touch handle. On 2015/09/08 15:42:55, jdduke wrote: > Nit: Origin shouldn't be capitalized. Done. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:50: // Returns the Transparent horizontal padding of the handle drawable. On 2015/09/08 15:42:55, jdduke wrote: > Should we call this |GetDrawableHorizontalPaddingRatio|? I think this needs a > little more explanation. > > Nit: Transparent shouldn't be capitalized. Done. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:74: const gfx::RectF viewport_rect); On 2015/09/08 15:42:55, jdduke wrote: > Nit: Pass by const ref. Done. https://codereview.chromium.org/481683003/diff/480001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:100: void SetViewportRect(const gfx::RectF viewport_rect); On 2015/09/08 15:42:55, jdduke wrote: > Nit: Pass by const ref. Done.
With the flag change I'll stamp this for landing. https://codereview.chromium.org/481683003/diff/500001/content/browser/android... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/481683003/diff/500001/content/browser/android... content/browser/android/content_startup_flags.cc:69: switches::kEnableAdaptiveHandleOrientation); Can we just not add the flag here? I'd much prefer that to ignoring the flag. https://codereview.chromium.org/481683003/diff/500001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/481683003/diff/500001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:273: // consistent on webview Please keep the flag check here, otherwise it's of no use. Rather, we should just not add the flag by default. I'd like to be able to test this locally with a flag.
avi.nitk@samsung.com changed reviewers: + jochen@chromium.org
Thanks @jdduke https://codereview.chromium.org/481683003/diff/500001/content/browser/android... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/481683003/diff/500001/content/browser/android... content/browser/android/content_startup_flags.cc:69: switches::kEnableAdaptiveHandleOrientation); On 2015/09/09 22:19:50, jdduke wrote: > Can we just not add the flag here? I'd much prefer that to ignoring the flag. Done. https://codereview.chromium.org/481683003/diff/500001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/481683003/diff/500001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:273: // consistent on webview On 2015/09/09 22:19:50, jdduke wrote: > Please keep the flag check here, otherwise it's of no use. Rather, we should > just not add the flag by default. I'd like to be able to test this locally with > a flag. Done.
lgtm
Thanks, I think you'll need to update ui/touch_selection/touch_handle_drawable_aura.cc for this to compile on Aura then we'll be good to go. https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:128: if (focus_top_ == top && focus_bottom_ == bottom) Don't we want to keep the DCHECK(enabled_)? https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:354: // Handle layout may is deferred while the handle is dragged. Nit: "may be deferred". https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:153: } Nit: No need for braces on the return line. https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:550: end_selection_handle_->SetViewportRect(viewport_rect_); Nit: Can we set the viewport after we set enabled (both here and for the insertion handle)? I think a common condition in our property setting is that properties affectiving visibility only change when the handle is enabled.
https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:128: if (focus_top_ == top && focus_bottom_ == bottom) On 2015/09/14 15:19:40, jdduke wrote: > Don't we want to keep the DCHECK(enabled_)? Added DCHECK for SetFocus and SetViewportRect as well. https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:354: // Handle layout may is deferred while the handle is dragged. On 2015/09/14 15:19:40, jdduke wrote: > Nit: "may be deferred". Done. https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:153: } On 2015/09/14 15:19:40, jdduke wrote: > Nit: No need for braces on the return line. Done. https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:550: end_selection_handle_->SetViewportRect(viewport_rect_); On 2015/09/14 15:19:40, jdduke wrote: > Nit: Can we set the viewport after we set enabled (both here and for the > insertion handle)? I think a common condition in our property setting is that > properties affectiving visibility only change when the handle is enabled. Done.
Will update touch_handle_drawable_aura.cc shortly. On 2015/09/14 15:57:40, AviD wrote: > https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... > File ui/touch_selection/touch_handle.cc (right): > > https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... > ui/touch_selection/touch_handle.cc:128: if (focus_top_ == top && focus_bottom_ > == bottom) > On 2015/09/14 15:19:40, jdduke wrote: > > Don't we want to keep the DCHECK(enabled_)? > > Added DCHECK for SetFocus and SetViewportRect as well. > > https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... > ui/touch_selection/touch_handle.cc:354: // Handle layout may is deferred while > the handle is dragged. > On 2015/09/14 15:19:40, jdduke wrote: > > Nit: "may be deferred". > > Done. > > https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... > File ui/touch_selection/touch_selection_controller.cc (right): > > https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... > ui/touch_selection/touch_selection_controller.cc:153: } > On 2015/09/14 15:19:40, jdduke wrote: > > Nit: No need for braces on the return line. > > Done. > > https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touc... > ui/touch_selection/touch_selection_controller.cc:550: > end_selection_handle_->SetViewportRect(viewport_rect_); > On 2015/09/14 15:19:40, jdduke wrote: > > Nit: Can we set the viewport after we set enabled (both here and for the > > insertion handle)? I think a common condition in our property setting is that > > properties affectiving visibility only change when the handle is enabled. > > Done.
@jdduke: PTAL
mohsen@: Can you look at the touch_handle_drawable_aura.h/cc changes? I don't think we can just treat the new "origin" argument as the "focus" property. https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_drawable_aura.cc (right): https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:102: if (orientation_ == orientation) Since it's unsupported, can you add DCHECK(!mirror_vertical); DCHECK(!mirror_horizontal); and move the TODO from the header above those 2 lines? https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:138: focal_position_ = position; Are you sure this is correct? https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:165: return 0.0f; Is that true?
mohsen@chromium.org changed reviewers: + mohsen@chromium.org
https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_drawable_aura.cc (right): https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:138: focal_position_ = position; On 2015/09/14 18:38:41, jdduke wrote: > Are you sure this is correct? Now that we set origin instead of focus point, we should get rid of |relative_bounds_| that was used to calculate origin from focus point. (Though, kSelectionHandlePadding should still be taken into account) https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:165: return 0.0f; On 2015/09/14 18:38:41, jdduke wrote: > Is that true? Yes, there is no padding around handles in aura resources.
https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_drawable_aura.cc (right): https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:102: if (orientation_ == orientation) On 2015/09/14 18:38:41, jdduke wrote: > Since it's unsupported, can you add > > DCHECK(!mirror_vertical); > DCHECK(!mirror_horizontal); > > and move the TODO from the header above those 2 lines? Acknowledged. https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:138: focal_position_ = position; On 2015/09/14 21:52:41, mohsen wrote: > On 2015/09/14 18:38:41, jdduke wrote: > > Are you sure this is correct? > > Now that we set origin instead of focus point, we should get rid of > |relative_bounds_| that was used to calculate origin from focus point. (Though, > kSelectionHandlePadding should still be taken into account) @mohsen: I have changed SetOrientation to : void TouchHandleDrawableAura::SetOrientation(TouchHandleOrientation orientation, bool mirror_vertical, bool mirror_horizontal) { DCHECK(!mirror_vertical); DCHECK(!mirror_horizontal); if (orientation_ == orientation) return; orientation_ = orientation; gfx::Image* image = GetHandleImage(orientation); window_delegate_->SetImage(*image); // Calculate the relative bounds. gfx::Size image_size = image->Size(); int window_width = image_size.width() + 2 * kSelectionHandlePadding; int window_height = image_size.height() + 2 * kSelectionHandlePadding; relative_bounds_ = gfx::RectF( -kSelectionHandlePadding, kSelectionHandleVerticalVisualOffset - kSelectionHandlePadding, window_width, window_height); UpdateBounds(); } Would this be fine? Or do you prefer getting rid of relative_bounds_ completely and make the corresponding calculations in UpdateBounds()? Additionally, could you guide me to any resource available for enabling touch handles and testing this for Aura?
https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_drawable_aura.cc (right): https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.cc:138: focal_position_ = position; On 2015/09/21 15:40:16, AviD wrote: > On 2015/09/14 21:52:41, mohsen wrote: > > On 2015/09/14 18:38:41, jdduke wrote: > > > Are you sure this is correct? > > > > Now that we set origin instead of focus point, we should get rid of > > |relative_bounds_| that was used to calculate origin from focus point. > (Though, > > kSelectionHandlePadding should still be taken into account) > > @mohsen: > > I have changed SetOrientation to : > > void TouchHandleDrawableAura::SetOrientation(TouchHandleOrientation orientation, > bool mirror_vertical, > bool mirror_horizontal) { > DCHECK(!mirror_vertical); > DCHECK(!mirror_horizontal); > if (orientation_ == orientation) > return; > orientation_ = orientation; > gfx::Image* image = GetHandleImage(orientation); > window_delegate_->SetImage(*image); > > // Calculate the relative bounds. > gfx::Size image_size = image->Size(); > int window_width = image_size.width() + 2 * kSelectionHandlePadding; > int window_height = image_size.height() + 2 * kSelectionHandlePadding; > relative_bounds_ = gfx::RectF( > -kSelectionHandlePadding, > kSelectionHandleVerticalVisualOffset - kSelectionHandlePadding, > window_width, > window_height); > UpdateBounds(); > } > > Would this be fine? Or do you prefer getting rid of relative_bounds_ completely > and make the corresponding calculations in UpdateBounds()? > > Additionally, could you guide me to any resource available for enabling touch > handles and testing this for Aura? Yeah, that seems fine to me. These handles are enabled on Aura. So, you can try any recent Aura build (i.e., Chrome OS, Linux, Windows) to test your changes.
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); When does this layout update actually happen? For example, if we hide the handle, move it, and then make it visible, you just set this 'required' flag. Do we need to call UpdateHandleLayout() after setting visibility?
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); On 2015/09/22 18:20:22, mohsen wrote: > When does this layout update actually happen? For example, if we hide the > handle, move it, and then make it visible, you just set this 'required' flag. Do > we need to call UpdateHandleLayout() after setting visibility? The visibility parameter would be set from the frame update calls in TouchSelectionController. There is a UpdateHandleLayoutIfNecessary() that is called after setting visibility and other parameters (position, orientation etc). If any of the layout parameters of the handle are changed, it would set the 'required' flag (More than one parameter can be updated per frame update call). UpdateHandleLayoutIfNecessary() calls UpdateHandleLayout() if selection or insertion is active so that all changed parameters are considered in a single drawable update call. This is done to avoid multiple drawable updates per frame update call.
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); On 2015/09/23 05:54:56, AviD wrote: > On 2015/09/22 18:20:22, mohsen wrote: > > When does this layout update actually happen? For example, if we hide the > > handle, move it, and then make it visible, you just set this 'required' flag. > Do > > we need to call UpdateHandleLayout() after setting visibility? > > The visibility parameter would be set from the frame update calls in > TouchSelectionController. There is a UpdateHandleLayoutIfNecessary() that is > called after setting visibility and other parameters (position, orientation > etc). If any of the layout parameters of the handle are changed, it would set > the 'required' flag (More than one parameter can be updated per frame update > call). > UpdateHandleLayoutIfNecessary() calls UpdateHandleLayout() if selection or > insertion is active so that all changed parameters are considered in a single > drawable update call. This is done to avoid multiple drawable updates per frame > update call. There is at least one other case that sets visibility (other than frame update) and that is TouchSelectionController::SetTemporariliyHidden(). One place that calls this function is in TouchSelectionControllerClientAura when user starts/ends scrolling the page. You probably need to call UpdateHandleLayoutIfNecessary() from within SetTemporarilyHidden() after it sets handle visibility?
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touc... ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); On 2015/09/23 06:03:56, mohsen wrote: > On 2015/09/23 05:54:56, AviD wrote: > > On 2015/09/22 18:20:22, mohsen wrote: > > > When does this layout update actually happen? For example, if we hide the > > > handle, move it, and then make it visible, you just set this 'required' > flag. > > Do > > > we need to call UpdateHandleLayout() after setting visibility? > > > > The visibility parameter would be set from the frame update calls in > > TouchSelectionController. There is a UpdateHandleLayoutIfNecessary() that is > > called after setting visibility and other parameters (position, orientation > > etc). If any of the layout parameters of the handle are changed, it would set > > the 'required' flag (More than one parameter can be updated per frame update > > call). > > UpdateHandleLayoutIfNecessary() calls UpdateHandleLayout() if selection or > > insertion is active so that all changed parameters are considered in a single > > drawable update call. This is done to avoid multiple drawable updates per > frame > > update call. > > There is at least one other case that sets visibility (other than frame update) > and that is TouchSelectionController::SetTemporariliyHidden(). One place that > calls this function is in TouchSelectionControllerClientAura when user > starts/ends scrolling the page. You probably need to call > UpdateHandleLayoutIfNecessary() from within SetTemporarilyHidden() after it sets > handle visibility? Done.
Aura handles LGTM
lgtm https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:61: virtual void SetNeedsAnimate() = 0; I'd feel slightly better if we had a |SetNeedsLayout()| call here, to make the layout requirement explicit to the caller. This patch has seen enough refactorings though, perhaps add a TODO in the SetUpdateLayoutRequired() implementation. https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:131: void SetUpdateLayoutRequired() { is_handle_layout_update_required_ = true; } Please move this implementation into the .cc file.
https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_drawable_aura.h (right): https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.h:52: gfx::PointF focal_position_; Forgot to mention that the variable name and comments need to be updated (focal_position_ -> origin_position_).
Thanks @jdduke @mohsen https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:61: virtual void SetNeedsAnimate() = 0; On 2015/09/23 15:46:33, jdduke wrote: > I'd feel slightly better if we had a |SetNeedsLayout()| call here, to make the > layout requirement explicit to the caller. This patch has seen enough > refactorings though, perhaps add a TODO in the SetUpdateLayoutRequired() > implementation. Ok. Added a TODO for now. https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... ui/touch_selection/touch_handle.h:131: void SetUpdateLayoutRequired() { is_handle_layout_update_required_ = true; } On 2015/09/23 15:46:33, jdduke wrote: > Please move this implementation into the .cc file. Done. https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... File ui/touch_selection/touch_handle_drawable_aura.h (right): https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touc... ui/touch_selection/touch_handle_drawable_aura.h:52: gfx::PointF focal_position_; On 2015/09/23 16:27:43, mohsen wrote: > Forgot to mention that the variable name and comments need to be updated > (focal_position_ -> origin_position_). Done.
The CQ bit was checked by avi.nitk@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org, jochen@chromium.org, jdduke@chromium.org, mohsen@chromium.org Link to the patchset: https://codereview.chromium.org/481683003/#ps620001 (title: "incorporated changes as per review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/620001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Looks like you need a rebase, let me know if you have issues making that happen, I imagine there were a number of potentially conflicting changes in the meantime.
On 2015/09/24 15:05:50, jdduke wrote: > Looks like you need a rebase, let me know if you have issues making that happen, > I imagine there were a number of potentially conflicting changes in the > meantime. Thanks jdduke. There were some merge conflicts in popup_tuouch_handle_drawable files. I have resolved them and uploaded the patch.
The CQ bit was checked by avi.nitk@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, jochen@chromium.org, mfomitchev@chromium.org, mohsen@chromium.org Link to the patchset: https://codereview.chromium.org/481683003/#ps680001 (title: "rebase fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/680001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/680001
The CQ bit was unchecked by jdduke@chromium.org
Sorry, just a couple more nits after the rebase. https://codereview.chromium.org/481683003/diff/680001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:207: final boolean hadValidOrientation = mOrientation != TouchHandleOrientation.UNDEFINED; Do we still need |hadValidOrientation|? https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:215: if (orientationChanged) mDrawable = getHandleDrawable(); I think we need to schedule an invalidate here: final boolean mirroringChanged = mMirroHorizontal != mirrorHorizontal || mMirrorVertical != mirrorVertical; ... if (orientationChanged) { mDrawable = getHandleDrawable(); if (mDrawable != null) mDrawable.setAlpha(...); } if (orientationChanged || mirroringChanged) scheduleInvalidate(); https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:217: mDrawable.setAlpha((int) (255 * mAlpha)); Please check the drawable for null. There are in fact minor cases in the wild where this can happen, and we don't want to crash (we still have an debug assert because this shouldn't happen with stock Android builds, but can occur with certain roms...). https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:220: private Drawable getHandleDrawable() { Can we make this static and pass in the orientation?
The CQ bit was unchecked by avi.nitk@samsung.com
PTAL https://codereview.chromium.org/481683003/diff/680001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:207: final boolean hadValidOrientation = mOrientation != TouchHandleOrientation.UNDEFINED; On 2015/09/25 16:44:03, jdduke wrote: > Do we still need |hadValidOrientation|? Removed. https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:215: if (orientationChanged) mDrawable = getHandleDrawable(); On 2015/09/25 16:44:03, jdduke wrote: > I think we need to schedule an invalidate here: > > final boolean mirroringChanged = mMirroHorizontal != mirrorHorizontal || > mMirrorVertical != mirrorVertical; > > ... > > if (orientationChanged) { > mDrawable = getHandleDrawable(); > if (mDrawable != null) mDrawable.setAlpha(...); > } > > if (orientationChanged || mirroringChanged) scheduleInvalidate(); Ok. We always call the setOrigin() call after setOrientation() from TouchHandle. setOrigin() would take care of calling scheduleInvalidate(). I will refactor this further to set both in a single call. For now, I will make changes as per your comments. https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:217: mDrawable.setAlpha((int) (255 * mAlpha)); On 2015/09/25 16:44:03, jdduke wrote: > Please check the drawable for null. There are in fact minor cases in the wild > where this can happen, and we don't want to crash (we still have an debug assert > because this shouldn't happen with stock Android builds, but can occur with > certain roms...). Done. https://codereview.chromium.org/481683003/diff/680001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:220: private Drawable getHandleDrawable() { On 2015/09/25 16:44:03, jdduke wrote: > Can we make this static and pass in the orientation? Done.
lgtm https://codereview.chromium.org/481683003/diff/700001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/481683003/diff/700001/content/public/common/c... content/public/common/content_switches.cc:888: const char kEnableAdaptiveHandleOrientation[] = Can we rename this to |kEnableAdaptiveSelectionHandleOrientation| (and "enable-adaptive-selection-handle-orientation")? I realize the standalone "handle" noun may not be the most globally descriptive name.
https://codereview.chromium.org/481683003/diff/700001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/481683003/diff/700001/content/public/common/c... content/public/common/content_switches.cc:888: const char kEnableAdaptiveHandleOrientation[] = On 2015/09/28 15:55:13, jdduke wrote: > Can we rename this to |kEnableAdaptiveSelectionHandleOrientation| (and > "enable-adaptive-selection-handle-orientation")? I realize the standalone > "handle" noun may not be the most globally descriptive name. Done.
The CQ bit was checked by avi.nitk@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mfomitchev@chromium.org, mohsen@chromium.org, jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/481683003/#ps720001 (title: "changed switch name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/720001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/720001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Since it's so close to branch point (this Friday), I'm somewhat inclined for us to land this after branch (any time on Sunday or thereafter) in the event that any unexpected issues pop up . I don't expect any related changes to land in the meantime so there should be minimal rebase conflicts.
On 2015/09/29 15:08:45, jdduke wrote: > Since it's so close to branch point (this Friday), I'm somewhat inclined for us > to land this after branch (any time on Sunday or thereafter) in the event that > any unexpected issues pop up . I don't expect any related changes to land in the > meantime so there should be minimal rebase conflicts. Sure, I'll rebase the on Monday and upload the same.
The CQ bit was checked by avi.nitk@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, jochen@chromium.org, mfomitchev@chromium.org, mohsen@chromium.org Link to the patchset: https://codereview.chromium.org/481683003/#ps740001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/740001
Message was sent while issue was closed.
Committed patchset #38 (id:740001)
Message was sent while issue was closed.
Patchset 38 (id:??) landed as https://crrev.com/348be68968d377e8a3cb55af747784936aa56185 Cr-Commit-Position: refs/heads/master@{#352319} |
