[Download Home] Custom tab backgrounds

[Download Home] Custom tab backgrounds

* Background and toolbar of the Custom Tab should be
  off-black for images and black for all other media.

* Add an extra to the Intent that allows changing the
  background.

* Overrides ChromeActivity methods so that the colors
  can be set at the right times.

* Add a method to disable the toolbar shadow.

BUG=650455
Committed: https://crrev.com/73877006b0b2cda3b6906f376f852d58b0c2ba4b
Cr-Commit-Position: refs/heads/master@{#430780}

[gone, 2016-11-08 00:32:46]

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

[gone, 2016-11-08 00:32:47]

Matt for the Toolbar changes.

Ian for all the other things.

[mdjones, 2016-11-08 00:52:54]

toolbar* lgtm % suggestion

https://codereview.chromium.org/2484053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2484053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:935:
public void disableShadow() {
Think it is worth mentioning the assumption that a control container contains a
toolbar_shadow? If not maybe a null check for the view?

[gone, 2016-11-08 00:54:56]

https://codereview.chromium.org/2484053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2484053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:935:
public void disableShadow() {
On 2016/11/08 00:52:53, mdjones wrote:
> Think it is worth mentioning the assumption that a control container contains
a
> toolbar_shadow? If not maybe a null check for the view?

Added a null check.

[gone, 2016-11-08 01:32:58]

The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 01:33:40]

Dry run: 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, 2016-11-08 02:06:06]

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

[commit-bot: I haz the power, 2016-11-08 02:06:07]

Dry run: This issue passed the CQ dry run.

[Ian Wen, 2016-11-08 21:40:27]

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

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:244:
? new ColorDrawable(mIntentDataProvider.getInitialBackgroundColor())
Would you mind de-coupling background color customization from the media viewer?
If so this will greatly help that we later expose background color tweak as
public api.

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:919:
private void prepareMediaViewerTab(final Tab tab, int mediaViewerColor) {
I would rename this method to be updateTabBackground.

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

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:65:
"org.chromium.chrome.browser.customtabs.EXTRA_BACKGROUND_COLOR";
How about EXTRA_INITIAL_TAB_COLOR or EXTRA_INITIAL_BACKGROUND_COLOR? Adding
initial here will make it less confusing for others, since we have a toolbar
background extra as well.

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:163:
intent, EXTRA_BACKGROUND_COLOR, Color.BLACK);
I think we should either make the default to be transparent or white, if we are
to decouple background from media viewer.

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

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:938:
((ViewGroup) toolbarShadow.getParent()).removeView(toolbarShadow);
Nit: use UiUtil#removeViewFromParent instead?

[gone, 2016-11-08 23:17:16]

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

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:244:
? new ColorDrawable(mIntentDataProvider.getInitialBackgroundColor())
On 2016/11/08 21:40:27, Ian Wen wrote:
> Would you mind de-coupling background color customization from the media
viewer?
> If so this will greatly help that we later expose background color tweak as
> public api.

Done.

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:919:
private void prepareMediaViewerTab(final Tab tab, int mediaViewerColor) {
On 2016/11/08 21:40:27, Ian Wen wrote:
> I would rename this method to be updateTabBackground.

Renamed prepareTabBackground because this doesn't really update anything.

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

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:65:
"org.chromium.chrome.browser.customtabs.EXTRA_BACKGROUND_COLOR";
On 2016/11/08 21:40:27, Ian Wen wrote:
> How about EXTRA_INITIAL_TAB_COLOR or EXTRA_INITIAL_BACKGROUND_COLOR? Adding
> initial here will make it less confusing for others, since we have a toolbar
> background extra as well.

Done.

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:163:
intent, EXTRA_BACKGROUND_COLOR, Color.BLACK);
On 2016/11/08 21:40:27, Ian Wen wrote:
> I think we should either make the default to be transparent or white, if we
are
> to decouple background from media viewer.

Using Color.TRANSPARENT as a signal to indicate that the background shouldn't be
forced now.

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

https://codereview.chromium.org/2484053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:938:
((ViewGroup) toolbarShadow.getParent()).removeView(toolbarShadow);
On 2016/11/08 21:40:27, Ian Wen wrote:
> Nit: use UiUtil#removeViewFromParent instead?

Done.

[gone, 2016-11-08 23:19:16]

The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 23:20:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Wen, 2016-11-08 23:28:52]

lgtm

[gone, 2016-11-08 23:31:37]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-11-08 23:31:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/2484053002/#ps60001
(title: "Rebasing")

[commit-bot: I haz the power, 2016-11-08 23:33:02]

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, 2016-11-09 00:09:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-09 00:12:33]

Description was changed from

==========
[Download Home] Custom tab backgrounds

* Background and toolbar of the Custom Tab should be
  off-black for images and black for all other media.

* Add an extra to the Intent that allows changing the
  background.

* Overrides ChromeActivity methods so that the colors
  can be set at the right times.

* Add a method to disable the toolbar shadow.

BUG=650455
==========

to

==========
[Download Home] Custom tab backgrounds

* Background and toolbar of the Custom Tab should be
  off-black for images and black for all other media.

* Add an extra to the Intent that allows changing the
  background.

* Overrides ChromeActivity methods so that the colors
  can be set at the right times.

* Add a method to disable the toolbar shadow.

BUG=650455

Committed: https://crrev.com/73877006b0b2cda3b6906f376f852d58b0c2ba4b
Cr-Commit-Position: refs/heads/master@{#430780}
==========

[commit-bot: I haz the power, 2016-11-09 00:12:34]

Patchset 4 (id:??) landed as
https://crrev.com/73877006b0b2cda3b6906f376f852d58b0c2ba4b
Cr-Commit-Position: refs/heads/master@{#430780}