[Android] Supporting floating select ActionModes for web content

[Android] Supporting floating select ActionModes for web content

Add floating toolbar support for web content selections.

BUG=389153
Committed: https://crrev.com/98a1a3a17168e4b61e723dc973dad81c1190d78a
Cr-Commit-Position: refs/heads/master@{#345172}

[jdduke (slow), 2015-07-22 19:59:57]

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

[jdduke (slow), 2015-07-22 19:59:57]

aurimas@: Could you take a look at content/? The key changes are in
SelectActionMode and ContentViewCore. This is mostly restoring the old style of
creating the callback via the ContentViewClient, and polishing how we hide the
ActionMode.

[aurimas (slooooooooow), 2015-07-23 17:45:07]

I think it looks good, but we will not able able to land it yet due to
dependency of M sdk. When are you hoping to land it?

https://codereview.chromium.org/1242613002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/SelectActionMode.java
(right):

https://codereview.chromium.org/1242613002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/SelectActionMode.java:16:
@TargetApi(Build.VERSION_CODES.MNC)
This will probably not compile until we roll to Android M SDK.

[jdduke (slow), 2015-07-23 18:21:11]

On 2015/07/23 17:45:07, aurimas wrote:
> I think it looks good, but we will not able able to land it yet due to
> dependency of M sdk. When are you hoping to land it?

Exactly, none of this will compile. I was posting this to start review so that
we'll be able to land soon after we get the M SDK. We won't need it until then.

[jdduke (slow), 2015-08-20 22:10:47]

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

[jdduke (slow), 2015-08-20 22:10:47]

+sgurun for android_webview
+mariakhomenko for chrome/android

aurimas could you take a look at content/? This should compile and run now that
we've rolled.

[Maria, 2015-08-20 22:14:27]

On 2015/08/20 22:10:47, jdduke wrote:
> +sgurun for android_webview
> +mariakhomenko for chrome/android
> 
> aurimas could you take a look at content/? This should compile and run now
that
> we've rolled.

lgtm for chrome/android only (did not look at the rest)

[aurimas (slooooooooow), 2015-08-20 22:20:17]

https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:764:
return new ChromeWebActionModeCallback(context, actionHandler);
Why do we even need ChromeWebActionModeCallback? Can we simply use
WebActionModeCallback and move onCreateActionMode() to WebActionModeCallback as
it is only using ui/ layer?

https://codereview.chromium.org/1242613002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/WebActionMode.java
(right):

https://codereview.chromium.org/1242613002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:118:
hideTemporarily(300);
How did we pick this number? Can we pull this constant out to the top of the
class?

[jdduke (slow), 2015-08-20 23:13:14]

https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:764:
return new ChromeWebActionModeCallback(context, actionHandler);
On 2015/08/20 22:20:16, aurimas wrote:
> Why do we even need ChromeWebActionModeCallback? Can we simply use
> WebActionModeCallback and move onCreateActionMode() to WebActionModeCallback
as
> it is only using ui/ layer?

I believe the only reason we have it is to supply a custom title to the
ActionMode for tablets (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...).

We could instead have a method |getActionModeTitle()|? Or is there a better way
to embed the asset? Alternatively, I wonder what the harm is in moving the
string resource into content/ and just fetching the title directly? I don't
suppose you know why this is the way it is currently?

https://codereview.chromium.org/1242613002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/WebActionMode.java
(right):

https://codereview.chromium.org/1242613002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:118:
hideTemporarily(300);
On 2015/08/20 22:20:16, aurimas wrote:
> How did we pick this number? Can we pull this constant out to the top of the
> class?

It's somewhat arbitrary, 300 ms should ensure we get any trailing selection
updates from the renderer after the user stops dragging a handle. I put a note
and moved the constant up top.

[sgurun-gerrit only, 2015-08-20 23:15:26]

On 2015/08/20 22:20:17, aurimas wrote:
>
https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
> File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
> (right):
> 
>
https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
> chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:764:
> return new ChromeWebActionModeCallback(context, actionHandler);
> Why do we even need ChromeWebActionModeCallback? Can we simply use
> WebActionModeCallback and move onCreateActionMode() to WebActionModeCallback
as
> it is only using ui/ layer?
> 
>
https://codereview.chromium.org/1242613002/diff/100001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/WebActionMode.java
> (right):
> 
>
https://codereview.chromium.org/1242613002/diff/100001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:118:
> hideTemporarily(300);
> How did we pick this number? Can we pull this constant out to the top of the
> class?

lgtm

[sgurun-gerrit only, 2015-08-20 23:15:40]

https://codereview.chromium.org/1242613002/diff/120001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
(left):

https://codereview.chromium.org/1242613002/diff/120001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:410:
}
These changes should be merged to internal glue layer.

[jdduke (slow), 2015-08-20 23:22:41]

On 2015/08/20 23:15:40, sgurun wrote:
>
https://codereview.chromium.org/1242613002/diff/120001/android_webview/glue/j...
> File
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
> (left):
> 
>
https://codereview.chromium.org/1242613002/diff/120001/android_webview/glue/j...
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:410:
> }
> These changes should be merged to internal glue layer.

Yep, I'll actually be removing everything from the internal glue layer, then
I'll remove these last remnants. The three way should avoid any roll breakages.

[aurimas (slooooooooow), 2015-08-21 01:30:23]

You will not be able to land this until webview starts linking against Android M
system jar (see discussion on clank-webview@ mailing list.

https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1242613002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:764:
return new ChromeWebActionModeCallback(context, actionHandler);
On 2015/08/20 at 23:13:14, jdduke wrote:
> On 2015/08/20 22:20:16, aurimas wrote:
> > Why do we even need ChromeWebActionModeCallback? Can we simply use
> > WebActionModeCallback and move onCreateActionMode() to WebActionModeCallback
as
> > it is only using ui/ layer?
> 
> I believe the only reason we have it is to supply a custom title to the
ActionMode for tablets (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...).
> 
> We could instead have a method |getActionModeTitle()|? Or is there a better
way to embed the asset? Alternatively, I wonder what the harm is in moving the
string resource into content/ and just fetching the title directly? I don't
suppose you know why this is the way it is currently?

I think moves the string to content is fine and just have WebActionModeCallback
there.

[jdduke (slow), 2015-08-21 17:17:59]

On 2015/08/21 01:30:23, aurimas wrote:
> You will not be able to land this until webview starts linking against Android
M
> system jar (see discussion on clank-webview@ mailing list.

Noted, sounds like that is in flight.

> I think moves the string to content is fine and just have
WebActionModeCallback
> there.

Will do, I'm somewhat hesitant to move around strings since we're so close to
branch (I suppose I should reconsider landing this anyway, as M46 isn't in time
for M, but it will make us platform equivalent 6 weeks earlier... any
thoughts?). I can make the string change when I do the final cleanup pass here
to remove the lingering WebView bindings.

[aurimas (slooooooooow), 2015-08-21 18:13:12]

lgtm as long as you follow up with the ChromeWebAction... class merge after this
lands.

[jdduke (slow), 2015-08-24 20:23:03]

Patchset #11 (id:200001) has been deleted

[jdduke (slow), 2015-08-24 20:25:15]

Patchset #11 (id:220001) has been deleted

[jdduke (slow), 2015-08-24 20:27:29]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-08-24 20:27:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org, sgurun@chromium.org, aurimas@chromium.org
Link to the patchset: https://codereview.chromium.org/1242613002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-24 20:27:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1242613002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1242613002/240001

[commit-bot: I haz the power, 2015-08-24 20:48:20]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-08-24 20:48:59]

Patchset 11 (id:??) landed as
https://crrev.com/98a1a3a17168e4b61e723dc973dad81c1190d78a
Cr-Commit-Position: refs/heads/master@{#345172}