Progress bar color is based on toolbar color

Progress bar color is based on toolbar color

BUG=464377
Committed: https://crrev.com/a527a68340e73807a9c0e12338b4d9122f25a395
Cr-Commit-Position: refs/heads/master@{#391148}

[mdjones, 2016-04-11 23:09:18]

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

[mdjones, 2016-04-11 23:09:19]

ptal

[Yusuf, 2016-04-12 20:09:27]

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

https://codereview.chromium.org/1874953003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2010:
ColorUtils.getDarkenedColorForStatusBar(currentPrimaryColor);
the name of the utility is a bit confusing. Why are we using the darkened status
bar color for progress bar color? If they are the same thing, should we rename
the utility?

[mdjones, 2016-04-13 00:47:35]

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

https://codereview.chromium.org/1874953003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2010:
ColorUtils.getDarkenedColorForStatusBar(currentPrimaryColor);
On 2016/04/12 20:09:27, Yusuf wrote:
> the name of the utility is a bit confusing. Why are we using the darkened
status
> bar color for progress bar color? If they are the same thing, should we rename
> the utility?

They're tot necessarily the same thing. Added a separate function and constant.

[mdjones, 2016-04-21 17:44:00]

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

[mdjones, 2016-04-21 17:44:00]

+dfalcantara for ColorUtils

[gone, 2016-04-21 17:54:08]

lgtm

[mdjones, 2016-04-27 21:37:40]

Got a better spec from design. Please take another look.

I have a pretty naive approach to finding a contrasting color based on the
toolbar color. If this is too ugly, I can simply multiply the base lightness by
0.6 like the status bar is.

[Yusuf, 2016-04-28 06:53:55]

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:369:
if (!isUsingDefaultToolbarColor(getToolbarDataProvider().getPrimaryColor())) {
I would use getBackgroundColor on the toolbar rather than getPrimaryColor from
the DataProvider. That one only gets updated for theme color changes. That is
probably a nice area for a bit of a clean up when we get a chance.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:502:
private boolean isUsingDefaultToolbarColor(int color) {
This very much looks like a colorutils static call rather than a private here.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:326:
animationColor =
this would fit to the above line

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:333:
ColorUtils.getColorWithOverlay(color, foregroundWhite,
this would fit to the above line

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:337:
super.setForegroundColor(foregroundColor);
no need for super

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:338:
super.setBackgroundColor(
no need for super

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:341:
if (mAnimatingView != null) {
one line

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:355:
private int findDarkerColorWithMinContrast(int color, float minContrast) {
this is also a colorutils call

[mdjones, 2016-04-29 22:47:55]

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:369:
if (!isUsingDefaultToolbarColor(getToolbarDataProvider().getPrimaryColor())) {
On 2016/04/28 06:53:55, Yusuf wrote:
> I would use getBackgroundColor on the toolbar rather than getPrimaryColor from
> the DataProvider. That one only gets updated for theme color changes. That is
> probably a nice area for a bit of a clean up when we get a chance. 

Unless I assume the background Drawable is a ColorDrawable, it doesn't look like
I can get it this way.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:502:
private boolean isUsingDefaultToolbarColor(int color) {
On 2016/04/28 06:53:55, Yusuf wrote:
> This very much looks like a colorutils static call rather than a private here.

Done.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:326:
animationColor =
On 2016/04/28 06:53:55, Yusuf wrote:
> this would fit to the above line

Done.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:333:
ColorUtils.getColorWithOverlay(color, foregroundWhite,
On 2016/04/28 06:53:55, Yusuf wrote:
> this would fit to the above line

Done.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:337:
super.setForegroundColor(foregroundColor);
On 2016/04/28 06:53:55, Yusuf wrote:
> no need for super

Done.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:338:
super.setBackgroundColor(
On 2016/04/28 06:53:55, Yusuf wrote:
> no need for super

Done.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:341:
if (mAnimatingView != null) {
On 2016/04/28 06:53:55, Yusuf wrote:
> one line

Done.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:355:
private int findDarkerColorWithMinContrast(int color, float minContrast) {
On 2016/04/28 06:53:55, Yusuf wrote:
> this is also a colorutils call

Done.

[Yusuf, 2016-05-02 21:16:56]

One comment only. Once we get this, this is good to go as far as I am concerned.

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:369:
if (!isUsingDefaultToolbarColor(getToolbarDataProvider().getPrimaryColor())) {
On 2016/04/29 22:47:55, mdjones wrote:
> On 2016/04/28 06:53:55, Yusuf wrote:
> > I would use getBackgroundColor on the toolbar rather than getPrimaryColor
from
> > the DataProvider. That one only gets updated for theme color changes. That
is
> > probably a nice area for a bit of a clean up when we get a chance. 
> 
> Unless I assume the background Drawable is a ColorDrawable, it doesn't look
like
> I can get it this way.

You can! I mean you can add an override to getBackgroundDrawable to
ToolbarLayout or something like getBackgroundColor that returns an int.

The background drawable is a ColorDrawable at all times.

[mdjones, 2016-05-03 00:16:58]

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/1874953003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:369:
if (!isUsingDefaultToolbarColor(getToolbarDataProvider().getPrimaryColor())) {
On 2016/05/02 21:16:56, Yusuf wrote:
> On 2016/04/29 22:47:55, mdjones wrote:
> > On 2016/04/28 06:53:55, Yusuf wrote:
> > > I would use getBackgroundColor on the toolbar rather than getPrimaryColor
> from
> > > the DataProvider. That one only gets updated for theme color changes. That
> is
> > > probably a nice area for a bit of a clean up when we get a chance. 
> > 
> > Unless I assume the background Drawable is a ColorDrawable, it doesn't look
> like
> > I can get it this way.
> 
> You can! I mean you can add an override to getBackgroundDrawable to
> ToolbarLayout or something like getBackgroundColor that returns an int.
> 
> The background drawable is a ColorDrawable at all times.

Done.

[Yusuf, 2016-05-03 00:41:11]

lgtm

[gone, 2016-05-03 00:44:05]

Still lgtm.

[mdjones, 2016-05-03 00:54:01]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2016-05-03 00:54:26]

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

[commit-bot: I haz the power, 2016-05-03 01:36:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-03 01:38:02]

Description was changed from

==========
Progress bar color is based on toolbar color

BUG=464377
==========

to

==========
Progress bar color is based on toolbar color

BUG=464377

Committed: https://crrev.com/a527a68340e73807a9c0e12338b4d9122f25a395
Cr-Commit-Position: refs/heads/master@{#391148}
==========

[commit-bot: I haz the power, 2016-05-03 01:38:03]

Patchset 9 (id:??) landed as
https://crrev.com/a527a68340e73807a9c0e12338b4d9122f25a395
Cr-Commit-Position: refs/heads/master@{#391148}