Track when fallback icon color is the default.

Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.

BUG=651056
Committed: https://crrev.com/b2a55666aab061458f3d721909533a26de137a29
Cr-Commit-Position: refs/heads/master@{#422739}

[sfiera, 2016-09-27 15:49:46]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org, noyau@chromium.org, pkotwicz@chromium.org

[sfiera, 2016-09-27 15:49:47]

PTAL. I don't know what the URL parser is used for, since there doesn't seem to
be any code that serializes fallback icon styles into that format. It's possible
I've made a mistake related to that.

[sfiera, 2016-09-27 15:51:59]

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

[commit-bot: I haz the power, 2016-09-27 15:52:11]

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

[Bernhard Bauer, 2016-09-27 16:14:57]

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java
(right):

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java:24:
private LruCache<String, Pair<Bitmap, Pair<Integer, Boolean>>> mFaviconCache;
This is getting a bit unwieldy... Would it make sense to make a struct-like
class out of it?

[commit-bot: I haz the power, 2016-09-27 16:41:30]

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

[commit-bot: I haz the power, 2016-09-27 16:41:31]

Dry run: This issue passed the CQ dry run.

[sfiera, 2016-09-27 16:50:37]

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java
(right):

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java:24:
private LruCache<String, Pair<Bitmap, Pair<Integer, Boolean>>> mFaviconCache;
On 2016/09/27 16:14:57, Bernhard Bauer wrote:
> This is getting a bit unwieldy... Would it make sense to make a struct-like
> class out of it?

I can do that. Initially I was looking for Tuple3<X, Y, Z> but as far as I can
tell Java doesn't have that.

[pkotwicz, 2016-09-27 17:06:20]

I'll take a look at this CL tonight

[pkotwicz, 2016-09-28 03:52:52]

L-G-T-M once you address bauerb@'s comments.

Thank you for the detailed CL description! Especially the explanation for why
you are writing this CL

Can you please add a bug to the CL description?

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java
(right):

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java:108:
Bitmap icon, int fallbackColor, boolean isDefaultColor) {
Nit: I think that things would be clearer if you renamed |isDefaultColor| to
|isFallbackColorDefault|.

This would make it more obvious that |isFallbackColorDefault| refers to
|fallbackColor|

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1047:
private static final int ICON_BACKGROUND_COLOR = 0xff787878;
This constant can be deleted now?

https://codereview.chromium.org/2374753002/diff/1/ios/chrome/browser/favicon/...
File ios/chrome/browser/favicon/large_icon_cache_unittest.cc (right):

https://codereview.chromium.org/2374753002/diff/1/ios/chrome/browser/favicon/...
ios/chrome/browser/favicon/large_icon_cache_unittest.cc:89:
result2->fallback_icon_style->background_color);
Shouldn't you also test:

EXPECT_EQ(expected_result2->fallback_icon_style->is_default_background_color,
result2->fallback_icon_style->is_default_background_color)

https://codereview.chromium.org/2374753002/diff/1/ios/chrome/browser/favicon/...
ios/chrome/browser/favicon/large_icon_cache_unittest.cc:97:
result3->fallback_icon_style->background_color);
Shouldn't you test is_default_background_color here too?

[sfiera, 2016-09-28 12:47:10]

Description was changed from

==========
Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.
==========

to

==========
Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.

BUG=651056
==========

[sfiera, 2016-09-28 13:15:43]

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

[commit-bot: I haz the power, 2016-09-28 13:15:49]

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

[sfiera, 2016-09-28 13:15:52]

Thanks.

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java
(right):

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java:24:
private LruCache<String, Pair<Bitmap, Pair<Integer, Boolean>>> mFaviconCache;
On 2016/09/27 16:50:36, sfiera wrote:
> On 2016/09/27 16:14:57, Bernhard Bauer wrote:
> > This is getting a bit unwieldy... Would it make sense to make a struct-like
> > class out of it?
> 
> I can do that. Initially I was looking for Tuple3<X, Y, Z> but as far as I can
> tell Java doesn't have that.

Done.

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java:108:
Bitmap icon, int fallbackColor, boolean isDefaultColor) {
On 2016/09/28 03:52:52, pkotwicz wrote:
> Nit: I think that things would be clearer if you renamed |isDefaultColor| to
> |isFallbackColorDefault|.
> 
> This would make it more obvious that |isFallbackColorDefault| refers to
> |fallbackColor|

Done.

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2374753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1047:
private static final int ICON_BACKGROUND_COLOR = 0xff787878;
On 2016/09/28 03:52:52, pkotwicz wrote:
> This constant can be deleted now?

Oops, yes, missed this one. Thanks!

https://codereview.chromium.org/2374753002/diff/1/ios/chrome/browser/favicon/...
File ios/chrome/browser/favicon/large_icon_cache_unittest.cc (right):

https://codereview.chromium.org/2374753002/diff/1/ios/chrome/browser/favicon/...
ios/chrome/browser/favicon/large_icon_cache_unittest.cc:89:
result2->fallback_icon_style->background_color);
On 2016/09/28 03:52:52, pkotwicz wrote:
> Shouldn't you also test:
> 
> EXPECT_EQ(expected_result2->fallback_icon_style->is_default_background_color,
> result2->fallback_icon_style->is_default_background_color)

Done.

https://codereview.chromium.org/2374753002/diff/1/ios/chrome/browser/favicon/...
ios/chrome/browser/favicon/large_icon_cache_unittest.cc:97:
result3->fallback_icon_style->background_color);
On 2016/09/28 03:52:52, pkotwicz wrote:
> Shouldn't you test is_default_background_color here too?

Done.

[commit-bot: I haz the power, 2016-09-28 13:27:01]

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

[commit-bot: I haz the power, 2016-09-28 13:27:02]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sfiera, 2016-09-28 13:30:57]

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

[commit-bot: I haz the power, 2016-09-28 13:31:12]

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

[Bernhard Bauer, 2016-09-28 13:49:50]

lgtm

https://codereview.chromium.org/2374753002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java
(right):

https://codereview.chromium.org/2374753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java:72:
int iconBitmapSize = icon.icon == null ? 0 : icon.icon.getByteCount();
Nit: icon.icon reads a bit ugly :) |favicon| maybe?

[commit-bot: I haz the power, 2016-09-28 13:56:51]

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

[commit-bot: I haz the power, 2016-09-28 13:56:52]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[sfiera, 2016-09-28 14:29:59]

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

[commit-bot: I haz the power, 2016-09-28 14:30:12]

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

[sfiera, 2016-09-28 14:30:47]

https://codereview.chromium.org/2374753002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java
(right):

https://codereview.chromium.org/2374753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java:72:
int iconBitmapSize = icon.icon == null ? 0 : icon.icon.getByteCount();
On 2016/09/28 13:49:50, Bernhard Bauer wrote:
> Nit: icon.icon reads a bit ugly :) |favicon| maybe?

Done.

[commit-bot: I haz the power, 2016-09-28 15:08:24]

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

[commit-bot: I haz the power, 2016-09-28 15:08:25]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2016-09-28 15:27:15]

LGTM 🐳

[sfiera, 2016-09-29 09:22:55]

Thanks!

Éric, there's a small test change under ios/, if you could please take a look.

[sfiera, 2016-09-29 09:44:17]

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

[commit-bot: I haz the power, 2016-09-29 09:44:34]

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-09-29 11:04:31]

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

[commit-bot: I haz the power, 2016-09-29 11:04:32]

Dry run: This issue passed the CQ dry run.

[noyau (Ping after 24h), 2016-10-03 08:19:35]

ios/ lgtm.

[sfiera, 2016-10-04 09:18:04]

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

[commit-bot: I haz the power, 2016-10-04 09:18:17]

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

[sfiera, 2016-10-04 10:01:57]

The CQ bit was unchecked by sfiera@chromium.org

[sfiera, 2016-10-04 10:02:01]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-10-04 10:02:02]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
pkotwicz@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2374753002/#ps100001
(title: "Merge branch 'refs/heads/master' into default-color")

[commit-bot: I haz the power, 2016-10-04 10:02:18]

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-10-04 10:07:37]

Description was changed from

==========
Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.

BUG=651056
==========

to

==========
Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.

BUG=651056
==========

[commit-bot: I haz the power, 2016-10-04 10:07:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-04 10:09:33]

Description was changed from

==========
Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.

BUG=651056
==========

to

==========
Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.

BUG=651056

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

[commit-bot: I haz the power, 2016-10-04 10:09:35]

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