Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762853003:
[Doodle] Add UMA for doodle image downloads (Closed)
|
Created:
3 years, 9 months ago by Marc Treib Modified:
3 years, 9 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Doodle] Add UMA for doodle image downloads
Split by whether the data came from the HTTP cache.
BUG=703227
Review-Url: https://codereview.chromium.org/2762853003
Cr-Commit-Position: refs/heads/master@{#458709}
Committed: https://chromium.googlesource.com/chromium/src/+/85614657faa27811620cfe097a34d38a1c7c2276
Patch Set 1 #
Total comments: 6
Patch Set 2 : better histograms enum, rebase #
Messages
Total messages: 27 (14 generated)
treib@chromium.org changed reviewers: + dgn@chromium.org, justincohen@chromium.org
dgn: Please look at logo_bridge.cc (and overall sanity). justincohen: Please look at search_provider_logos. Thanks!
lgtm
treib@chromium.org changed reviewers: + isherman@chromium.org, tedchoc@chromium.org
+tedchoc for OWNERS approval on logo_bridge.cc +isherman for histograms.xml PTAL, thanks!
https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:367: bool from_http_cache = source->WasCached(); Can we test this?
https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:367: bool from_http_cache = source->WasCached(); On 2017/03/21 15:03:52, justincohen wrote: > Can we test this? What exactly would you want to get tested?
https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:367: bool from_http_cache = source->WasCached(); On 2017/03/21 15:37:00, Marc Treib wrote: > On 2017/03/21 15:03:52, justincohen wrote: > > Can we test this? > > What exactly would you want to get tested? I'm curious if this works on iOS, specifically.
https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2762853003/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:367: bool from_http_cache = source->WasCached(); On 2017/03/21 15:48:52, justincohen wrote: > On 2017/03/21 15:37:00, Marc Treib wrote: > > On 2017/03/21 15:03:52, justincohen wrote: > > > Can we test this? > > > > What exactly would you want to get tested? > > I'm curious if this works on iOS, specifically. "This" being the WasCached() method on URLFetcher? TBH, I just assumed the basic net/ functionality would work on all platforms... In practice, it should always be false here anyway - the /async/newtab_mobile responses shouldn't get cached. It'll get more interesting once we have the ios equivalent of the change to logo_bridge.cc - and I guess then UMA will tell us if it works on ios. I'll also manually check that it works before launching anything on ios. Is that good enough, or were you looking for something else?
That sounds good, thanks. LGTM
On 2017/03/21 15:57:14, justincohen wrote: > That sounds good, thanks. LGTM logo_bridge.cc - lgtm
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics LGTM % a nit: https://codereview.chromium.org/2762853003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2762853003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:41024: +<histogram name="NewTabPage.LogoImageDownloaded" enum="Boolean"> nit: Please use a custom enum with labels like "Downloaded" vs. "Served from cache". (No changes needed in the C++ code -- please just clarify within this file.)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
https://codereview.chromium.org/2762853003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2762853003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:41024: +<histogram name="NewTabPage.LogoImageDownloaded" enum="Boolean"> On 2017/03/21 21:09:54, Ilya Sherman wrote: > nit: Please use a custom enum with labels like "Downloaded" vs. "Served from > cache". (No changes needed in the C++ code -- please just clarify within this > file.) Thanks! Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, isherman@chromium.org, dgn@chromium.org, justincohen@chromium.org Link to the patchset: https://codereview.chromium.org/2762853003/#ps20001 (title: "better histograms enum, rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490178483738790,
"parent_rev": "fa78cf3a4331be2113c320020e10d1223127c125", "commit_rev":
"85614657faa27811620cfe097a34d38a1c7c2276"}
Message was sent while issue was closed.
Description was changed from ========== [Doodle] Add UMA for doodle image downloads Split by whether the data came from the HTTP cache. BUG=703227 ========== to ========== [Doodle] Add UMA for doodle image downloads Split by whether the data came from the HTTP cache. BUG=703227 Review-Url: https://codereview.chromium.org/2762853003 Cr-Commit-Position: refs/heads/master@{#458709} Committed: https://chromium.googlesource.com/chromium/src/+/85614657faa27811620cfe097a34... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/85614657faa27811620cfe097a34... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
