Let Custom Tabs Use Light Progressbar for Dark Toolbar

Let Custom Tabs Use Light Progressbar for Dark Toolbar

Users requested us to make our progressbar more friendly for darker
toolbar colors. This CL implements a white progressbar with a different
background color, depending on the toolbar color.

BUG=531622
Committed: https://crrev.com/18949d2a87fd188ba0df9d5d2764a9458eab1628
Cr-Commit-Position: refs/heads/master@{#350014}

[Ian Wen, 2015-09-18 01:32:59]

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

[Ian Wen, 2015-09-18 01:32:59]

yusufo@chromium.org: Please review changes in progressbar
tedchoc@chromium.org: Please review changes in customtabs

[Yusuf, 2015-09-18 20:48:38]

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

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:64:
private ImageButton mCloseButton;
blank line here.

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:301:
getProgressBar().setForegroundColor(Color.WHITE);
There should be a res id for this.

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

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ColorUtils.java:56: /**
@param

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:81:
mForegroundDrawable = new ColorDrawable(mProgressBarColor);
I would pick one of these fields (probably mForegroundDrawable) and only keep
that one. Other then within setForegroundColor it is not immediately apparent
from names that these two have to be in sync. Doing
mForegroundDrawable.getColor() is easy enough.

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:185:
* Sets the color for the foreground (i.e. the moving part) of the progress bar.
@param

[Ian Wen, 2015-09-18 21:46:04]

Thanks for the comments! All addressed.

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

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:64:
private ImageButton mCloseButton;
On 2015/09/18 20:48:38, Yusuf wrote:
> blank line here.

Done.

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:301:
getProgressBar().setForegroundColor(Color.WHITE);
On 2015/09/18 20:48:38, Yusuf wrote:
> There should be a res id for this.

Done.

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

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ColorUtils.java:56: /**
On 2015/09/18 20:48:38, Yusuf wrote:
> @param

Done.

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:81:
mForegroundDrawable = new ColorDrawable(mProgressBarColor);
On 2015/09/18 20:48:38, Yusuf wrote:
> I would pick one of these fields (probably mForegroundDrawable) and only keep
> that one. Other then within setForegroundColor it is not immediately apparent
> from names that these two have to be in sync. Doing
> mForegroundDrawable.getColor() is easy enough.

Had the same idea myself but was afraid of breaking the symmetry. Done.

https://codereview.chromium.org/1352133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:185:
* Sets the color for the foreground (i.e. the moving part) of the progress bar.
On 2015/09/18 20:48:38, Yusuf wrote:
> @param

Done.

[Yusuf, 2015-09-18 21:56:07]

lgtm other than the nit question.

Off to Ted.

https://codereview.chromium.org/1352133002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1352133002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:40:
private ColorDrawable mForegroundDrawable;
is init called multiple times? Can these be finals?

[Ian Wen, 2015-09-21 17:45:43]

https://codereview.chromium.org/1352133002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java
(right):

https://codereview.chromium.org/1352133002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ClipDrawableProgressBar.java:40:
private ColorDrawable mForegroundDrawable;
On 2015/09/18 21:56:06, Yusuf wrote:
> is init called multiple times? Can these be finals?

init() is a private function that is solely called by the constructor. I think
it's safe to remove it. Done.

[Ted C, 2015-09-21 20:00:39]

lgtm

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

https://codereview.chromium.org/1352133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:307:
progressBarBackgroundColorResource));
ah...the visuals can never change for a custom tab right?  just making sure we
don't need to set the foreground color here to be the corollary to the one
above.

[Ian Wen, 2015-09-21 20:02:17]

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

[commit-bot: I haz the power, 2015-09-21 20:03:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1352133002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1352133002/80001

[commit-bot: I haz the power, 2015-09-21 20:44:14]

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

[commit-bot: I haz the power, 2015-09-21 20:44:15]

Dry run: This issue passed the CQ dry run.

[Ian Wen, 2015-09-21 20:44:47]

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

https://codereview.chromium.org/1352133002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:307:
progressBarBackgroundColorResource));
On 2015/09/21 20:00:39, Ted C wrote:
> ah...the visuals can never change for a custom tab right?  just making sure we
> don't need to set the foreground color here to be the corollary to the one
> above.

Yes, the background color of the toolbar won't change after initialization. The
action button might be changed by service APIs, yet it won't affect the
progressbar.

[Ian Wen, 2015-09-21 20:44:51]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-09-21 20:44:51]

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/1352133002/#ps80001
(title: "Removed init() in ClipDrawableProgressbar")

[commit-bot: I haz the power, 2015-09-21 20:46:09]

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

[commit-bot: I haz the power, 2015-09-21 20:59:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-21 21:00:37]

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