[Android] Allow custom ActionMode creation via ContentViewClient

[Android] Allow custom ActionMode creation via ContentViewClient

Use the ContentViewClient to create the selection ActionMode. This
allows the embedder to completely control the style and behavior
of the ActionMode popup.

BUG=389153
NOTRY=true

Committed: https://crrev.com/3628fb3825e9119a4f96b0d425ffdf2c608bd6b7
Cr-Commit-Position: refs/heads/master@{#325154}

[jdduke (slow), 2015-04-08 00:37:59]

jdduke@chromium.org changed reviewers:
+ sgurun@chromium.org

[jdduke (slow), 2015-04-08 00:38:00]

+sgurun: I guess we can push forward with review on this. I'll add more owners
once you think the approach is passable (note that the current build is failing
because I need to update some native unit tests).

[sgurun-gerrit only, 2015-04-08 04:06:34]

On 2015/04/08 00:38:00, jdduke wrote:
> +sgurun: I guess we can push forward with review on this. I'll add more owners
> once you think the approach is passable (note that the current build is
failing
> because I need to update some native unit tests).

looked at aw and skimmed through the rest. seems good to me. Will review again
once the tests pass.

[jdduke (slow), 2015-04-09 00:26:20]

OK, tests now passing, thanks for reviewing Selim. Note that this will break
downstream builds, so I'll certainly wait to land this before we get the
downstream changes in order.

+aelias for content/browser/renderer_host/

[sgurun-gerrit only, 2015-04-09 05:01:20]

On 2015/04/09 00:26:20, jdduke wrote:
> OK, tests now passing, thanks for reviewing Selim. Note that this will break
> downstream builds, so I'll certainly wait to land this before we get the
> downstream changes in order.
> 
> +aelias for content/browser/renderer_host/

lgtm. I am not super familiar with selection logic even though I fixed a few
bugs here and there.

[sgurun-gerrit only, 2015-04-09 05:01:31]

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:578:

nit: stale line

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java
(right):

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java:161:
if (mEditable || mIsInsertion || mIncognito ||
!mActionHandler.isWebSearchAvailable()) {
why can't you get mIncognito from mActionHandler here?

[jdduke (slow), 2015-04-09 17:46:19]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org, mfomitchev@chromium.org

[jdduke (slow), 2015-04-09 17:46:20]

+aelias for content/browser/renderer_host/
+mfomitchev for ui/touch_selection

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:578:

On 2015/04/09 05:01:31, sgurun wrote:
> nit: stale line

Done.

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java
(right):

https://codereview.chromium.org/1066053002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java:161:
if (mEditable || mIsInsertion || mIncognito ||
!mActionHandler.isWebSearchAvailable()) {
On 2015/04/09 05:01:31, sgurun wrote:
> why can't you get mIncognito from mActionHandler here?

No reason, I think I originally cached it to guard against unnecessary view
recreations, but since it should never change, there's no need. Done.

[aelias_OOO_until_Jul13, 2015-04-09 19:58:20]

content/browser/renderer_host/ lgtm

[mfomitchev, 2015-04-10 15:04:04]

I thought we agreed that we wouldn't pass the positions with the SelectionEvent
and instead have a method in the controller to query the "anchor rect".
I have made similar changes under the 'unification' umbrella - take a look at
the latest revision in https://codereview.chromium.org/1046783002. It's still
pretty wip, but check out GetRectBetweenBounds() in TSC.

[jdduke (slow), 2015-04-10 15:55:57]

On 2015/04/10 15:04:04, mfomitchev wrote:
> I thought we agreed that we wouldn't pass the positions with the
SelectionEvent
> and instead have a method in the controller to query the "anchor rect".
> I have made similar changes under the 'unification' umbrella - take a look at
> the latest revision in https://codereview.chromium.org/1046783002. It's still
> pretty wip, but check out GetRectBetweenBounds() in TSC.

Right, we can always refactor this change for Android (should be pretty trivial,
just have the TouchSelectionControllerAndroid pull the coords before passing to
its client). I suppose I can do that now, whatever you prefer.

[mfomitchev, 2015-04-10 16:23:09]

On 2015/04/10 15:55:57, jdduke wrote:
> On 2015/04/10 15:04:04, mfomitchev wrote:
> > I thought we agreed that we wouldn't pass the positions with the
> SelectionEvent
> > and instead have a method in the controller to query the "anchor rect".
> > I have made similar changes under the 'unification' umbrella - take a look
at
> > the latest revision in https://codereview.chromium.org/1046783002. It's
still
> > pretty wip, but check out GetRectBetweenBounds() in TSC.
> 
> Right, we can always refactor this change for Android (should be pretty
trivial,
> just have the TouchSelectionControllerAndroid pull the coords before passing
to
> its client). I suppose I can do that now, whatever you prefer.

Ok, ui/touch_selection/ LGTM as long as bottom bound positions is all we need.
But as discussed on the IM, we probably need more for correct positioning.

[jdduke (slow), 2015-04-10 18:46:37]

Patchset #5 (id:80001) has been deleted

[jdduke (slow), 2015-04-14 00:07:39]

Mikhail: Can you take a look at the changes to TouchSelectionController, namely,
the addition of GetRectBetweenBounds() and
GetRectBetweenBoundsIncludingHandles()? As you suspected we definitely need the
line height data as well as the handle rects to properly position the popup.

I tried to make it friendly to the pending unification code, though there are
some rounding differences we may want to finalize at some point. If you think it
looks OK, I'll add a few tests. Thanks.

[mfomitchev, 2015-04-14 15:02:34]

https://codereview.chromium.org/1066053002/diff/140001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1066053002/diff/140001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:245: return
RectFBetweenSelectionBounds(start_, end_);
Can't we also have a case when non are visible (but selection is active)? E.g.
if the selection was scrolled off the screen.

https://codereview.chromium.org/1066053002/diff/140001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:248: gfx::RectF
TouchSelectionController::GetRectBetweenBoundsIncludingHandles()
Having a method returning MaxHandleImageSize/MaxHandleVisibleSize would give us
more flexibility, since we'd be able to position the menu between the two bounds
if they are far enough. This is what the Aura implementation does (see
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/touchui/t...).
The downside is that this logic becomes more complicated when you introduce
inverted/flipped handles, but IMHO this is the complexity we have to deal with.
Always positioning the menu below the handles even when there is plenty of space
in between looks pretty weird on a desktop. Take a look:
https://drive.google.com/open?id=0B2LVVIKSxUVYS0lqYkJEWDFNYWc&authuser=0. I
suspect it will look weird on a tablet as well.

[jdduke (slow), 2015-04-14 15:19:30]

https://codereview.chromium.org/1066053002/diff/140001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1066053002/diff/140001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:245: return
RectFBetweenSelectionBounds(start_, end_);
On 2015/04/14 15:02:33, mfomitchev wrote:
> Can't we also have a case when non are visible (but selection is active)? E.g.
> if the selection was scrolled off the screen.

For sure, but I think the controller should be somewhat agnostic to the View
bounds. That is, don't you want the handles to display even if the selection
endpoint are both offscreen? There are also cases where the selection may be
onscreen but the handles themselves are occluded, in which case I think we still
may want the full selection region. We can then let the client do the right
thing with respect to either clipping the selection rect or dropping the rect if
it exceeds the viewport.

https://codereview.chromium.org/1066053002/diff/140001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:248: gfx::RectF
TouchSelectionController::GetRectBetweenBoundsIncludingHandles()
On 2015/04/14 15:02:33, mfomitchev wrote:
> Having a method returning MaxHandleImageSize/MaxHandleVisibleSize would give
us
> more flexibility, since we'd be able to position the menu between the two
bounds
> if they are far enough. This is what the Aura implementation does (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/touchui/t...).
> The downside is that this logic becomes more complicated when you introduce
> inverted/flipped handles, but IMHO this is the complexity we have to deal
with.
> Always positioning the menu below the handles even when there is plenty of
space
> in between looks pretty weird on a desktop. Take a look:
> https://drive.google.com/open?id=0B2LVVIKSxUVYS0lqYkJEWDFNYWc&authuser=0. I
> suspect it will look weird on a tablet as well.

Well, we may simply have to differ between platforms here. The Android API
doesn't offer this distinction; we only provide it one bounding rect. I'd
generalize this method to support Aura's approach, but that might be premature.

[jdduke (slow), 2015-04-14 18:39:11]

jdduke@chromium.org changed reviewers:
+ sadrul@chromium.org

[jdduke (slow), 2015-04-14 18:39:12]

+sadrul for ui/base/touch.

Mikhail, I made the changes we discusse, could you take another look at
TouchSelectionController? Thanks.

https://codereview.chromium.org/1066053002/diff/160001/ui/base/touch/selectio...
File ui/base/touch/selection_bound.cc (right):

https://codereview.chromium.org/1066053002/diff/160001/ui/base/touch/selectio...
ui/base/touch/selection_bound.cc:70: gfx::PointF top_left(b1.edge_top());
Note that we could implement RectBetweenSelectionBounds in terms of
RectFBetweenSelectionBounds, and we might also rename RectBetweenSelectionBounds
to RoundedRectBetweenSelectionBounds.

[jdduke (slow), 2015-04-14 18:45:25]

Patchset #9 (id:180001) has been deleted

[jdduke (slow), 2015-04-14 18:59:02]

Patchset #9 (id:200001) has been deleted

[mfomitchev, 2015-04-14 19:37:51]

LGTM

[sadrul, 2015-04-14 19:38:43]

lgtm

https://codereview.chromium.org/1066053002/diff/220001/ui/base/touch/selectio...
File ui/base/touch/selection_bound.h (right):

https://codereview.chromium.org/1066053002/diff/220001/ui/base/touch/selectio...
ui/base/touch/selection_bound.h:72: 
mfomitchev@ Can you look into replacing the uses of RectBetweenSelectionBounds()
(which seems to be used only in aura) with RectFBetweenSelectionBounds() (+
appropriate To*Rect() functions if need be)?

[jdduke (slow), 2015-04-14 19:44:19]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-04-14 19:44:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1066053002/#ps220001
(title: "Final fix")

[commit-bot: I haz the power, 2015-04-14 19:44:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066053002/220001

[jdduke (slow), 2015-04-14 21:25:09]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2015-04-14 21:49:14]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-04-14 21:49:15]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
aelias@chromium.org, sadrul@chromium.org, mfomitchev@chromium.org
Link to the patchset: https://codereview.chromium.org/1066053002/#ps230001
(title: "Fix null check")

[commit-bot: I haz the power, 2015-04-14 21:50:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066053002/230001

[jdduke (slow), 2015-04-15 00:06:16]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2015-04-15 00:06:54]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-04-15 00:07:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066053002/230001

[commit-bot: I haz the power, 2015-04-15 00:09:15]

Committed patchset #10 (id:230001)

[commit-bot: I haz the power, 2015-04-15 00:10:06]

Patchset 10 (id:??) landed as
https://crrev.com/3628fb3825e9119a4f96b0d425ffdf2c608bd6b7
Cr-Commit-Position: refs/heads/master@{#325154}