webapps: utilize manifest theme colors

webapps: utilize theme color specified in manifest

Currently the manifest spec allows a theme color to be specified. However, we do not parse nor use this currently.

Add the code for parsing the theme color attribute using a newly exposed CSS color parsing API from Blink and then utilize the theme color by setting the recents color.

Note that each page is able to override the manifest color through the use of a meta tag which is currently supported.

BUG=510402
Committed: https://crrev.com/2f5becad9f3833633845c94db12a3a043be76e31
Cr-Commit-Position: refs/heads/master@{#343002}

[Lalit Maganti, 2015-07-15 12:49:32]

lalitm@google.com changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-07-15 14:43:07]

Looks generally good!

A few comments from a first look:
- could you split the CL in two: one part for adding theme_color to the Manifest
and one part for using it in Chrome Android webapps.
- could you use constants for -1 like ScreenOrientationValues.DEFAULT or
UNKWNON_SOURCE do?
- could you add theme_color after the current list of webapps info. It's not
clear why it is between title and orientation. For coding style, I think it
should be appended to the list unless the list is sorted in some ways.

Thanks :)

[Lalit Maganti, 2015-07-15 14:44:30]

On 2015/07/15 at 14:43:07, mlamouri wrote:
> Looks generally good!
> 
> A few comments from a first look:
> - could you split the CL in two: one part for adding theme_color to the
Manifest and one part for using it in Chrome Android webapps.
> - could you use constants for -1 like ScreenOrientationValues.DEFAULT or
UNKWNON_SOURCE do?
> - could you add theme_color after the current list of webapps info. It's not
clear why it is between title and orientation. For coding style, I think it
should be appended to the list unless the list is sorted in some ways.
> 
> Thanks :)

On your points:
1. Will do.
2. Yup sounds good.
3. I followed roughly the order in the manifest. I can put it in the end if
that's what is usually done.

[Lalit Maganti, 2015-07-15 16:08:19]

Hopefully this is good to go too.

[mlamouri (slow - plz ping), 2015-07-17 13:28:29]

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:40:
public static final long THEME_COLOR_INVALID = -1;
I guess this should be renamed to match the C++ constant.

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:776:
ShortcutHelper.EXTRA_THEME_COLOR, -1);
Don't use -1, use a constant instead.

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:266:
if (mBrandColor == null && mWebappInfo.themeColor() != -1) {
ditto

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:124:
mThemeColor = newInfo.mThemeColor;
There might be one too many mThemeColor here ;)

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:88:
assertEquals(info.themeColor(), -1);
Use ShortcutHelper.THEME_COLOR_INVALID

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:123:
assertEquals(bundle.getLong(ShortcutHelper.EXTRA_THEME_COLOR, -1), themeColor);
use ShortcutHelper.THEME_COLOR_INVALID

https://codereview.chromium.org/1234653004/diff/110001/chrome/browser/android...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/browser/android...
chrome/browser/android/shortcut_data_fetcher.cc:77: shortcut_info_.theme_color =
-1;
Please use the content::Manifest constant.

https://codereview.chromium.org/1234653004/diff/110001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:35: if (manifest.theme_color != -1)
ditto

[Lalit Maganti, 2015-07-17 14:34:15]

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:40:
public static final long THEME_COLOR_INVALID = -1;
On 2015/07/17 at 13:28:29, Mounir Lamouri wrote:
> I guess this should be renamed to match the C++ constant.

Done.

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:776:
ShortcutHelper.EXTRA_THEME_COLOR, -1);
On 2015/07/17 at 13:28:29, Mounir Lamouri wrote:
> Don't use -1, use a constant instead.

Done.

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:266:
if (mBrandColor == null && mWebappInfo.themeColor() != -1) {
On 2015/07/17 at 13:28:29, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:124:
mThemeColor = newInfo.mThemeColor;
On 2015/07/17 at 13:28:29, Mounir Lamouri wrote:
> There might be one too many mThemeColor here ;)

Copied instead of moved :)

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(right):

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:88:
assertEquals(info.themeColor(), -1);
On 2015/07/17 at 13:28:29, Mounir Lamouri wrote:
> Use ShortcutHelper.THEME_COLOR_INVALID

Done.

https://codereview.chromium.org/1234653004/diff/110001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:123:
assertEquals(bundle.getLong(ShortcutHelper.EXTRA_THEME_COLOR, -1), themeColor);
On 2015/07/17 at 13:28:29, Mounir Lamouri wrote:
> use ShortcutHelper.THEME_COLOR_INVALID

Done.

[mlamouri (slow - plz ping), 2015-07-20 13:03:31]

Looks good!

In addition of the comments below, I think you need to update
chrome/browser/webapps/FullScreenActivityTab.java

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:39: //
This value is equal to kInvalidOrMissingThemeColor in the C++ Manifest struct.
nit: content::Manifest

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:269:
}
Not related to your change but I'm surprised to see that we don't update the
theme color if |!isWebappDomain()|. Does that mean that we would keep the
Manifest's theme colour if we end up navigating to a different domain that
specifies its own theme colour?

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:127:
mThemeColor = newInfo.mThemeColor;
You still have one too many mThemeColor here.

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
chrome/browser/android/shortcut_data_fetcher.cc:77: shortcut_info_.theme_color =
content::Manifest::kInvalidOrMissingThemeColor;
It doesn't sound right to do that here. Shouldn't you do that in ShortcutInfo's
constructor?

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:36: theme_color = manifest.theme_color;
Move that to the end of the method.

[Lalit Maganti, 2015-07-20 13:15:19]

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:39: //
This value is equal to kInvalidOrMissingThemeColor in the C++ Manifest struct.
On 2015/07/20 at 13:03:30, Mounir Lamouri wrote:
> nit: content::Manifest

Done.

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:269:
}
On 2015/07/20 at 13:03:30, Mounir Lamouri wrote:
> Not related to your change but I'm surprised to see that we don't update the
theme color if |!isWebappDomain()|. Does that mean that we would keep the
Manifest's theme colour if we end up navigating to a different domain that
specifies its own theme colour?

Looks like it. Should I remove that check?

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:127:
mThemeColor = newInfo.mThemeColor;
On 2015/07/20 at 13:03:30, Mounir Lamouri wrote:
> You still have one too many mThemeColor here.

Not sure why the previous patch didn't have that. Maybe the file didn't save?
Anyway done.

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
chrome/browser/android/shortcut_data_fetcher.cc:77: shortcut_info_.theme_color =
content::Manifest::kInvalidOrMissingThemeColor;
On 2015/07/20 at 13:03:30, Mounir Lamouri wrote:
> It doesn't sound right to do that here. Shouldn't you do that in
ShortcutInfo's constructor?

Moved.

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:36: theme_color = manifest.theme_color;
On 2015/07/20 at 13:03:30, Mounir Lamouri wrote:
> Move that to the end of the method.

Done.

[mlamouri (slow - plz ping), 2015-07-20 13:29:45]

lgtm with the comments applied.

Also, I think you are adding theme_color in different places depending on the
file. Sometimes before SOURCE, sometimes after. Could you make sure it's
consistent? I think after ORIENTATION is fine. I don't mind before or after
SOURCE but consistency is always good :)

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/130001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:269:
}
On 2015/07/20 at 13:15:19, Lalit Maganti wrote:
> On 2015/07/20 at 13:03:30, Mounir Lamouri wrote:
> > Not related to your change but I'm surprised to see that we don't update the
theme color if |!isWebappDomain()|. Does that mean that we would keep the
Manifest's theme colour if we end up navigating to a different domain that
specifies its own theme colour?
> 
> Looks like it. Should I remove that check?

Not in this CL but feel free to look into it.

https://codereview.chromium.org/1234653004/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivityTab.java
(right):

https://codereview.chromium.org/1234653004/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivityTab.java:329:
intent.putExtra(ShortcutHelper.EXTRA_THEME_COLOR, webAppInfo.themeColor());
Should that be after EXTRA_SOURCE?

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:18:
theme_color(content::Manifest::kInvalidOrMissingThemeColor),
I think you can try to use a C++11 new feature: delegated constructor. I'm not
100% if it does work for default ctor but I don't see an obvious reason why it
shouldn't. Could you try:

ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
    : ShorcutInfo(),
      url(shortcut_url) {
  [...]
}

That way, you don't need to repeat yourself.

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:52: // Set the theme color based on the
manifest value, if any
nit: finish the sentence with a period.

[Lalit Maganti, 2015-07-20 13:56:13]

Have introduced consistency.

https://codereview.chromium.org/1234653004/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivityTab.java
(right):

https://codereview.chromium.org/1234653004/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivityTab.java:329:
intent.putExtra(ShortcutHelper.EXTRA_THEME_COLOR, webAppInfo.themeColor());
On 2015/07/20 at 13:29:45, Mounir Lamouri wrote:
> Should that be after EXTRA_SOURCE?

Moved everything to be after SOURCE

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:18:
theme_color(content::Manifest::kInvalidOrMissingThemeColor),
On 2015/07/20 at 13:29:45, Mounir Lamouri wrote:
> I think you can try to use a C++11 new feature: delegated constructor. I'm not
100% if it does work for default ctor but I don't see an obvious reason why it
shouldn't. Could you try:
> 
> ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
>     : ShorcutInfo(),
>       url(shortcut_url) {
>   [...]
> }
> 
> That way, you don't need to repeat yourself.

Done.

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:52: // Set the theme color based on the
manifest value, if any
On 2015/07/20 at 13:29:45, Mounir Lamouri wrote:
> nit: finish the sentence with a period.

Done.

[Lalit Maganti, 2015-07-20 13:59:46]

lalitm@google.com changed reviewers:
+ bauerb@chromium.org, dfalcantara@chromium.org

[Lalit Maganti, 2015-07-20 13:59:46]

bauerb: could you please review the entire patch as you own most of the files?
dfalcantara: same as above please.

[Bernhard Bauer, 2015-07-20 16:18:23]

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:268:
mBrandColor = (int) mWebappInfo.themeColor();
How does this work? What happens if the themeColor doesn't fit into an int? How
is mBrandColor encoded anyway?

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:158:
public long themeColor() {
Can you add Javadoc please? How is the color specified? Any special values?

[Lalit Maganti, 2015-07-20 16:24:49]

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:268:
mBrandColor = (int) mWebappInfo.themeColor();
On 2015/07/20 at 16:18:23, Bernhard Bauer wrote:
> How does this work? What happens if the themeColor doesn't fit into an int?
How is mBrandColor encoded anyway?

So the way this works is that mBrandColor is the color the web page wants to set
for recents. However, the website as a whole can specify a "theme color" they
want to use for every page in the website. The page's setting takes precedence
over the site's setting. 

Theme color is an unsigned 32 bit int so it will definitely fit into the size of
an int. long is used because we need to encode error state of -1.

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:158:
public long themeColor() {
On 2015/07/20 at 16:18:23, Bernhard Bauer wrote:
> Can you add Javadoc please? How is the color specified? Any special values?

Will add javadoc. Please see my other comment for an explanation of themeColor.
Special attribute of ShortcutHelper.THEME_COLOR_INVALID_OR_MISSING

[Bernhard Bauer, 2015-07-20 17:11:36]

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:18:
theme_color(content::Manifest::kInvalidOrMissingThemeColor),
On 2015/07/20 13:56:13, Lalit Maganti wrote:
> On 2015/07/20 at 13:29:45, Mounir Lamouri wrote:
> > I think you can try to use a C++11 new feature: delegated constructor. I'm
not
> 100% if it does work for default ctor but I don't see an obvious reason why it
> shouldn't. Could you try:
> > 
> > ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
> >     : ShorcutInfo(),
> >       url(shortcut_url) {
> >   [...]
> > }
> > 
> > That way, you don't need to repeat yourself.
> 
> Done.

Is it?

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:268:
mBrandColor = (int) mWebappInfo.themeColor();
On 2015/07/20 16:24:49, Lalit Maganti wrote:
> On 2015/07/20 at 16:18:23, Bernhard Bauer wrote:
> > How does this work? What happens if the themeColor doesn't fit into an int?
> How is mBrandColor encoded anyway?
> 
> So the way this works is that mBrandColor is the color the web page wants to
set
> for recents. However, the website as a whole can specify a "theme color" they
> want to use for every page in the website. The page's setting takes precedence
> over the site's setting. 
> 
> Theme color is an unsigned 32 bit int so it will definitely fit into the size
of
> an int. long is used because we need to encode error state of -1.

Hm... this whole thing feels a bit brittle. We are absolutely sure there are no
cases where we make a long out of an int, right? Because the compiler will
happily do that even implicitly, and if the MSB of the int is set, we'll end up
with a negative long value.

I wonder whether it would make sense to do the long encoding in a way that
preserves the numeric values (i.e. a negative int color value would be the same
numeric value in long), with e.g. MAX_INTEGER + 1 used for the sentinel value.
Alternatively, can't we pass around an Integer object (and either an associated
boolean flag or a scoped_ptr<int32_t> in native code)?

https://codereview.chromium.org/1234653004/diff/230001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/230001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:160:
* in ARGB format. mThemeColor is a long because we also need to encode the
Wait a sec, this is ARGB, but the comment for WebColor says it's RGBA (and that
it's equivalent to SkColor... which is ARGB again). Is that comment just wrong?

[Lalit Maganti, 2015-07-20 17:17:05]

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:18:
theme_color(content::Manifest::kInvalidOrMissingThemeColor),
On 2015/07/20 at 17:11:35, Bernhard Bauer wrote:
> On 2015/07/20 13:56:13, Lalit Maganti wrote:
> > On 2015/07/20 at 13:29:45, Mounir Lamouri wrote:
> > > I think you can try to use a C++11 new feature: delegated constructor. I'm
not
> > 100% if it does work for default ctor but I don't see an obvious reason why
it
> > shouldn't. Could you try:
> > > 
> > > ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
> > >     : ShorcutInfo(),
> > >       url(shortcut_url) {
> > >   [...]
> > > }
> > > 
> > > That way, you don't need to repeat yourself.
> > 
> > Done.
> 
> Is it?

I did try it but since Android uses GCC and not Clang, initializer lists are not
allowed :(

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:268:
mBrandColor = (int) mWebappInfo.themeColor();
On 2015/07/20 at 17:11:35, Bernhard Bauer wrote:
> On 2015/07/20 16:24:49, Lalit Maganti wrote:
> > On 2015/07/20 at 16:18:23, Bernhard Bauer wrote:
> > > How does this work? What happens if the themeColor doesn't fit into an
int?
> > How is mBrandColor encoded anyway?
> > 
> > So the way this works is that mBrandColor is the color the web page wants to
set
> > for recents. However, the website as a whole can specify a "theme color"
they
> > want to use for every page in the website. The page's setting takes
precedence
> > over the site's setting. 
> > 
> > Theme color is an unsigned 32 bit int so it will definitely fit into the
size of
> > an int. long is used because we need to encode error state of -1.
> 
> Hm... this whole thing feels a bit brittle. We are absolutely sure there are
no cases where we make a long out of an int, right? Because the compiler will
happily do that even implicitly, and if the MSB of the int is set, we'll end up
with a negative long value.
> 
> I wonder whether it would make sense to do the long encoding in a way that
preserves the numeric values (i.e. a negative int color value would be the same
numeric value in long), with e.g. MAX_INTEGER + 1 used for the sentinel value.
Alternatively, can't we pass around an Integer object (and either an associated
boolean flag or a scoped_ptr<int32_t> in native code)?

I did consider the boolean flag and pointer but Mounir and I both felt that
would be uglier than simply using a long.

I am pretty sure that we will never have any other value than a valid 32 bit
unsigned integer or -1 as we make this check in multiple places: see
https://codereview.chromium.org/1235883007

https://codereview.chromium.org/1234653004/diff/230001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1234653004/diff/230001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:160:
* in ARGB format. mThemeColor is a long because we also need to encode the
On 2015/07/20 at 17:11:35, Bernhard Bauer wrote:
> Wait a sec, this is ARGB, but the comment for WebColor says it's RGBA (and
that it's equivalent to SkColor... which is ARGB again). Is that comment just
wrong?

I think the WebColor comment is incorrect.

[Lalit Maganti, 2015-07-20 17:28:30]

On 2015/07/20 at 17:17:05, Lalit Maganti wrote:
>
https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
> File chrome/browser/android/shortcut_info.cc (right):
> 
>
https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
> chrome/browser/android/shortcut_info.cc:18:
theme_color(content::Manifest::kInvalidOrMissingThemeColor),
> On 2015/07/20 at 17:11:35, Bernhard Bauer wrote:
> > On 2015/07/20 13:56:13, Lalit Maganti wrote:
> > > On 2015/07/20 at 13:29:45, Mounir Lamouri wrote:
> > > > I think you can try to use a C++11 new feature: delegated constructor.
I'm not
> > > 100% if it does work for default ctor but I don't see an obvious reason
why it
> > > shouldn't. Could you try:
> > > > 
> > > > ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
> > > >     : ShorcutInfo(),
> > > >       url(shortcut_url) {
> > > >   [...]
> > > > }
> > > > 
> > > > That way, you don't need to repeat yourself.
> > > 
> > > Done.
> > 
> > Is it?
> 
> I did try it but since Android uses GCC and not Clang, initializer lists are
not allowed :(
> 
>
https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
> File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):
> 
>
https://codereview.chromium.org/1234653004/diff/210001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:268:
mBrandColor = (int) mWebappInfo.themeColor();
> On 2015/07/20 at 17:11:35, Bernhard Bauer wrote:
> > On 2015/07/20 16:24:49, Lalit Maganti wrote:
> > > On 2015/07/20 at 16:18:23, Bernhard Bauer wrote:
> > > > How does this work? What happens if the themeColor doesn't fit into an
int?
> > > How is mBrandColor encoded anyway?
> > > 
> > > So the way this works is that mBrandColor is the color the web page wants
to set
> > > for recents. However, the website as a whole can specify a "theme color"
they
> > > want to use for every page in the website. The page's setting takes
precedence
> > > over the site's setting. 
> > > 
> > > Theme color is an unsigned 32 bit int so it will definitely fit into the
size of
> > > an int. long is used because we need to encode error state of -1.
> > 
> > Hm... this whole thing feels a bit brittle. We are absolutely sure there are
no cases where we make a long out of an int, right? Because the compiler will
happily do that even implicitly, and if the MSB of the int is set, we'll end up
with a negative long value.
> > 
> > I wonder whether it would make sense to do the long encoding in a way that
preserves the numeric values (i.e. a negative int color value would be the same
numeric value in long), with e.g. MAX_INTEGER + 1 used for the sentinel value.
Alternatively, can't we pass around an Integer object (and either an associated
boolean flag or a scoped_ptr<int32_t> in native code)?
> 
> I did consider the boolean flag and pointer but Mounir and I both felt that
would be uglier than simply using a long.
> 
> I am pretty sure that we will never have any other value than a valid 32 bit
unsigned integer or -1 as we make this check in multiple places: see
https://codereview.chromium.org/1235883007
> 
>
https://codereview.chromium.org/1234653004/diff/230001/chrome/android/java/sr...
> File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):
> 
>
https://codereview.chromium.org/1234653004/diff/230001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:160:
* in ARGB format. mThemeColor is a long because we also need to encode the
> On 2015/07/20 at 17:11:35, Bernhard Bauer wrote:
> > Wait a sec, this is ARGB, but the comment for WebColor says it's RGBA (and
that it's equivalent to SkColor... which is ARGB again). Is that comment just
wrong?
> 
> I think the WebColor comment is incorrect.

Delegating constructors not initialzer lists :)

[gone, 2015-07-20 17:31:27]

lgtm % bauerb's concerns

[mlamouri (slow - plz ping), 2015-08-05 13:27:15]

Any progress on that?

[Lalit Maganti, 2015-08-05 13:42:11]

On 2015/08/05 at 13:27:15, mlamouri wrote:
> Any progress on that?

Blocked on https://codereview.chromium.org/1246953002/

[Lalit Maganti, 2015-08-10 12:58:41]

bauerb@: now that the int casting issue has been resolved could you please take
another look at this please?

[Bernhard Bauer, 2015-08-11 09:40:48]

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1234653004/diff/150001/chrome/browser/android...
chrome/browser/android/shortcut_info.cc:18:
theme_color(content::Manifest::kInvalidOrMissingThemeColor),
On 2015/07/20 17:17:04, Lalit Maganti wrote:
> On 2015/07/20 at 17:11:35, Bernhard Bauer wrote:
> > On 2015/07/20 13:56:13, Lalit Maganti wrote:
> > > On 2015/07/20 at 13:29:45, Mounir Lamouri wrote:
> > > > I think you can try to use a C++11 new feature: delegated constructor.
I'm
> not
> > > 100% if it does work for default ctor but I don't see an obvious reason
why
> it
> > > shouldn't. Could you try:
> > > > 
> > > > ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
> > > >     : ShorcutInfo(),
> > > >       url(shortcut_url) {
> > > >   [...]
> > > > }
> > > > 
> > > > That way, you don't need to repeat yourself.
> > > 
> > > Done.
> > 
> > Is it?
> 
> I did try it but since Android uses GCC and not Clang, initializer lists are
not
> allowed :(

:(

OTOH, the style guide actually does allow default arguments in constructors
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...),
so you could use that.

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:272:
&& (mWebappInfo.themeColor() | 0xFF000000L) != 0) {
This condition is always true, no?

[Lalit Maganti, 2015-08-11 09:43:21]

Will have a look at default args.

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:272:
&& (mWebappInfo.themeColor() | 0xFF000000L) != 0) {
On 2015/08/11 at 09:40:48, Bernhard Bauer wrote:
> This condition is always true, no?

This is to stop fully transparent colors getting through.

[Bernhard Bauer, 2015-08-11 09:52:22]

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:272:
&& (mWebappInfo.themeColor() | 0xFF000000L) != 0) {
On 2015/08/11 09:43:20, Lalit Maganti wrote:
> On 2015/08/11 at 09:40:48, Bernhard Bauer wrote:
> > This condition is always true, no?
> 
> This is to stop fully transparent colors getting through.

Right, but the expression is always going to have the most significant bytes set
to FF, so it will never be equal to 0.

[Lalit Maganti, 2015-08-11 09:55:24]

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1234653004/diff/370001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:272:
&& (mWebappInfo.themeColor() | 0xFF000000L) != 0) {
On 2015/08/11 at 09:52:22, Bernhard Bauer wrote:
> On 2015/08/11 09:43:20, Lalit Maganti wrote:
> > On 2015/08/11 at 09:40:48, Bernhard Bauer wrote:
> > > This condition is always true, no?
> > 
> > This is to stop fully transparent colors getting through.
> 
> Right, but the expression is always going to have the most significant bytes
set to FF, so it will never be equal to 0.

Sorry I completely blanked here - the | should be a & :/ Will fix.

[Lalit Maganti, 2015-08-11 10:17:44]

bauerb@: this should be good now

[Bernhard Bauer, 2015-08-11 10:33:51]

lgtm

[Lalit Maganti, 2015-08-12 10:10:11]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-12 10:10:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, dfalcantara@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1234653004/#ps440001
(title: "Fix missing include")

[commit-bot: I haz the power, 2015-08-12 10:10:16]

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

[commit-bot: I haz the power, 2015-08-12 10:53:53]

Committed patchset #24 (id:440001)

[commit-bot: I haz the power, 2015-08-12 10:54:29]

Patchset 24 (id:??) landed as
https://crrev.com/2f5becad9f3833633845c94db12a3a043be76e31
Cr-Commit-Position: refs/heads/master@{#343002}