[Android] Implement copy and open in background of Browser Actions

[Android] Implement copy and open in background of Browser Actions

This CL implements two simple predefined options in Browser Action
context menu including:
1. Copy link: Copies the given url and displays a toast message for it.
It uses Android ClipBoaard api instead Chrome custom api since native
library might not be loaded when option is selected.
2. Open in incognito tab: modifies the ChromeTabbedActivity to allow
incognito tab created with FROM_EXTERNAL_APP launch type. This allows
back button closes the tab and exits Chrome.

BUG=719080
Review-Url: https://codereview.chromium.org/2872353004
Cr-Commit-Position: refs/heads/master@{#473256}
Committed: https://chromium.googlesource.com/chromium/src/+/7efcbacb378eb77a93738a5e60479c8085a5564c

[ltian, 2017-05-11 18:34:45]

ltian@chromium.org changed reviewers:
+ tedchoc@chromium.org

[ltian, 2017-05-11 18:34:45]

Can you take a look of the changes in this CL?

Thanks!

[Ted C, 2017-05-11 19:04:49]

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1103:
if (IntentHandler.wasIntentSenderChrome(intent)) {
We only allow chrome to open incognito tabs, so this is actually changing the
behavior for all of them.

I don't think we want to do this as it would cause chrome to exit in cases where
it didn't previously.

We should probably pass some other intent extra that singles this is from a
browser action popup than relying on whether it came from chrome as a signal.

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1731: Copied to clipboard
in the bug, can you link to any discussions where this was decided upon?

We specifically have not shown a toast when a user copies within chrome:
https://bugs.chromium.org/p/chromium/issues/detail?id=367021

So I feel we should be consistent between browser actions and chrome (even if
that means deviating from the past decisions).

[ltian, 2017-05-11 22:10:58]

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1103:
if (IntentHandler.wasIntentSenderChrome(intent)) {
On 2017/05/11 19:04:49, Ted C wrote:
> We only allow chrome to open incognito tabs, so this is actually changing the
> behavior for all of them.
> 
> I don't think we want to do this as it would cause chrome to exit in cases
where
> it didn't previously.
> 
> We should probably pass some other intent extra that singles this is from a
> browser action popup than relying on whether it came from chrome as a signal.

Done.

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2872353004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1731: Copied to clipboard
On 2017/05/11 19:04:49, Ted C wrote:
> in the bug, can you link to any discussions where this was decided upon?
> 
> We specifically have not shown a toast when a user copies within chrome:
> https://bugs.chromium.org/p/chromium/issues/detail?id=367021
> 
> So I feel we should be consistent between browser actions and chrome (even if
> that means deviating from the past decisions).

Remove showing the toast from this CL. If later we want to have the toast
message for copy cross Chrome, I will have a separate CL for it.

[Ted C, 2017-05-12 00:07:44]

tedchoc@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Ted C, 2017-05-12 00:07:45]

+mariakhomenko to join in my bikesheding/naming struggle

https://codereview.chromium.org/2872353004/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
(right):

https://codereview.chromium.org/2872353004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:1023:
public static void setOpenIncognitoFromExternal(Intent intent) {
Bikesheding on names.  This is a bit confusing as we really don't want to allow
external apps to open incognito tabs.

I wonder if we should just allow intents within Chrome to override the
TabLaunchType.  I think enums can be put into an intent so we could maybe just
have setTabLaunchTypeOverride or something where you pass external app for your
usecase.

[ltian, 2017-05-12 19:09:19]

https://codereview.chromium.org/2872353004/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
(right):

https://codereview.chromium.org/2872353004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:1023:
public static void setOpenIncognitoFromExternal(Intent intent) {
On 2017/05/12 00:07:45, Ted C wrote:
> Bikesheding on names.  This is a bit confusing as we really don't want to
allow
> external apps to open incognito tabs.
> 
> I wonder if we should just allow intents within Chrome to override the
> TabLaunchType.  I think enums can be put into an intent so we could maybe just
> have setTabLaunchTypeOverride or something where you pass external app for
your
> usecase.

Done.

[Maria, 2017-05-12 21:18:17]

lgtm

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1104:
if (IntentHandler.wasIntentSenderChrome(intent) && launchType != null) {
do we need to check that Chrome is the intent sender? I think we block all
non-chrome incognito actions in IntentHandler.shouldHandleIntent()

[Ted C, 2017-05-12 23:04:55]

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1104:
if (IntentHandler.wasIntentSenderChrome(intent) && launchType != null) {
On 2017/05/12 21:18:17, Maria wrote:
> do we need to check that Chrome is the intent sender? I think we block all
> non-chrome incognito actions in IntentHandler.shouldHandleIntent()

Yeah, that is already handled further upstream.  We don't need to have
wasIntentSenderChrome here.  The fact that we are attempting to deserialize
something that is only in our classloader anyway probably makes this only
feasible by us to pass a different launchtype

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
(right):

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:155:
public static final String EXTRA_TAB_LAUNCH_TYPE =
I think if you keep this public, you could just have the call sites use this
instead of adding the static method (if you want to keep the static methods, you
can make this private then)

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
(right):

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java:271:
public static Serializable safeGetSerializableExtra(Intent intent, String name)
{
I think we could make this a bit nicer and do something like this:

    @SuppressWarnings("unchecked")
    private static <T extends Serializable> T safeGetSerializableExtra(Intent
intent, String name)) {
        try {
            return (T) intent.getSerializableExtra(name);
        } catch (ClassCastException ex) {
            Log.e("Invalid class for Serializable: " + name, ex);
            return null;
        } catch (Throwable t) {
            Log.e(TAG, "getSerializableExtra failed on intent " + intent);
            return null;
        }
    }

[ltian, 2017-05-15 21:48:05]

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1104:
if (IntentHandler.wasIntentSenderChrome(intent) && launchType != null) {
On 2017/05/12 23:04:55, Ted C wrote:
> On 2017/05/12 21:18:17, Maria wrote:
> > do we need to check that Chrome is the intent sender? I think we block all
> > non-chrome incognito actions in IntentHandler.shouldHandleIntent()
> 
> Yeah, that is already handled further upstream.  We don't need to have
> wasIntentSenderChrome here.  The fact that we are attempting to deserialize
> something that is only in our classloader anyway probably makes this only
> feasible by us to pass a different launchtype

Done.

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
(right):

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:155:
public static final String EXTRA_TAB_LAUNCH_TYPE =
On 2017/05/12 23:04:55, Ted C wrote:
> I think if you keep this public, you could just have the call sites use this
> instead of adding the static method (if you want to keep the static methods,
you
> can make this private then)

Done.

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
(right):

https://codereview.chromium.org/2872353004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java:271:
public static Serializable safeGetSerializableExtra(Intent intent, String name)
{
On 2017/05/12 23:04:55, Ted C wrote:
> I think we could make this a bit nicer and do something like this:
> 
>     @SuppressWarnings("unchecked")
>     private static <T extends Serializable> T safeGetSerializableExtra(Intent
> intent, String name)) {
>         try {
>             return (T) intent.getSerializableExtra(name);
>         } catch (ClassCastException ex) {
>             Log.e("Invalid class for Serializable: " + name, ex);
>             return null;
>         } catch (Throwable t) {
>             Log.e(TAG, "getSerializableExtra failed on intent " + intent);
>             return null;
>         }
>     }

This code looks more descent, thanks!

[Ted C, 2017-05-16 18:33:42]

lgtm

[ltian, 2017-05-19 17:44:02]

The CQ bit was checked by ltian@chromium.org

[ltian, 2017-05-19 17:44:03]

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

[commit-bot: I haz the power, 2017-05-19 17:45:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 18:45:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495215842705090,
"parent_rev": "7a400ee2cb54a4be1a7b4dc6a3d407693d3699a4", "commit_rev":
"7efcbacb378eb77a93738a5e60479c8085a5564c"}

[commit-bot: I haz the power, 2017-05-19 18:45:16]

Description was changed from

==========
[Android] Implement copy and open in background of Browser Actions

This CL implements two simple predefined options in Browser Action
context menu including:
1. Copy link: Copies the given url and displays a toast message for it.
It uses Android ClipBoaard api instead Chrome custom api since native
library might not be loaded when option is selected.
2. Open in incognito tab: modifies the ChromeTabbedActivity to allow
incognito tab created with FROM_EXTERNAL_APP launch type. This allows
back button closes the tab and exits Chrome.

BUG=719080
==========

to

==========
[Android] Implement copy and open in background of Browser Actions

This CL implements two simple predefined options in Browser Action
context menu including:
1. Copy link: Copies the given url and displays a toast message for it.
It uses Android ClipBoaard api instead Chrome custom api since native
library might not be loaded when option is selected.
2. Open in incognito tab: modifies the ChromeTabbedActivity to allow
incognito tab created with FROM_EXTERNAL_APP launch type. This allows
back button closes the tab and exits Chrome.

BUG=719080

Review-Url: https://codereview.chromium.org/2872353004
Cr-Commit-Position: refs/heads/master@{#473256}
Committed:
https://chromium.googlesource.com/chromium/src/+/7efcbacb378eb77a93738a5e6047...
==========

[commit-bot: I haz the power, 2017-05-19 18:45:17]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7efcbacb378eb77a93738a5e6047...