Support for Adaptive Handle Orientation

content/browser/android/composited_touch_handle_drawable.h

Index: content/browser/android/composited_touch_handle_drawable.h
diff --git a/content/browser/android/composited_touch_handle_drawable.h b/content/browser/android/composited_touch_handle_drawable.h
index 28c6910b9d437dfa96c8a2962c2dc03c8ce6033f..625d4373d1e15354c78c33fd2fb629896fac392b 100644
--- a/content/browser/android/composited_touch_handle_drawable.h
+++ b/content/browser/android/composited_touch_handle_drawable.h
@@ -22,10 +22,13 @@ class CompositedTouchHandleDrawable : public ui::TouchHandleDrawable {
 
   // ui::TouchHandleDrawable implementation.
   void SetEnabled(bool enabled) override;
-  void SetOrientation(ui::TouchHandleOrientation orientation) override;
+  void SetLayout(const gfx::PointF& position,
+                 ui::TouchHandleOrientation orientation,
+                 bool mirror_vertical,
+                 bool mirror_horizontal) override;
   void SetAlpha(float alpha) override;
-  void SetFocus(const gfx::PointF& position) override;
   gfx::RectF GetVisibleBounds() const override;
+  gfx::SizeF GetDrawablePadding() const override;

[mfomitchev, 2015/06/18 16:16:31]

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?

[AviD, 2015/06/19 07:20:47]

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

[mfomitchev, 2015/06/19 13:43:29]

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

[AviD, 2015/07/30 15:09:55]

Done.

 
   static bool RegisterHandleViewResources(JNIEnv* env);
 
@@ -34,6 +37,7 @@ class CompositedTouchHandleDrawable : public ui::TouchHandleDrawable {
   void UpdateLayerPosition();
 
   const float dpi_scale_;
+  gfx::SizeF drawable_padding_;
   ui::TouchHandleOrientation orientation_;
   gfx::PointF focal_position_;
   gfx::Vector2dF focal_offset_from_origin_;