New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 669621 link

Starred by 1 user
Status: Fixed
Owner:
█ joh...@chromium.org
no longer working on chrome
Closed: Dec 2016
Cc:
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Feature



Sign in to add a comment

Split up NotificationImageLoader histograms by image type

Project Member Reported by joh...@chromium.org, Nov 29 2016 
It'll be useful to compare timings and byte sizes for the different image types (content image, icon, badge, action icon). Needed for M56.

Comment 2 by joh...@chromium.org, Dec 1 2016

Labels: Merge-Request-56
Requesting approval to merge 468ed78ec76f5db7d89a0829b33c0f6427553411 to M56. It just adds UMA that was requested in our launch review thread, and is a safe patch with no functional changes.
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 1 2016 

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

commit df48f17f5512fa5a727e63290aded588bc776751
Author: jbroman <jbroman@chromium.org>
Date: Thu Dec 01 16:25:15 2016

Revert "Notifications: Split up image loading histograms by image type"

This reverts commit 468ed78ec76f5db7d89a0829b33c0f6427553411.
https://codereview.chromium.org/2540763002/

Failing on bots.

NotificationImageLoaderTest.SuccessTest (run #1):
[ RUN      ] NotificationImageLoaderTest.SuccessTest
../../base/test/histogram_tester.cc:158: Failure
Value of: actual_count
  Actual: 1
Expected: expected_count
Which is: 0
Histogram "Notifications.LoadFinishTime.Icon" does not have the right number of samples (0) in the expected bucket (0). It has (1).
[  FAILED  ] NotificationImageLoaderTest.SuccessTest (5 ms)

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty/builds/20543

BUG= 669621 , 614456 
TBR=johnme@chromium.org
NOTRY=true

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

[modify] https://crrev.com/df48f17f5512fa5a727e63290aded588bc776751/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
[modify] https://crrev.com/df48f17f5512fa5a727e63290aded588bc776751/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
[modify] https://crrev.com/df48f17f5512fa5a727e63290aded588bc776751/third_party/WebKit/Source/modules/notifications/NotificationImageLoaderTest.cpp
[modify] https://crrev.com/df48f17f5512fa5a727e63290aded588bc776751/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
[modify] https://crrev.com/df48f17f5512fa5a727e63290aded588bc776751/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
[modify] https://crrev.com/df48f17f5512fa5a727e63290aded588bc776751/tools/metrics/histograms/histograms.xml

Comment 4 by joh...@chromium.org, Dec 1 2016

Labels: -Merge-Request-56
Canceling merge request until relanded.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 1 2016 

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

commit 548de683cebfaf09526f22bfa922512a114c251a
Author: johnme <johnme@chromium.org>
Date: Thu Dec 01 20:52:46 2016

Reland: Notifications: Split up image loading histograms by image type

Reland of https://codereview.chromium.org/2540763002 which was reverted
in https://codereview.chromium.org/2540423003. Test has been fixed.

It'll be useful to compare timings and byte sizes for the
different image types (content image, icon, badge, action icon).

BUG= 669621 ,  614456 
TBR=isherman@chromium.org (histograms unchanged from reverted patch)

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

[modify] https://crrev.com/548de683cebfaf09526f22bfa922512a114c251a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
[modify] https://crrev.com/548de683cebfaf09526f22bfa922512a114c251a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
[modify] https://crrev.com/548de683cebfaf09526f22bfa922512a114c251a/third_party/WebKit/Source/modules/notifications/NotificationImageLoaderTest.cpp
[modify] https://crrev.com/548de683cebfaf09526f22bfa922512a114c251a/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
[modify] https://crrev.com/548de683cebfaf09526f22bfa922512a114c251a/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
[modify] https://crrev.com/548de683cebfaf09526f22bfa922512a114c251a/tools/metrics/histograms/histograms.xml

Comment 6 by joh...@chromium.org, Dec 1 2016

Labels: Merge-Request-56
Requesting approval to merge 548de683cebfaf09526f22bfa922512a114c251a to M56. It just adds UMA that was requested in our launch review thread, and is a safe patch with no functional changes.

Comment 7 by dimu@chromium.org, Dec 2 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)

Comment 8 by ligim...@chromium.org, Dec 5 2016 

M56 Beta promotion is scheduled on Dec 7 .Please ensure to verify the fix and merge your change ASAP so that we could take it for next Release.
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 6 2016

Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d91470167eb9d96442978d3bf17632aa5e5c7d05

commit d91470167eb9d96442978d3bf17632aa5e5c7d05
Author: John Mellor <johnme@chromium.org>
Date: Tue Dec 06 11:55:17 2016

Reland: Notifications: Split up image loading histograms by image type

Reland of https://codereview.chromium.org/2540763002 which was reverted
in https://codereview.chromium.org/2540423003. Test has been fixed.

It'll be useful to compare timings and byte sizes for the
different image types (content image, icon, badge, action icon).

BUG= 669621 ,  614456 

Review-Url: https://codereview.chromium.org/2544613004
Cr-Commit-Position: refs/heads/master@{#435705}
(cherry picked from commit 548de683cebfaf09526f22bfa922512a114c251a)

Review URL: https://codereview.chromium.org/2553813003 .

Cr-Commit-Position: refs/branch-heads/2924@{#354}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/d91470167eb9d96442978d3bf17632aa5e5c7d05/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
[modify] https://crrev.com/d91470167eb9d96442978d3bf17632aa5e5c7d05/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
[modify] https://crrev.com/d91470167eb9d96442978d3bf17632aa5e5c7d05/third_party/WebKit/Source/modules/notifications/NotificationImageLoaderTest.cpp
[modify] https://crrev.com/d91470167eb9d96442978d3bf17632aa5e5c7d05/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
[modify] https://crrev.com/d91470167eb9d96442978d3bf17632aa5e5c7d05/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
[modify] https://crrev.com/d91470167eb9d96442978d3bf17632aa5e5c7d05/tools/metrics/histograms/histograms.xml

Comment 10 by joh...@chromium.org, Dec 6 2016

Status: Fixed (was: Started)
►

Sign in to add a comment