Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1904293003:
Restore the histograms for notification resource loading. (Closed)
|
Created:
4 years, 8 months ago by Michael van Ouwerkerk Modified:
4 years, 7 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews, blink-reviews, haraken, Peter Beverloo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestore the histograms for notification resource loading.
These were lost when this functionality was ported from content to blink.
* Originally defined in https://codereview.chromium.org/1715763002
* Notifications.Icon.LoadFinishTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration.
* Notifications.Icon.LoadFailTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration.
* Notifications.Icon.ScaleDownTime was originally defined using SCOPED_UMA_HISTOGRAM_TIMER so matches that configuration.
BUG=592188
Committed: https://crrev.com/1cdaa38724146a00b299b52fc2fc226049a3e363
Cr-Commit-Position: refs/heads/master@{#392943}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address peter's comments. #Patch Set 3 : Rebase. #Patch Set 4 : Rebase. #
Messages
Total messages: 27 (15 generated)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904293003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904293003/1
mvanouwerkerk@chromium.org changed reviewers: + peter@chromium.org
Hi Peter, I heard you like macros...
lgtm Please refer to https://codereview.chromium.org/1715763002 in your CLs description. One thing I'd like you to consider is whether it would be worthwhile to have a NotificationHistogramHelper that unifies interaction with UMA. There's a lot of magic numbers in there. https://codereview.chromium.org/1904293003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/1904293003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:88: // Matches UMA_HISTOGRAM_LONG_TIMES - the way Notifications.Icon.LoadFinishTime was originally defined. Here and elsewhere - "the way Notifications.Icon.LoadFinishTime was originally defined." I think these comments are more valuable in the CL description than in the code. From the code's point of view, it doesn't matter where or how it originally was defined.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Peter. https://codereview.chromium.org/1904293003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/1904293003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:88: // Matches UMA_HISTOGRAM_LONG_TIMES - the way Notifications.Icon.LoadFinishTime was originally defined. On 2016/04/22 15:56:13, Peter Beverloo wrote: > Here and elsewhere - > "the way Notifications.Icon.LoadFinishTime was originally defined." > > I think these comments are more valuable in the CL description than in the code. > From the code's point of view, it doesn't matter where or how it originally was > defined. Done.
Description was changed from ========== Restore the histograms for notification resource loading. These were lost when this functionality was ported from content to blink. BUG=592188 ========== to ========== Restore the histograms for notification resource loading. These were lost when this functionality was ported from content to blink. * Originally defined in https://codereview.chromium.org/1715763002 * Notifications.Icon.LoadFinishTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.LoadFailTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.ScaleDownTime was originally defined using SCOPED_UMA_HISTOGRAM_TIMER so matches that configuration. BUG=592188 ==========
The CQ bit was checked by mvanouwerkerk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1904293003/#ps20001 (title: "Address peter's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904293003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904293003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:
While running git apply --index -3 -p1;
error: patch failed:
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:30
Falling back to three-way merge...
Applied patch to
'third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp'
with conflicts.
U third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
Patch:
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
Index:
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
diff --git
a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
index
dbd0919a44ec72f30f1d20b6e886a00e82e7e178..5354ea387f8bab58c85d71fcf0fa4a3aa367b581
100644
---
a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
+++
b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
@@ -6,6 +6,7 @@
#include "core/dom/ExecutionContext.h"
#include "core/fetch/ResourceLoaderOptions.h"
+#include "platform/Histogram.h"
#include "platform/image-decoders/ImageDecoder.h"
#include "platform/image-decoders/ImageFrame.h"
#include "platform/network/ResourceError.h"
@@ -14,11 +15,12 @@
#include "platform/weborigin/KURL.h"
#include "public/platform/WebURLRequest.h"
#include "third_party/skia/include/core/SkBitmap.h"
+#include "wtf/CurrentTime.h"
namespace blink {
NotificationImageLoader::NotificationImageLoader()
- : m_stopped(false)
+ : m_stopped(false), m_startTime(0.0)
{
}
@@ -30,6 +32,7 @@ void NotificationImageLoader::start(ExecutionContext*
executionContext, const KU
{
DCHECK(!m_stopped);
+ m_startTime = monotonicallyIncreasingTimeMS();
m_imageCallback = imageCallback;
// TODO(mvanouwerkerk): Add a timeout mechanism: crbug.com/579137.
@@ -82,7 +85,13 @@ void NotificationImageLoader::didFinishLoading(unsigned long
resourceIdentifier,
if (m_stopped)
return;
+ DEFINE_STATIC_LOCAL(CustomCountHistogram, finishedTimeHistogram,
("Notifications.Icon.LoadFinishTime", 1, 1000 * 60 * 60 /* 1 hour max */, 50 /*
buckets */));
+ finishedTimeHistogram.count(monotonicallyIncreasingTimeMS() - m_startTime);
+
if (m_data) {
+ DEFINE_STATIC_LOCAL(CustomCountHistogram, fileSizeHistogram,
("Notifications.Icon.FileSize", 1, 10000000 /* ~10mb max */, 50 /* buckets */));
+ fileSizeHistogram.count(m_data->size());
+
OwnPtr<ImageDecoder> decoder = ImageDecoder::create(*m_data.get(),
ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied);
if (decoder) {
decoder->setData(m_data.get(), true /* allDataReceived */);
@@ -99,6 +108,9 @@ void NotificationImageLoader::didFinishLoading(unsigned long
resourceIdentifier,
void NotificationImageLoader::didFail(const ResourceError& error)
{
+ DEFINE_STATIC_LOCAL(CustomCountHistogram, failedTimeHistogram,
("Notifications.Icon.LoadFailTime", 1, 1000 * 60 * 60 /* 1 hour max */, 50 /*
buckets */));
+ failedTimeHistogram.count(monotonicallyIncreasingTimeMS() - m_startTime);
+
runCallbackWithEmptyBitmap();
}
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by mvanouwerkerk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1904293003/#ps40001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904293003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904293003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by mvanouwerkerk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1904293003/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904293003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904293003/60001
Message was sent while issue was closed.
Description was changed from ========== Restore the histograms for notification resource loading. These were lost when this functionality was ported from content to blink. * Originally defined in https://codereview.chromium.org/1715763002 * Notifications.Icon.LoadFinishTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.LoadFailTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.ScaleDownTime was originally defined using SCOPED_UMA_HISTOGRAM_TIMER so matches that configuration. BUG=592188 ========== to ========== Restore the histograms for notification resource loading. These were lost when this functionality was ported from content to blink. * Originally defined in https://codereview.chromium.org/1715763002 * Notifications.Icon.LoadFinishTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.LoadFailTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.ScaleDownTime was originally defined using SCOPED_UMA_HISTOGRAM_TIMER so matches that configuration. BUG=592188 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Restore the histograms for notification resource loading. These were lost when this functionality was ported from content to blink. * Originally defined in https://codereview.chromium.org/1715763002 * Notifications.Icon.LoadFinishTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.LoadFailTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.ScaleDownTime was originally defined using SCOPED_UMA_HISTOGRAM_TIMER so matches that configuration. BUG=592188 ========== to ========== Restore the histograms for notification resource loading. These were lost when this functionality was ported from content to blink. * Originally defined in https://codereview.chromium.org/1715763002 * Notifications.Icon.LoadFinishTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.LoadFailTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration. * Notifications.Icon.ScaleDownTime was originally defined using SCOPED_UMA_HISTOGRAM_TIMER so matches that configuration. BUG=592188 Committed: https://crrev.com/1cdaa38724146a00b299b52fc2fc226049a3e363 Cr-Commit-Position: refs/heads/master@{#392943} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1cdaa38724146a00b299b52fc2fc226049a3e363 Cr-Commit-Position: refs/heads/master@{#392943} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
