Issue 703165 link

Starred by 4 users

Issue metadata

Status:	Fixed
Owner:	mastiz@chromium.org
Closed:	May 2017
Cc:	mastiz@chromium.org █ sfiera@chromium.org treib@chromium.org █ ramyan@chromium.org
Components:	UI>Browser>NewTabPage
EstimatedDays:	----
NextAction:	----
OS:	Linux , Windows , Chrome , Mac
Pri:	1
Type:	Bug
M-59 zine-ntp-pe merge-merged-3071

Blocked on:
issue 714095

Blocking:
issue 718413
issue 718811

Introduce desktop NTP thumbnail metrics

Project Member Reported by treib@chromium.org, Mar 20 2017

Issue description

We've been getting reports of lots of gray (i.e. missing) thumbnails on the desktop NTP. We should (re-)introduce metrics to track the number of impressions with local, remote, and missing thumbnails.

Comment 1 by treib@chromium.org, Mar 29 2017

Components: UI>Browser>NewTabPage
Owner: treib@chromium.org
Status: Assigned (was: Available)

Comment 2 by treib@chromium.org, Mar 30 2017

Status: Started (was: Assigned)

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 3 2017

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

commit 92005c86d523ba4d37d4c2ff60eb05b385bb5c38
Author: treib <treib@chromium.org>
Date: Mon Apr 03 10:25:32 2017

NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe
rather than the plural "thumbnailUrls".
This allows us to get rid of a whole bunch of ugly JS code.

The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the
remote fallback thumbnail) for ML suggestions for a long time, but
TopSites suggestions used "thumbnailUrls" (but with only ever a single
value in the array).

With NTPTilesInInstantService enabled, before this CL all suggestions
used the plural version, but never with more than two entries. Now it's
always the singular version, with an "?fb=" param if appropriate.

I've checked that local NTP, remote NTP, and third-party NTPs still work.

BUG= 703165 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2786833003
Cr-Commit-Position: refs/heads/master@{#461401}

[modify] https://crrev.com/92005c86d523ba4d37d4c2ff60eb05b385bb5c38/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/92005c86d523ba4d37d4c2ff60eb05b385bb5c38/chrome/browser/resources/local_ntp/most_visited_thumbnail.js
[modify] https://crrev.com/92005c86d523ba4d37d4c2ff60eb05b385bb5c38/chrome/renderer/searchbox/searchbox_extension.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 4 2017

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

commit 35508f05e62ec481dfa71cde31cb1c9aa5492b17
Author: treib <treib@chromium.org>
Date: Tue Apr 04 06:40:39 2017

Remove histograms NewTabPage.IconsReal/IconsColor/IconsGray

They have been replaced by NewTabPage.TileType and
NewTabPage.SuggestionsImpression.IconsReal/IconsColor/IconsGray.

BUG= 703165 

Review-Url: https://codereview.chromium.org/2785183002
Cr-Commit-Position: refs/heads/master@{#461645}

[modify] https://crrev.com/35508f05e62ec481dfa71cde31cb1c9aa5492b17/components/ntp_tiles/metrics.cc
[modify] https://crrev.com/35508f05e62ec481dfa71cde31cb1c9aa5492b17/components/ntp_tiles/metrics_unittest.cc
[modify] https://crrev.com/35508f05e62ec481dfa71cde31cb1c9aa5492b17/tools/metrics/histograms/histograms.xml

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 4 2017

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

commit 231212e9d80a4868f0f1192c75e7203dbabb07f6
Author: treib <treib@chromium.org>
Date: Tue Apr 04 17:09:22 2017

ntp_tiles: Cleanup enum names

- Rename "NTPTileSource" to "TileSource". ("NTP" is already in the
  folder/namespace, and is also not really accurate anyway.)
- Rename "MostVisitedTileType" to "TileVisualType", and move it out of
  metrics.h and into its own file. This makes it more accurate, and
  more consistent with TileSource.
- Update the Java packages for these from
  "org.chromium.chrome.browser.ntp" to
  "org.chromium.chrome.browser.suggestions".

BUG= 703165 

Review-Url: https://codereview.chromium.org/2790463003
Cr-Commit-Position: refs/heads/master@{#461756}

[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/FakeMostVisitedSites.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileTest.java
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/search/instant_service.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/search/instant_service_unittest.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/search/search_ipc_router.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/search/search_ipc_router.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/search/search_ipc_router_unittest.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/search/search_tab_helper.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/search/search_tab_helper_unittest.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/common/instant.mojom
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/common/instant.typemap
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/common/instant_struct_traits.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/common/render_messages.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/common/search/instant_types.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/common/search/instant_types.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/renderer/searchbox/searchbox.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/renderer/searchbox/searchbox.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/chrome/renderer/searchbox/searchbox_extension.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/BUILD.gn
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/metrics.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/metrics.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/metrics_unittest.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/most_visited_sites.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/most_visited_sites_unittest.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/ntp_tile.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/ntp_tile.h
[rename] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/tile_source.h
[add] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/tile_visual_type.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/components/ntp_tiles/webui/ntp_tiles_internals_message_handler_client.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/ios/chrome/browser/ui/ntp/google_landing_controller.mm
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/ios/chrome/browser/ui/ntp/most_visited_cell.h
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/ios/chrome/browser/ui/ntp/most_visited_cell.mm
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc
[modify] https://crrev.com/231212e9d80a4868f0f1192c75e7203dbabb07f6/tools/metrics/histograms/histograms.xml

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 7 2017

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

commit 80f5122d11253e8c76379813a5352feb024b9ea2
Author: treib <treib@chromium.org>
Date: Fri Apr 07 07:59:36 2017

NTP: Record TileType metrics also on desktop

BUG= 703165 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2796643002
Cr-Commit-Position: refs/heads/master@{#462804}

[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/search/search_ipc_router.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/search/search_ipc_router.h
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/search/search_ipc_router_unittest.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/search/search_tab_helper.h
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/search/search_tab_helper_unittest.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/common/instant.mojom
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/common/instant.typemap
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/common/instant_struct_traits.h
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/renderer/resources/extensions/searchbox_api.js
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/renderer/searchbox/searchbox.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/renderer/searchbox/searchbox.h
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/chrome/renderer/searchbox/searchbox_extension.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/components/ntp_tiles/metrics.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/components/ntp_tiles/metrics_unittest.cc
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/components/ntp_tiles/tile_visual_type.h
[modify] https://crrev.com/80f5122d11253e8c76379813a5352feb024b9ea2/tools/metrics/histograms/histograms.xml

Comment 7 by treib@chromium.org, Apr 7 2017

Status: Fixed (was: Started)

We now have metrics! Note that they only distinguish "thumbnail" vs "failed (gray) thumbnail"; we can't tell apart local thumbnails from remote (Kodachrome) ones in client metrics. If we need that, we'll have to use server-side metrics.

Comment 8 by rpop@chromium.org, Apr 20 2017

It doesn't look like the histograms mentioned in these commits are recording data on desktop:
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170419&dayCount=1&histograms=NewTabPage.SuggestionsImpression%2CNewTabPage.SuggestionsImpression.IconsColor%2CNewTabPage.SuggestionsImpression.IconsGray%2CNewTabPage.SuggestionsImpression.IconsReal%2CNewTabPage.TileType&fixupData=true&showMax=true&filters=platform%2Cone_of%2CW%7CM%7CL%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

Comment 9 by sfiera@chromium.org, Apr 21 2017

You mean NewTabPage.SuggestionsImpression.Icons{Real,Color,Gray}? This is expected; Desktop doesn't show icons, it shows thumbnails.

Comment 10 by mastiz@chromium.org, Apr 21 2017

I believe rpop@ refers to NewTabPage.TileType which should already be exposing the new buckets on dev channel.

Comment 11 by mastiz@chromium.org, Apr 21 2017

Owner: mastiz@chromium.org
Status: Assigned (was: Fixed)

Found at least one bug, picking this up.

Comment 12 by mastiz@chromium.org, Apr 21 2017

Update: a fix is under review, https://codereview.chromium.org/2832173002/.

Comment 13 by mastiz@chromium.org, Apr 21 2017

Blockedon: 714095

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 26 2017

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

commit 79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1
Author: mastiz <mastiz@chromium.org>
Date: Wed Apr 26 10:30:26 2017

NTP: Fix TileType metrics on desktop

Follow-up to https://codereview.chromium.org/2796643002 where the
metrics were added but an inconsistency was introduced between the C++
enum and the Javascript counterpart. This caused the wrong buckets being
counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail;
ThumbnailServer instead of ThumbnailFailed). The affected buckets
were already deprecated (DeprecatedThumbnailLocal and
DeprecatedThumbnailServer) and hence, luckily, there are no conflated
buckets.

Besides, histograms.xml was missing the new suffixes for
NewTabPageIconTypes, used to provide desktop-specific breakdowns for
NewTabPage.MostVisited and NewTabPage.SuggestionsImpression.

BUG= 714095 , 703165 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2832173002
Cr-Commit-Position: refs/heads/master@{#467287}

[modify] https://crrev.com/79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1/tools/metrics/histograms/histograms.xml

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 27 2017

Labels: merge-merged-3071

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

commit ccbb674bcfe3f1f1395235edd0bb433670a63837
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Apr 27 08:40:54 2017

NTP: Fix TileType metrics on desktop

Follow-up to https://codereview.chromium.org/2796643002 where the
metrics were added but an inconsistency was introduced between the C++
enum and the Javascript counterpart. This caused the wrong buckets being
counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail;
ThumbnailServer instead of ThumbnailFailed). The affected buckets
were already deprecated (DeprecatedThumbnailLocal and
DeprecatedThumbnailServer) and hence, luckily, there are no conflated
buckets.

Besides, histograms.xml was missing the new suffixes for
NewTabPageIconTypes, used to provide desktop-specific breakdowns for
NewTabPage.MostVisited and NewTabPage.SuggestionsImpression.

BUG= 714095 , 703165 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2832173002
Cr-Commit-Position: refs/heads/master@{#467287}
(cherry picked from commit 79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1)

Review-Url: https://codereview.chromium.org/2845863003 .
Cr-Commit-Position: refs/branch-heads/3071@{#261}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/ccbb674bcfe3f1f1395235edd0bb433670a63837/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/ccbb674bcfe3f1f1395235edd0bb433670a63837/tools/metrics/histograms/histograms.xml

Comment 16 by treib@chromium.org, May 4 2017

Status: Fixed (was: Assigned)

Metrics work correctly now, thanks!

Comment 17 by treib@chromium.org, May 4 2017

Blocking: 718413

Comment 18 by treib@chromium.org, May 5 2017

Blocking: 718811

►

Issue 703165 link

Issue metadata

Introduce desktop NTP thumbnail metrics

Issue description

Comment 1 by treib@chromium.org, Mar 29 2017

Comment 1 Deleted

Comment 2 by treib@chromium.org, Mar 30 2017

Comment 2 Deleted

Comment 3 by bugdroid1@chromium.org, Apr 3 2017

Comment 3 Deleted

Comment 4 by bugdroid1@chromium.org, Apr 4 2017

Comment 4 Deleted

Comment 5 by bugdroid1@chromium.org, Apr 4 2017

Comment 5 Deleted

Comment 6 by bugdroid1@chromium.org, Apr 7 2017

Comment 6 Deleted

Comment 7 by treib@chromium.org, Apr 7 2017

Comment 7 Deleted

Comment 8 by rpop@chromium.org, Apr 20 2017

Comment 8 Deleted

Comment 9 by sfiera@chromium.org, Apr 21 2017

Comment 9 Deleted

Comment 10 by mastiz@chromium.org, Apr 21 2017

Comment 10 Deleted

Comment 11 by mastiz@chromium.org, Apr 21 2017

Comment 11 Deleted

Comment 12 by mastiz@chromium.org, Apr 21 2017

Comment 12 Deleted

Comment 13 by mastiz@chromium.org, Apr 21 2017

Comment 13 Deleted

Comment 14 by bugdroid1@chromium.org, Apr 26 2017

Comment 14 Deleted

Comment 15 by bugdroid1@chromium.org, Apr 27 2017

Comment 15 Deleted

Comment 16 by treib@chromium.org, May 4 2017

Comment 16 Deleted

Comment 17 by treib@chromium.org, May 4 2017

Comment 17 Deleted

Comment 18 by treib@chromium.org, May 5 2017

Comment 18 Deleted

Sign in to add a comment