NTP: Fix metrics recording crash by plumbing the necessary data to Java.

NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG=612160
Committed: https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2
Cr-Commit-Position: refs/heads/master@{#405079}

[dewittj, 2016-06-28 21:37:49]

dewittj@chromium.org changed reviewers:
+ bauerb@chromium.org, treib@chromium.org

[dewittj, 2016-06-28 21:37:50]

I ran into this crash while testing today, so hoping to get the fix in for M53. 
PTAL, thanks.

Let me know if the comments in NTP code can be improved in any way, I only have
very trivial knowledge about how the various enums etc are used.

[Marc Treib, 2016-06-29 07:53:57]

Description was changed from

==========
NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG=612160
==========

to

==========
NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG=612160
==========

[Marc Treib, 2016-06-29 08:19:32]

Thanks for taking this on! Some comments below.

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java
(right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:70:
* @param index The index of this item in the list of most visited items.
Add comments for source and providerIndex?

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:556:
public static void recordTileTypeMetrics(MostVisitedItem[] items) {
Hmmmm. I know I said differently on the bug, but I think I'd prefer to pass the
info back to C++ and record the metrics there after all. We've been trying to
get all the NTP metrics for all platforms in one place, because the current
practice of things being scattered everywhere and duplicated for various
platforms has led to a bunch of inconsistencies (if you're curious:
https://docs.google.com/spreadsheets/d/1W9eXR0G1GiJbd8SCX_mGSVqnxnTy-r7NsJKvG...).

If you pass the sources and providerIndices back along with the tileTypes, then
that should also fix the bug, right?

Bernhard, Chris, WDYT?

https://codereview.chromium.org/2105933002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites_bridge.cc:127:
whitelist_icon_paths.reserve(suggestions.size());
Also reserve sources and provider_indexes

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:115: };
Any particular reason for moving this into the header?

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:121: // Item comes from the regional
top sites list.
Nope, these are the *personal* top sites.

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:125: // Item is most popular.
These are the regionally popular ones.

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:127: // Item on static whitelist
s/static/supervised user/ ?
(These whitelists exist only for supervised users, and I don't know what
"static" is supposed to mean)

[Bernhard Bauer, 2016-06-29 10:17:43]

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:556:
public static void recordTileTypeMetrics(MostVisitedItem[] items) {
On 2016/06/29 08:19:31, Marc Treib wrote:
> Hmmmm. I know I said differently on the bug, but I think I'd prefer to pass
the
> info back to C++ and record the metrics there after all. We've been trying to
> get all the NTP metrics for all platforms in one place, because the current
> practice of things being scattered everywhere and duplicated for various
> platforms has led to a bunch of inconsistencies (if you're curious:
>
https://docs.google.com/spreadsheets/d/1W9eXR0G1GiJbd8SCX_mGSVqnxnTy-r7NsJKvG...).
> 
> If you pass the sources and providerIndices back along with the tileTypes,
then
> that should also fix the bug, right?
> 
> Bernhard, Chris, WDYT?

I somewhat prefer doing the tallying up here in Java and then only passing final
counts to C++. Could we define the interface on the C++ side so it will accept
the counts? That should still avoid inconsistencies.

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:563:
++countsPerType[tileType];
Small nit: I'm not sure to what extent the microoptimization of preincrement
instead of postincrement applies to Java. Just in terms of readability, I
somewhat prefer postincrement if the expression value is not needed, because it
reads a bit more imperative.

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:590:
assert false; // An unknown source was used.
Nit: additional space before comments. Also, you could add the value to the
assertion message.

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:121: // Item comes from the regional
top sites list.
On 2016/06/29 08:19:32, Marc Treib wrote:
> Nope, these are the *personal* top sites.

(The regional ones are called "popular sites" 😃)

[Marc Treib, 2016-06-29 10:23:11]

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:556:
public static void recordTileTypeMetrics(MostVisitedItem[] items) {
On 2016/06/29 10:17:43, Bernhard Bauer wrote:
> On 2016/06/29 08:19:31, Marc Treib wrote:
> > Hmmmm. I know I said differently on the bug, but I think I'd prefer to pass
> the
> > info back to C++ and record the metrics there after all. We've been trying
to
> > get all the NTP metrics for all platforms in one place, because the current
> > practice of things being scattered everywhere and duplicated for various
> > platforms has led to a bunch of inconsistencies (if you're curious:
> >
>
https://docs.google.com/spreadsheets/d/1W9eXR0G1GiJbd8SCX_mGSVqnxnTy-r7NsJKvG...).
> > 
> > If you pass the sources and providerIndices back along with the tileTypes,
> then
> > that should also fix the bug, right?
> > 
> > Bernhard, Chris, WDYT?
> 
> I somewhat prefer doing the tallying up here in Java and then only passing
final
> counts to C++. Could we define the interface on the C++ side so it will accept
> the counts? That should still avoid inconsistencies.

Hm, we would have one count per "source+providerIndex". So we'd need to pass
some sort of key-value store. Not sure that's worth the complexity...
Mid-term, I'd like to get rid of the provider indices altogether, since they
don't give any useful information anyway, but that's not going to happen before
branch point.

[dewittj, 2016-06-30 17:48:54]

ptal

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java
(right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:70:
* @param index The index of this item in the list of most visited items.
On 2016/06/29 08:19:31, Marc Treib wrote:
> Add comments for source and providerIndex?

Done.

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:556:
public static void recordTileTypeMetrics(MostVisitedItem[] items) {
On 2016/06/29 10:23:11, Marc Treib wrote:
> On 2016/06/29 10:17:43, Bernhard Bauer wrote:
> > On 2016/06/29 08:19:31, Marc Treib wrote:
> > > Hmmmm. I know I said differently on the bug, but I think I'd prefer to
pass
> > the
> > > info back to C++ and record the metrics there after all. We've been trying
> to
> > > get all the NTP metrics for all platforms in one place, because the
current
> > > practice of things being scattered everywhere and duplicated for various
> > > platforms has led to a bunch of inconsistencies (if you're curious:
> > >
> >
>
https://docs.google.com/spreadsheets/d/1W9eXR0G1GiJbd8SCX_mGSVqnxnTy-r7NsJKvG...).
> > > 
> > > If you pass the sources and providerIndices back along with the tileTypes,
> > then
> > > that should also fix the bug, right?
> > > 
> > > Bernhard, Chris, WDYT?
> > 
> > I somewhat prefer doing the tallying up here in Java and then only passing
> final
> > counts to C++. Could we define the interface on the C++ side so it will
accept
> > the counts? That should still avoid inconsistencies.
> 
> Hm, we would have one count per "source+providerIndex". So we'd need to pass
> some sort of key-value store. Not sure that's worth the complexity...
> Mid-term, I'd like to get rid of the provider indices altogether, since they
> don't give any useful information anyway, but that's not going to happen
before
> branch point.

I passed the types, sources, and indices back to C++; wdyt about that strategy?

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:563:
++countsPerType[tileType];
On 2016/06/29 10:17:43, Bernhard Bauer wrote:
> Small nit: I'm not sure to what extent the microoptimization of preincrement
> instead of postincrement applies to Java. Just in terms of readability, I
> somewhat prefer postincrement if the expression value is not needed, because
it
> reads a bit more imperative.

Done.

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:590:
assert false; // An unknown source was used.
On 2016/06/29 10:17:43, Bernhard Bauer wrote:
> Nit: additional space before comments. Also, you could add the value to the
> assertion message.

Done.

https://codereview.chromium.org/2105933002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites_bridge.cc:127:
whitelist_icon_paths.reserve(suggestions.size());
On 2016/06/29 08:19:31, Marc Treib wrote:
> Also reserve sources and provider_indexes

Done.

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:115: };
On 2016/06/29 08:19:32, Marc Treib wrote:
> Any particular reason for moving this into the header?

A few reasons.  This enum is exposed to Java, making it semi-public already. 
Also, the gypi build file appears to take only one source file, so I preferred
to have it scan the .h than to make two rules, one for the .h and one for the
.cc

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:121: // Item comes from the regional
top sites list.
On 2016/06/29 10:17:43, Bernhard Bauer wrote:
> On 2016/06/29 08:19:32, Marc Treib wrote:
> > Nope, these are the *personal* top sites.
> 
> (The regional ones are called "popular sites" 😃)

Done.

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:125: // Item is most popular.
On 2016/06/29 08:19:32, Marc Treib wrote:
> These are the regionally popular ones.

Done.

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:127: // Item on static whitelist
On 2016/06/29 08:19:32, Marc Treib wrote:
> s/static/supervised user/ ?
> (These whitelists exist only for supervised users, and I don't know what
> "static" is supposed to mean)

Done.

[Marc Treib, 2016-07-01 10:01:04]

LGTM, thanks!

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2105933002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:556:
public static void recordTileTypeMetrics(MostVisitedItem[] items) {
On 2016/06/30 17:48:53, dewittj wrote:
> On 2016/06/29 10:23:11, Marc Treib wrote:
> > On 2016/06/29 10:17:43, Bernhard Bauer wrote:
> > > On 2016/06/29 08:19:31, Marc Treib wrote:
> > > > Hmmmm. I know I said differently on the bug, but I think I'd prefer to
> pass
> > > the
> > > > info back to C++ and record the metrics there after all. We've been
trying
> > to
> > > > get all the NTP metrics for all platforms in one place, because the
> current
> > > > practice of things being scattered everywhere and duplicated for various
> > > > platforms has led to a bunch of inconsistencies (if you're curious:
> > > >
> > >
> >
>
https://docs.google.com/spreadsheets/d/1W9eXR0G1GiJbd8SCX_mGSVqnxnTy-r7NsJKvG...).
> > > > 
> > > > If you pass the sources and providerIndices back along with the
tileTypes,
> > > then
> > > > that should also fix the bug, right?
> > > > 
> > > > Bernhard, Chris, WDYT?
> > > 
> > > I somewhat prefer doing the tallying up here in Java and then only passing
> > final
> > > counts to C++. Could we define the interface on the C++ side so it will
> accept
> > > the counts? That should still avoid inconsistencies.
> > 
> > Hm, we would have one count per "source+providerIndex". So we'd need to pass
> > some sort of key-value store. Not sure that's worth the complexity...
> > Mid-term, I'd like to get rid of the provider indices altogether, since they
> > don't give any useful information anyway, but that's not going to happen
> before
> > branch point.
> 
> I passed the types, sources, and indices back to C++; wdyt about that
strategy?

SGTM

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2105933002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:115: };
On 2016/06/30 17:48:54, dewittj wrote:
> On 2016/06/29 08:19:32, Marc Treib wrote:
> > Any particular reason for moving this into the header?
> 
> A few reasons.  This enum is exposed to Java, making it semi-public already. 
> Also, the gypi build file appears to take only one source file, so I preferred
> to have it scan the .h than to make two rules, one for the .h and one for the
> .cc

Acknowledged.

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/most_visited_sites.cc:272: DCHECK_LT(index,
static_cast<int>(current_suggestions_.size()));
Huh, now that I look at this, it seems that this method has the exact same
problem - |current_suggestions_| might have changed since the UI got its copy,
right?
Clearly it's not your responsibility to fix this, but would you mind adding a
TODO(treib)?

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/most_visited_sites.h:127: // Item is on an
administrator-managed whitelist.
s/administrator/custodian (that's the terminology we use in the supervised user
world)

[dewittj, 2016-07-01 21:35:21]

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/most_visited_sites.cc:272: DCHECK_LT(index,
static_cast<int>(current_suggestions_.size()));
On 2016/07/01 10:01:04, Marc Treib wrote:
> Huh, now that I look at this, it seems that this method has the exact same
> problem - |current_suggestions_| might have changed since the UI got its copy,
> right?
> Clearly it's not your responsibility to fix this, but would you mind adding a
> TODO(treib)?

Done.

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2105933002/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/most_visited_sites.h:127: // Item is on an
administrator-managed whitelist.
On 2016/07/01 10:01:04, Marc Treib wrote:
> s/administrator/custodian (that's the terminology we use in the supervised
user
> world)

Done.

[Bernhard Bauer, 2016-07-04 12:49:56]

lgtm

[dewittj, 2016-07-11 20:55:21]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-07-11 20:55:22]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2105933002/#ps120001
(title: "Address treib@ comments.")

[commit-bot: I haz the power, 2016-07-11 20:56:00]

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-07-11 22:20:54]

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

[commit-bot: I haz the power, 2016-07-11 22:20:55]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Marc Treib, 2016-07-12 11:11:44]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-07-12 11:12:13]

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-07-12 15:16:29]

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

[commit-bot: I haz the power, 2016-07-12 15:16:31]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2016-07-12 16:29:17]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-07-12 16:32:53]

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-07-12 20:16:15]

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

[commit-bot: I haz the power, 2016-07-12 20:16:16]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dewittj, 2016-07-12 21:53:01]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2016-07-12 21:55:39]

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-07-13 00:47:42]

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

[commit-bot: I haz the power, 2016-07-13 00:47:44]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2016-07-13 07:46:16]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-07-13 07:46:41]

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-07-13 08:25:53]

Description was changed from

==========
NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG=612160
==========

to

==========
NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG=612160
==========

[commit-bot: I haz the power, 2016-07-13 08:25:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-13 08:26:14]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 08:27:24]

Description was changed from

==========
NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG=612160
==========

to

==========
NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG=612160

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

[commit-bot: I haz the power, 2016-07-13 08:27:25]

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