Replace return button with close button on custom tab toolbar

Replace return button with close button on custom tab toolbar

Previously a chevron-like left arrow return key is placed on the toolbar
in custom tabs, which may confuse users with the software return key.
Since the actual behavior of this button is to "close" the tab, a more
accurate icon is favored here.

BUG=495145
Committed: https://crrev.com/19a0c5ac2d00b18416ab9d0892afe6923c0ac035
Cr-Commit-Position: refs/heads/master@{#336838}

[Ian Wen, 2015-06-24 20:38:06]

ianwen@chromium.org changed reviewers:
+ yusufo@chromium.org

[Yusuf, 2015-06-24 21:23:36]

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/custom_tabs_toolbar.xml (right):

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/custom_tabs_toolbar.xml:11:
android:src="@drawable/btn_close" />
in the bug it mentions we would like to give a choice here rather than
defaulting to x only. Should we not be adding an extra for this and keeping the
chevron_left image?

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:123:
public void setCustomTabReturnClickHandler(OnClickListener listener) {
we should rename this as well if we are renaming things to close.

[Ian Wen, 2015-06-29 22:07:24]

ptal. :)

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/custom_tabs_toolbar.xml (right):

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/custom_tabs_toolbar.xml:11:
android:src="@drawable/btn_close" />
On 2015/06/24 21:23:36, Yusuf wrote:
> in the bug it mentions we would like to give a choice here rather than
> defaulting to x only. Should we not be adding an extra for this and keeping
the
> chevron_left image?

Added an intent extra in the next patch.

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1203223003/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:123:
public void setCustomTabReturnClickHandler(OnClickListener listener) {
On 2015/06/24 21:23:36, Yusuf wrote:
> we should rename this as well if we are renaming things to close.

Done.

[Yusuf, 2015-06-29 22:20:01]

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:140:
int closeIconRes = 0;
Move all this logic to IntentDataProvider and make the activity get a resource
id.

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

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:196:
mCloseButtonStyle = IntentUtils.safeGetIntExtra(intent,
add a default state here. Cross is the default one right now.

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/Toolbar.java:68:
void setCustomTabCloseClickHandler(OnClickListener listener);
lets remove this from the interface as well since we only need this to be called
through the manager. Make this a public call on ToolbarLayout only.

[Ian Wen, 2015-06-29 22:59:51]

All done. Thanks!

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:140:
int closeIconRes = 0;
On 2015/06/29 22:20:01, Yusuf wrote:
> Move all this logic to IntentDataProvider and make the activity get a resource
> id.

Done.

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

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:196:
mCloseButtonStyle = IntentUtils.safeGetIntExtra(intent,
On 2015/06/29 22:20:01, Yusuf wrote:
> add a default state here. Cross is the default one right now.

Default style is set in line 197, isn't it?

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1203223003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/Toolbar.java:68:
void setCustomTabCloseClickHandler(OnClickListener listener);
On 2015/06/29 22:20:01, Yusuf wrote:
> lets remove this from the interface as well since we only need this to be
called
> through the manager. Make this a public call on ToolbarLayout only.

Done.

[Yusuf, 2015-06-29 23:05:59]

lgtm after renaming nits

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:140:

extra line

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

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:204:
mCloseButtonRes = R.drawable.btn_close;
mCloseButtonResId

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:253:
public int getCloseButtonIconRes() {
getCloseButtonIconResId

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:124:
public void setCloseButtonIcon(int iconRes) {
setCloseButtonImageResource

[Ian Wen, 2015-06-29 23:56:00]

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:140:

On 2015/06/29 23:05:58, Yusuf wrote:
> extra line

Done.

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

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:204:
mCloseButtonRes = R.drawable.btn_close;
On 2015/06/29 23:05:58, Yusuf wrote:
> mCloseButtonResId

Done.

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:253:
public int getCloseButtonIconRes() {
On 2015/06/29 23:05:58, Yusuf wrote:
> getCloseButtonIconResId

Done.

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1203223003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:124:
public void setCloseButtonIcon(int iconRes) {
On 2015/06/29 23:05:58, Yusuf wrote:
> setCloseButtonImageResource

Done.

[Ian Wen, 2015-06-29 23:56:07]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-06-29 23:56:08]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/1203223003/#ps60001
(title: "renaming")

[commit-bot: I haz the power, 2015-06-29 23:57:49]

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

[commit-bot: I haz the power, 2015-06-30 00:01:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-30 00:01:38]

Exceeded global retry quota

[Ian Wen, 2015-06-30 18:20:33]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-06-30 18:20:34]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/1203223003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-30 18:20:53]

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

[commit-bot: I haz the power, 2015-06-30 19:09:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-30 19:10:01]

Patchset 5 (id:??) landed as
https://crrev.com/19a0c5ac2d00b18416ab9d0892afe6923c0ac035
Cr-Commit-Position: refs/heads/master@{#336838}