Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(43)

Issue 2796643002: NTP: Record TileType metrics also on desktop (Closed)

Created:
2 years, 2 months ago by Marc Treib
Modified:
2 years, 2 months ago
CC:
chromium-reviews, skanuj+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, noyau+watch_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, melevin+watch_chromium.org, abarth-chromium, chromium-apps-reviews_chromium.org, pedrosimonetti+watch_chromium.org, dbeam+watch-ntp_chromium.org, ntp-dev+reviews_chromium.org, jfweitz+watch_chromium.org, asvitkine+watch_chromium.org, Jered, donnd+watch_chromium.org, Aaron Boodman, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, darin (slow to review), mastiz
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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} Committed: https://chromium.googlesource.com/chromium/src/+/80f5122d11253e8c76379813a5352feb024b9ea2

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : Tests! #

Total comments: 8

Patch Set 4 : review #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+508 lines, -204 lines) Patch
M chrome/browser/resources/local_ntp/most_visited_single.js View 7 chunks +47 lines, -13 lines 0 comments Download
M chrome/browser/ui/search/search_ipc_router.h View 3 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/ui/search/search_ipc_router.cc View 3 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/ui/search/search_ipc_router_unittest.cc View 1 2 31 chunks +96 lines, -68 lines 0 comments Download
M chrome/browser/ui/search/search_tab_helper.h View 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/ui/search/search_tab_helper.cc View 1 chunk +8 lines, -6 lines 0 comments Download
M chrome/browser/ui/search/search_tab_helper_unittest.cc View 1 2 1 chunk +8 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/ntp/ntp_user_data_logger.h View 3 chunks +9 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc View 3 chunks +9 lines, -6 lines 0 comments Download
M chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc View 1 2 14 chunks +142 lines, -30 lines 0 comments Download
M chrome/common/instant.mojom View 2 chunks +7 lines, -2 lines 0 comments Download
M chrome/common/instant.typemap View 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/common/instant_struct_traits.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/renderer/resources/extensions/searchbox_api.js View 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/renderer/searchbox/searchbox.h View 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/renderer/searchbox/searchbox.cc View 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/renderer/searchbox/searchbox_extension.cc View 1 2 3 3 chunks +17 lines, -7 lines 0 comments Download
M components/ntp_tiles/metrics.cc View 1 2 3 5 chunks +31 lines, -26 lines 0 comments Download
M components/ntp_tiles/metrics_unittest.cc View 1 2 3 7 chunks +74 lines, -10 lines 0 comments Download
M components/ntp_tiles/tile_visual_type.h View 1 2 3 1 chunk +9 lines, -6 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 2 chunks +10 lines, -4 lines 1 comment Download

Messages

Total messages: 32 (15 generated)
Marc Treib
This is not yet ready for submission (no tests etc), but I'm looking for some ...
2 years, 2 months ago (2017-04-03 14:53:01 UTC) #3
sfiera
On 2017/04/03 14:53:01, Marc Treib wrote: > This is not yet ready for submission (no ...
2 years, 2 months ago (2017-04-03 15:00:45 UTC) #4
Marc Treib
On 2017/04/03 15:00:45, sfiera wrote: > On 2017/04/03 14:53:01, Marc Treib wrote: > > This ...
2 years, 2 months ago (2017-04-03 15:08:29 UTC) #5
mastiz
On 2017/04/03 14:53:01, Marc Treib wrote: > This is not yet ready for submission (no ...
2 years, 2 months ago (2017-04-03 15:26:31 UTC) #6
sfiera
On 2017/04/03 15:08:29, Marc Treib wrote: > On 2017/04/03 15:00:45, sfiera wrote: > > On ...
2 years, 2 months ago (2017-04-03 15:27:44 UTC) #7
Marc Treib
On 2017/04/03 15:27:44, sfiera wrote: > On 2017/04/03 15:08:29, Marc Treib wrote: > > On ...
2 years, 2 months ago (2017-04-04 08:05:36 UTC) #8
Marc Treib
Tests are here now. sfiera@, please review! :)
2 years, 2 months ago (2017-04-04 08:56:06 UTC) #11
Marc Treib
(mastiz to CC)
2 years, 2 months ago (2017-04-04 08:56:56 UTC) #13
Marc Treib
Ping!
2 years, 2 months ago (2017-04-05 14:22:35 UTC) #16
sfiera
https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchbox/searchbox_extension.cc File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchbox/searchbox_extension.cc#newcode1007 chrome/renderer/searchbox/searchbox_extension.cc:1007: args[2]->Uint32Value() <= ntp_tiles::MAX_RECORDED_TILE_TYPE) { Can we instead check here ...
2 years, 2 months ago (2017-04-06 08:53:28 UTC) #17
Marc Treib
Comments addressed, PTAL again! https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchbox/searchbox_extension.cc File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchbox/searchbox_extension.cc#newcode1007 chrome/renderer/searchbox/searchbox_extension.cc:1007: args[2]->Uint32Value() <= ntp_tiles::MAX_RECORDED_TILE_TYPE) { On ...
2 years, 2 months ago (2017-04-06 09:16:44 UTC) #18
sfiera
LGTM
2 years, 2 months ago (2017-04-06 09:24:12 UTC) #21
Marc Treib
+rsesek for IPC (instant.mojom, instant.typemap, instant_struct_traits.h) +isherman for histograms.xml PTAL! https://codereview.chromium.org/2796643002/diff/60001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2796643002/diff/60001/tools/metrics/histograms/histograms.xml#newcode42408 ...
2 years, 2 months ago (2017-04-06 09:35:35 UTC) #23
Robert Sesek
IPC LGTM
2 years, 2 months ago (2017-04-06 15:06:21 UTC) #26
Ilya Sherman
histograms.xml lgtm
2 years, 2 months ago (2017-04-06 19:39:15 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2796643002/60001
2 years, 2 months ago (2017-04-07 07:54:53 UTC) #29
commit-bot: I haz the power
2 years, 2 months ago (2017-04-07 08:00:40 UTC) #32
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/80f5122d11253e8c76379813a535...

Powered by Google App Engine
This is Rietveld 408576698