Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710953002:
Add metrics for quantity and type of Favicon candidates (Closed)
DescriptionAdd metrics for quantity and type of Favicon candidates
The metrics help us understand what type of icon candidates are
available when the FaviconDriver is queried.
BUG=694956
Review-Url: https://codereview.chromium.org/2710953002
Cr-Commit-Position: refs/heads/master@{#454227}
Committed: https://chromium.googlesource.com/chromium/src/+/b060f62b067b36eaef6106c0e258e1ed45d4f38a
Patch Set 1 #Patch Set 2 : Use UMA_HISTOGRAM_COUNTS_100 and update histograms description #
Total comments: 26
Patch Set 3 : Change histogram descriptions #
Total comments: 2
Patch Set 4 : Adress comments #
Messages
Total messages: 24 (12 generated)
fhorschig@chromium.org changed reviewers: + mastiz@chromium.org
Hi Mikel, could you please take a look before I ask any OWNERs?
fhorschig@chromium.org changed reviewers: + jkrcal@chromium.org - mastiz@chromium.org
-mastiz +jkrcal Hi Jan, could you please have a quick look on the metrics? (Sorry I noticed so late that Mikel is still out for a while)
(non-owners) lgtm https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Watch out for conflicts with Mikel's https://codereview.chromium.org/2697803003/ (not sure who's gonna land first)
fhorschig@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, would you please have a look the first metrics for the driver? https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/02/27 10:20:29, jkrcal wrote: > Watch out for conflicts with Mikel's https://codereview.chromium.org/2697803003/ > > (not sure who's gonna land first) Thank you. Let's hope it's him (although unlikely) because the merge would be way easier that way.
fhorschig@chromium.org changed reviewers: + holte@chromium.org
Hi Steven, would you please take a look at the histograms.xml?
Just nits! https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:137: TEST_F(ContentFaviconDriverTest, RecordsHistorgramsForCandidates) { Nit: define a function local variable kSizes16x16and32x32 with the sizes 16x16 and 32x32. This will make the call on line 154 a bit more legible https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:138: base::HistogramTester tester; Move this initialization after line 158 to where it is used. https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:145: std::vector<gfx::Size>()), Nit: Make use of kEmptyIconSizes that mastiz@ introduces in https://codereview.chromium.org/2697803003/ https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:154: driver_as_observer->DidUpdateFaviconURL(std::vector<content::FaviconURL>( I think that you can do DidUpdateFaviconURL({}) instead of DidUpdateFaviconURL(std::vector<content::FaviconURL>({})) https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:173: tester.ExpectTotalCount("Favicons.CandidatesCount", 3); Isn't this check redundant with assertion on line 174? Count of 3 = count of 1 + count of 2 https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:183: favicon_service()->Shutdown(); Can Shutdown() be called in the TearDown() function? https://codereview.chromium.org/2710953002/diff/20001/components/favicon/core... File components/favicon/core/favicon_driver_impl.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/core... components/favicon/core/favicon_driver_impl.cc:28: UMA_HISTOGRAM_COUNTS_100("Favicons.CandidatesCount", candidates.size()); Nit: Put this metric together with the other metrics at the bottom of the function https://codereview.chromium.org/2710953002/diff/20001/components/favicon/core... components/favicon/core/favicon_driver_impl.cc:43: UMA_HISTOGRAM_COUNTS_100("Favicons.CandidatesWithLargeIconsCount", Nit: Maybe rename to CandidatesWithTouchIconsCount? https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19305: + <summary> How about: "Records the number of favicon <link> tags on a page once the page has finished loading. The count includes the automatically added favicon.ico entry." I think that "number of favicon URLs that are available" is a bit confusing. I think that my version is less confusing. https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19313: + <summary> How about: "Records the number of favicon <link> tags with a non empty sizes attribute once the page has finished loading." https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19321: + <summary> How about: "Records the number of apple-touch-icon and apple-touch-icon-precomposed <link> tags once the page has finished loading."
lgtm
Thank you for the nice descriptions. (<> break git cl format, by the way, so I kept them out of the xml) https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:137: TEST_F(ContentFaviconDriverTest, RecordsHistorgramsForCandidates) { On 2017/02/27 18:54:57, pkotwicz wrote: > Nit: define a function local variable kSizes16x16and32x32 with the sizes 16x16 > and 32x32. This will make the call on line 154 a bit more legible Done. https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:138: base::HistogramTester tester; On 2017/02/27 18:54:57, pkotwicz wrote: > Move this initialization after line 158 to where it is used. Done. https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:145: std::vector<gfx::Size>()), On 2017/02/27 18:54:57, pkotwicz wrote: > Nit: Make use of kEmptyIconSizes that mastiz@ introduces in > https://codereview.chromium.org/2697803003/ Done. https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:154: driver_as_observer->DidUpdateFaviconURL(std::vector<content::FaviconURL>( On 2017/02/27 18:54:57, pkotwicz wrote: > I think that you can do > DidUpdateFaviconURL({}) instead of > DidUpdateFaviconURL(std::vector<content::FaviconURL>({})) Done. https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:173: tester.ExpectTotalCount("Favicons.CandidatesCount", 3); On 2017/02/27 18:54:57, pkotwicz wrote: > Isn't this check redundant with assertion on line 174? Count of 3 = count of 1 + > count of 2 Done. https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:183: favicon_service()->Shutdown(); On 2017/02/27 18:54:57, pkotwicz wrote: > Can Shutdown() be called in the TearDown() function? Gone. It's a mock (due to Mikels CL). https://codereview.chromium.org/2710953002/diff/20001/components/favicon/core... File components/favicon/core/favicon_driver_impl.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/core... components/favicon/core/favicon_driver_impl.cc:28: UMA_HISTOGRAM_COUNTS_100("Favicons.CandidatesCount", candidates.size()); On 2017/02/27 18:54:57, pkotwicz wrote: > Nit: Put this metric together with the other metrics at the bottom of the > function Done. https://codereview.chromium.org/2710953002/diff/20001/components/favicon/core... components/favicon/core/favicon_driver_impl.cc:43: UMA_HISTOGRAM_COUNTS_100("Favicons.CandidatesWithLargeIconsCount", On 2017/02/27 18:54:57, pkotwicz wrote: > Nit: Maybe rename to CandidatesWithTouchIconsCount? Done. https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19305: + <summary> On 2017/02/27 18:54:57, pkotwicz wrote: > How about: "Records the number of favicon <link> tags on a page once the page > has finished loading. The count includes the automatically added favicon.ico > entry." > > I think that "number of favicon URLs that are available" is a bit confusing. I > think that my version is less confusing. Thank you very much! https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19313: + <summary> On 2017/02/27 18:54:57, pkotwicz wrote: > How about: "Records the number of favicon <link> tags with a non empty sizes > attribute once the page has finished loading." Thanks, again. https://codereview.chromium.org/2710953002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19321: + <summary> On 2017/02/27 18:54:57, pkotwicz wrote: > How about: "Records the number of apple-touch-icon and > apple-touch-icon-precomposed <link> tags once the page has finished loading." Thanks
LGTM with nits https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:138: base::HistogramTester tester; Sorry for the confusion. I meant |favicon_urls| https://codereview.chromium.org/2710953002/diff/40001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:29: using testing::Mock; Is testing::Mock used?
https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/20001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:138: base::HistogramTester tester; On 2017/03/01 22:13:37, pkotwicz wrote: > Sorry for the confusion. I meant |favicon_urls| Ah, okay, that makes more sense. https://codereview.chromium.org/2710953002/diff/40001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2710953002/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:29: using testing::Mock; On 2017/03/01 22:13:38, pkotwicz wrote: > Is testing::Mock used? Gone.
The CQ bit was checked by fhorschig@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.
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, holte@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2710953002/#ps60001 (title: "Adress comments")
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": 60001, "attempt_start_ts": 1488450047309330,
"parent_rev": "c463181850479e23e89ebd766fbe1fa2990d9d51", "commit_rev":
"b060f62b067b36eaef6106c0e258e1ed45d4f38a"}
Message was sent while issue was closed.
Description was changed from ========== Add metrics for quantity and type of Favicon candidates The metrics help us understand what type of icon candidates are available when the FaviconDriver is queried. BUG=694956 ========== to ========== Add metrics for quantity and type of Favicon candidates The metrics help us understand what type of icon candidates are available when the FaviconDriver is queried. BUG=694956 Review-Url: https://codereview.chromium.org/2710953002 Cr-Commit-Position: refs/heads/master@{#454227} Committed: https://chromium.googlesource.com/chromium/src/+/b060f62b067b36eaef6106c0e258... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b060f62b067b36eaef6106c0e258... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
