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

Issue 2781093002: Add attempt count metric to FaviconHandler (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 4 weeks ago by fhorschig
Modified:
3 months, 1 week ago
CC:
chromium-reviews, asvitkine+watch_chromium.org, mastiz
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add attempt count metric to FaviconHandler The histograms this metric introduces aggregates how many download attempts were needed to determine the best-fitting favicon. This is stored in two metrics in order to separate requests for large icons. BUG=694956 Review-Url: https://codereview.chromium.org/2781093002 Cr-Commit-Position: refs/heads/master@{#465526} Committed: https://chromium.googlesource.com/chromium/src/+/a9fb2c3cf6bc9b4877d9c21f9f12094fb5c26ecd

Patch Set 1 #

Total comments: 2

Patch Set 2 : Add third metric #

Patch Set 3 : Require metric for every notification icon type #

Total comments: 19

Patch Set 4 : Record 404s as attempts and add separate tests for recording #

Patch Set 5 : Readded a different metric for touch icons #

Total comments: 12

Patch Set 6 : Use histogram suffix instead of completely new histogram #

Patch Set 7 : Use dedicated variable for request count and simplify tests #

Total comments: 6

Patch Set 8 : Verify expired results are recorded exactly once. #

Total comments: 10

Patch Set 9 : Correct test and restrict histogram #

Total comments: 2

Patch Set 10 : Use public sparse_slowly #

Unified diffs Side-by-side diffs Delta from patch set Stats (+184 lines, -11 lines) Patch
M components/favicon/core/favicon_handler.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M components/favicon/core/favicon_handler.cc View 1 2 3 4 5 6 7 8 9 9 chunks +35 lines, -0 lines 0 comments Download
M components/favicon/core/favicon_handler_unittest.cc View 1 2 3 4 5 6 7 8 6 chunks +128 lines, -11 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 2 chunks +17 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 48 (20 generated)
fhorschig
Hi Jan, Is this metric something you would consider useful? (You proposed it a while ...
3 months, 4 weeks ago (2017-03-29 13:51:41 UTC) #2
jkrcal
Yep, this CL seems useful, indeed! https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/favicon_handler.cc#newcode481 components/favicon/core/favicon_handler.cc:481: UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", Can we ...
3 months, 4 weeks ago (2017-03-30 07:53:28 UTC) #3
fhorschig
https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/favicon_handler.cc#newcode481 components/favicon/core/favicon_handler.cc:481: UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", On 2017/03/30 07:53:28, jkrcal wrote: > Can we ...
3 months, 3 weeks ago (2017-04-03 09:56:49 UTC) #4
jkrcal
On 2017/04/03 09:56:49, fhorschig wrote: > https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/favicon_handler.cc > File components/favicon/core/favicon_handler.cc (right): > > https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/favicon_handler.cc#newcode481 > ...
3 months, 3 weeks ago (2017-04-03 10:12:58 UTC) #5
fhorschig
On 2017/04/03 10:12:58, jkrcal wrote: > On 2017/04/03 09:56:49, fhorschig wrote: > > > https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/favicon_handler.cc ...
3 months, 3 weeks ago (2017-04-03 13:11:52 UTC) #6
jkrcal
lgtm, thanks!
3 months, 3 weeks ago (2017-04-03 17:25:44 UTC) #11
fhorschig
Hi Peter, would you please have a look at the new metrics for recording the ...
3 months, 3 weeks ago (2017-04-04 08:05:35 UTC) #13
pkotwicz
https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core/favicon_handler.cc#newcode76 components/favicon/core/favicon_handler.cc:76: return; Can we use the same histogram for Favicons.LargeIconDownloadAttempts ...
3 months, 3 weeks ago (2017-04-04 13:04:38 UTC) #14
pkotwicz
https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core/favicon_handler.cc#newcode76 components/favicon/core/favicon_handler.cc:76: return; Can we use the same histogram for Favicons.LargeIconDownloadAttempts ...
3 months, 3 weeks ago (2017-04-04 13:04:38 UTC) #15
fhorschig
https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core/favicon_handler.cc#newcode76 components/favicon/core/favicon_handler.cc:76: return; On 2017/04/04 13:04:37, pkotwicz wrote: > Can we ...
3 months, 2 weeks ago (2017-04-10 09:55:50 UTC) #16
fhorschig
+mastiz (CC) as OWNER of the IconType consolidation CL I readded the third bucket for ...
3 months, 2 weeks ago (2017-04-10 13:59:41 UTC) #17
pkotwicz
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc#newcode525 components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); Why are we recording Favicons.FaviconDownloadAttempts from here?
3 months, 2 weeks ago (2017-04-10 14:58:11 UTC) #18
fhorschig
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc#newcode525 components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); On 2017/04/10 14:58:11, pkotwicz wrote: > Why ...
3 months, 2 weeks ago (2017-04-10 15:05:21 UTC) #19
pkotwicz
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc#newcode525 components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); Sorry for the confusion earlier. I think ...
3 months, 2 weeks ago (2017-04-10 17:26:10 UTC) #20
fhorschig
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core/favicon_handler.cc#newcode525 components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); On 2017/04/10 17:26:10, pkotwicz wrote: > Sorry ...
3 months, 2 weeks ago (2017-04-11 12:07:22 UTC) #21
pkotwicz
https://codereview.chromium.org/2781093002/diff/120001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/120001/components/favicon/core/favicon_handler.cc#newcode63 components/favicon/core/favicon_handler.cc:63: void RecordAttemptsForHandlerType( Maybe rename this method to: RecordDownloadAttemptsForHandlerType() https://codereview.chromium.org/2781093002/diff/120001/components/favicon/core/favicon_handler.cc#newcode426 ...
3 months, 2 weeks ago (2017-04-12 02:48:21 UTC) #22
pkotwicz
3 months, 2 weeks ago (2017-04-12 02:48:22 UTC) #23
fhorschig
https://codereview.chromium.org/2781093002/diff/120001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/120001/components/favicon/core/favicon_handler.cc#newcode63 components/favicon/core/favicon_handler.cc:63: void RecordAttemptsForHandlerType( On 2017/04/12 02:48:20, pkotwicz wrote: > Maybe ...
3 months, 2 weeks ago (2017-04-12 08:32:40 UTC) #24
pkotwicz
https://codereview.chromium.org/2781093002/diff/140001/components/favicon/core/favicon_handler_unittest.cc File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/140001/components/favicon/core/favicon_handler_unittest.cc#newcode1141 components/favicon/core/favicon_handler_unittest.cc:1141: kPageURL, kIconURL16x16, In order to trigger the error case ...
3 months, 2 weeks ago (2017-04-12 14:39:24 UTC) #25
pkotwicz
LGTM with nit
3 months, 2 weeks ago (2017-04-12 14:39:37 UTC) #26
pkotwicz
LGTM with nit in comment #25
3 months, 2 weeks ago (2017-04-12 14:39:59 UTC) #27
fhorschig
Hi Ilya, could you please also take a look at this metric? It records the ...
3 months, 2 weeks ago (2017-04-12 15:33:10 UTC) #29
Ilya Sherman (Away til Aug 14)
https://codereview.chromium.org/2781093002/diff/140001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/140001/components/favicon/core/favicon_handler.cc#newcode69 components/favicon/core/favicon_handler.cc:69: return; How many download attempts do you expect to ...
3 months, 2 weeks ago (2017-04-12 20:43:03 UTC) #30
fhorschig
https://codereview.chromium.org/2781093002/diff/140001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/140001/components/favicon/core/favicon_handler.cc#newcode69 components/favicon/core/favicon_handler.cc:69: return; On 2017/04/12 20:43:03, Ilya Sherman wrote: > How ...
3 months, 2 weeks ago (2017-04-13 14:49:35 UTC) #31
Ilya Sherman (Away til Aug 14)
Thanks. Metrics LGTM % a final comment: https://codereview.chromium.org/2781093002/diff/160001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/160001/components/favicon/core/favicon_handler.cc#newcode72 components/favicon/core/favicon_handler.cc:72: INTERNAL_HISTOGRAM_SPARSE_SLOWLY("Favicons.DownloadAttempts.Favicons", Please ...
3 months, 1 week ago (2017-04-13 21:17:59 UTC) #36
fhorschig
https://codereview.chromium.org/2781093002/diff/160001/components/favicon/core/favicon_handler.cc File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/160001/components/favicon/core/favicon_handler.cc#newcode72 components/favicon/core/favicon_handler.cc:72: INTERNAL_HISTOGRAM_SPARSE_SLOWLY("Favicons.DownloadAttempts.Favicons", On 2017/04/13 21:17:59, Ilya Sherman wrote: > Please ...
3 months, 1 week ago (2017-04-18 15:15:26 UTC) #37
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/2781093002/180001
3 months, 1 week ago (2017-04-19 07:56:21 UTC) #44
commit-bot: I haz the power
3 months, 1 week ago (2017-04-19 08:02:56 UTC) #47
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a9fb2c3cf6bc9b4877d9c21f9f12...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973