Herb: Show an icon allowing the user to open a tab in Chrome

Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539
Committed: https://crrev.com/3d5e851ca2d3baaaf570af6dc93872cf73d2772f
Cr-Commit-Position: refs/heads/master@{#374797}

[gone, 2016-02-09 01:48:25]

dfalcantara@chromium.org changed reviewers:
+ ianwen@chromium.org, tedchoc@chromium.org

[gone, 2016-02-09 01:48:26]

+Ian for the Custom Tab changes
+Ted to make sure what I'm doing makes sense in general

[gone, 2016-02-09 18:56:36]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

BUG=582539
==========

[gone, 2016-02-09 19:40:09]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

* Redefines back button behavior when for when the return
  button is visible.

BUG=582539
==========

[Ian Wen, 2016-02-09 20:30:49]

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Only one png file for xxxhdpi? What about hdpi/xhdpi/etc?

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:361:
getCustomButtonOnToolbar().getPendingIntent().getCreatorPackage())) {
Maybe we should move the package comparison to the constructor above, and make
mFinishActivityOnAction the only signal to use?  Traditionally this class deals
most of the things in constructor and other methods don't have much code.

If we move the package name comparison to the constructor, then we need to use
Intent#getPackageName, not the pendingintent then.

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:90:
public static final String EXTRA_IS_ALLOWED_TO_RETURN_TO_PARENT =
So if this extra is set to false, when the user hit back key, the client app
will be shown?

[gone, 2016-02-09 21:09:52]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

* Redefines back button behavior when for when the return
  button is visible.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

* Redefines back button behavior when for when the return
  button is visible.  There's still a lingering issue with
  the back button for tabs that are created from Custom Tabs
  and opened in Chrome, where the tab will close but
  shouldn't according to the spec.

BUG=582539
==========

[gone, 2016-02-09 21:09:59]

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

https://codereview.chromium.org/1683533002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/02/09 20:30:49, Ian Wen wrote:
> Only one png file for xxxhdpi? What about hdpi/xhdpi/etc?

It's a placeholder.  There's no real asset yet.

https://codereview.chromium.org/1683533002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1683533002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:361:
getCustomButtonOnToolbar().getPendingIntent().getCreatorPackage())) {
On 2016/02/09 20:30:49, Ian Wen wrote:
> Maybe we should move the package comparison to the constructor above, and make
> mFinishActivityOnAction the only signal to use?  Traditionally this class
deals
> most of the things in constructor and other methods don't have much code.
> 
> If we move the package name comparison to the constructor, then we need to use
> Intent#getPackageName, not the pendingintent then.

Are you sure about this?  This means that any Intent fired to open a custom tab
that explicitly targets the Chrome package will get close privileges.

[gone, 2016-02-09 21:20:46]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

* Redefines back button behavior when for when the return
  button is visible.  There's still a lingering issue with
  the back button for tabs that are created from Custom Tabs
  and opened in Chrome, where the tab will close but
  shouldn't according to the spec.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

* Redefines back button behavior to never close a tab when an
  Herb flavor is active.

BUG=582539
==========

[gone, 2016-02-09 21:21:32]

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:90:
public static final String EXTRA_IS_ALLOWED_TO_RETURN_TO_PARENT =
On 2016/02/09 20:30:49, Ian Wen wrote:
> So if this extra is set to false, when the user hit back key, the client app
> will be shown?

Changed the behavior according to Chris' comment on the doc.  This should just
be used for determining whether the X is visible now.

[gone, 2016-02-09 21:29:04]

Hmm now I'm concerned about the catch all back button case being wrong for child
tabs.  Can you guys check if the rest of it makes sense?

[Ian Wen, 2016-02-10 01:33:31]

lgtm with nits.

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:360:
getCustomButtonOnToolbar().getPendingIntent().getCreatorPackage())) {
getCreatorPackage() is API 17, please guard it with API version check. (you can
also create a private package checker method)

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:361:
Log.e(TAG, "PendingIntent was created by Chrome.  Finishing Activity.");
Remove one space before "Finishing".

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:88:
* Whether or not the toolbar should indicate that a tab was spawned by another
Activity.
s/should indicate/indicates ?

[gone, 2016-02-10 18:47:58]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds a temporary icon for the "open in chrome" action.

* Redefines back button behavior to never close a tab when an
  Herb flavor is active.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539
==========

[gone, 2016-02-10 18:48:33]

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:360:
getCustomButtonOnToolbar().getPendingIntent().getCreatorPackage())) {
On 2016/02/10 01:33:31, Ian Wen wrote:
> getCreatorPackage() is API 17, please guard it with API version check. (you
can
> also create a private package checker method)

Switched to using ApiCompatibilityUtils

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:361:
Log.e(TAG, "PendingIntent was created by Chrome.  Finishing Activity.");
On 2016/02/10 01:33:31, Ian Wen wrote:
> Remove one space before "Finishing".

Done.

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:88:
* Whether or not the toolbar should indicate that a tab was spawned by another
Activity.
On 2016/02/10 01:33:31, Ian Wen wrote:
> s/should indicate/indicates ?

I think this makes sense in this case.  The toolbar can opt to not display this
information.

[Ted C, 2016-02-10 18:54:50]

lgtm

[gone, 2016-02-10 18:56:29]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-02-10 18:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1683533002/#ps120001 (title: "Removed
return, updated icons")

[commit-bot: I haz the power, 2016-02-10 18:58:11]

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

[gone, 2016-02-10 19:14:41]

The CQ bit was unchecked by dfalcantara@chromium.org

[gone, 2016-02-10 19:15:13]

Redoing this slightly so that the same pathway is used (as much as possible).

[gone, 2016-02-10 21:51:40]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

* If back is hit while Herb is active, don't close any tabs
  opened by external apps.  This is prone to change on the
  whims of the committee.

BUG=582539
==========

[gone, 2016-02-10 21:52:25]

Er, PTAL again.  I've rerouted how URLs are opened in the browser so that the
correct URL is used.  I can't think of a cleaner way to do this, ATM.

[Ian Wen, 2016-02-10 22:08:40]

Looks good again. The only issue is that we should still handle intent parsing
in intent data provider, not the activity.

https://chromiumcodereview.appspot.com/1683533002/diff/140001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:77:
"org.chromium.chrome.browser.customtabs.ALLOW_OPEN_IN_BROWSER";
Move this extra to CustomTabsIntentDataProvider, which is the only place that
CCT parses intent data.

Also maybe we should rename this extra to "finish after action button click" or
"finish after open in browser"? Opening in browser is always allowed in CCT and
the name is slightly confounding.

https://chromiumcodereview.appspot.com/1683533002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:188:
mIsAllowedToOpenInBrowser =
!TextUtils.isEmpty(ChromePreferenceManager.getHerbFlavor())
mIsAllowedToOpenInBrowser() should still be initialized in
CustomTabIntentDataProvider. You can create a method like
isAllowedToOpenInBrowser().

[gone, 2016-02-10 22:28:24]

Alrighty, comments addressed again.

https://chromiumcodereview.appspot.com/1683533002/diff/140001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1683533002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:77:
"org.chromium.chrome.browser.customtabs.ALLOW_OPEN_IN_BROWSER";
On 2016/02/10 22:08:40, Ian Wen wrote:
> Move this extra to CustomTabsIntentDataProvider, which is the only place that
> CCT parses intent data.
> 
> Also maybe we should rename this extra to "finish after action button click"
or
> "finish after open in browser"? Opening in browser is always allowed in CCT
and
> the name is slightly confounding.

Went with FINISH_AFTER_OPENING_IN_BROWSER.  We'll probably need a better way to
have Chrome-specific actions triggered by Custom Tabs if this prototype sticks.

https://chromiumcodereview.appspot.com/1683533002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:188:
mIsAllowedToOpenInBrowser =
!TextUtils.isEmpty(ChromePreferenceManager.getHerbFlavor())
On 2016/02/10 22:08:40, Ian Wen wrote:
> mIsAllowedToOpenInBrowser() should still be initialized in
> CustomTabIntentDataProvider. You can create a method like
> isAllowedToOpenInBrowser().

Done.

[Ian Wen, 2016-02-10 22:41:41]

Still lgtm. :)

[gone, 2016-02-10 23:09:41]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

* If back is hit while Herb is active, don't close any tabs
  opened by external apps.  This is prone to change on the
  whims of the committee.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539
==========

[gone, 2016-02-10 23:09:52]

Ripped out the back button behavior again.

[Ted C, 2016-02-10 23:24:48]

lgtm

[gone, 2016-02-10 23:25:00]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-02-10 23:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1683533002/#ps180001 (title: "Ripping out
back behavior yet again because people can't decide things")

[commit-bot: I haz the power, 2016-02-10 23:25:37]

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

[commit-bot: I haz the power, 2016-02-11 00:11:40]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539
==========

[commit-bot: I haz the power, 2016-02-11 00:11:40]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-02-16 22:33:00]

Description was changed from

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539
==========

to

==========
Herb: Show an icon allowing the user to open a tab in Chrome

* Adds an icon to CCTs opened by Chrome that allows the user
  to open the URL in the full Chrome browser.

* Allows CustomTabActivity to be finished() when it fires a
  PendingIntent that was created by Chrome itself.

* Adds an icon for the "open in chrome" action.

BUG=582539

Committed: https://crrev.com/3d5e851ca2d3baaaf570af6dc93872cf73d2772f
Cr-Commit-Position: refs/heads/master@{#374797}
==========

[commit-bot: I haz the power, 2016-02-16 22:33:01]

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