Can you please specify what you want to define a tile impression as? Currently on the NTP it's defined as "NTP created". for the Chrome Home, should that be:

1) Bottom Sheet created
2) Bottom Sheet opened
3) Home Sheet displayed
4) ???

There are also some other metrics related calls that we do on NTP load, should we port them too? In particular I'm thinking about "NewTabPage.TileOfflineAvailable". (for reference, call is made here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java?type=cs&q=NewTabPage+onLoadingComplete&l=269)

Should we use a new metric since the meaning will be slightly different? 

We can also distinguish cases where the home sheet is opened as a new tab (as opposed to over an existing web page) if you want

I think for these metrics we do not care much about absolute numbers, we rather care about ratios between different types and these will stay the same. Thus, I propose to stick with the same metrics (there are 15 different metrics, currently).

Any of the options above is roughly fine. The only situation where it matters is interpreting CTR. For this, option 3) is the best. 
+mastiz to double-check.


Please keep in mind that we need to call the new function RecordTileImpression at the moment of time when we already know the type of the icon (i.e. result from LargeIconService have already arrived). This is currently broken - we report many impressions with icon type NONE.

I plan to fix this NONE-problem for the existing android non-ChromeHome NTP. However, if you could possibly fix that as well, that would be awesome :)

FYI, "Home Sheet displayed" includes things like switching from the bookmarks tab to the home tab, or going to the home screen while the bottom sheet is open and then coming back to chrome and seeing the bottom sheet open.

Does that align with your expectations?

Hm...

Ideal would be option 4):
Home sheet displayed but record only once between any two "Botton sheet opened" events. 
(This is basically like 2) but delay the reporting to the moment when Home sheet is displayed)

Mikel, do you have a take on recording tile impressions in ChromeHome?

mastiz@, can you please comment on the question above?

It would be cool to get the fix in before 61 BP.

I agree with jkrcal's rationale. Counting the impression once seems best. As per 2) vs 4), I can't really tell if the difference will be big in reality, and how difficult 4) would be to instrument.

Thanks, Mikel.

We've discussed it offline -> 4) still seems reasonably simple to implement.
Nicolas will take a look on that.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b59aefda1eede7818478cd39e984f6868392396b

commit b59aefda1eede7818478cd39e984f6868392396b
Author: Nicolas Dossou-gbete <dgn@chromium.org>
Date: Tue Jul 11 12:14:02 2017

[Suggestions] Log Tile metrics when ChromeHome is enabled

Add load task tracking to the TileGrid to allow it to record UMA when
the TileGroup has completed all its load tasks.

In addition to logging the Tile state when the group is initialised,
we also do so when the bottom sheet opens. To support the new load
task triggers, Tile load task tracking moved inside of the TileGroup
and only the completion event is exposed through the delegate.

Appropriately updated the non-Chrome Home code to separate non-Tile
completion logging for Tile specific one.

Bug:  735945 
Change-Id: I8582ae510fe1edda31197460bcf9db2527e22682
Reviewed-on: https://chromium-review.googlesource.com/558358
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485602}
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java
[modify] https://crrev.com/b59aefda1eede7818478cd39e984f6868392396b/chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/371065d24ec1d737d69949568c5320dcba27e49c

commit 371065d24ec1d737d69949568c5320dcba27e49c
Author: Nicolas Dossou-gbete <dgn@chromium.org>
Date: Thu Jul 20 10:38:24 2017

[Suggestions] Separate init trigger from bottom sheet opening

We want the initialisation of the home sheet to happen when the bottom
sheet opens and shows the home sheet. However, even though it's the
default sheet, the bottom sheet can also open to show omnibox
suggestions. In that case the home sheet is not shown and should not
be loaded.

This CL makes initialisation happen the first time the sheet is shown
since the opening of the bottom sheet.

Making the home sheet not be selected when the omnibox suggestions are
shown is out of scope and tracked in  https://crbug.com/731128 

Bug:  735945 
Change-Id: I61406ea0ff4207370d86d3fd0712e339813b7ff7
Reviewed-on: https://chromium-review.googlesource.com/563309
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488189}
[modify] https://crrev.com/371065d24ec1d737d69949568c5320dcba27e49c/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
[modify] https://crrev.com/371065d24ec1d737d69949568c5320dcba27e49c/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsSheetVisibilityChangeObserver.java

I'm going to mark this fixed for now, this from the bottom sheet perspective we're doing the right thing.  issue 731128  is still pending and means we would trigger more often than necessary, but that would hopefully be fixed for M-62