Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222573003:
Add use counter for chrome.loadTimes (Closed)
|
Created:
4 years, 4 months ago by panicker Modified:
4 years, 4 months ago CC:
Bryan McQuade, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, dglazkov+blink, extensions-reviews_chromium.org, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd use counter for chrome.loadTimes
BUG=621512
Committed: https://crrev.com/3fcbfceb613b77295f543c55a9147b3343986571
Cr-Commit-Position: refs/heads/master@{#410427}
Patch Set 1 #
Total comments: 9
Patch Set 2 : address review comments #
Messages
Total messages: 33 (15 generated)
panicker@chromium.org changed reviewers: + esprehn@chromium.org
Hey Elliot, This plumbing is based on our conversation a couple weeks ago. Does this look sane?
https://codereview.chromium.org/2222573003/diff/1/chrome/renderer/loadtimes_e... File chrome/renderer/loadtimes_extension_bindings.cc (right): https://codereview.chromium.org/2222573003/diff/1/chrome/renderer/loadtimes_e... chrome/renderer/loadtimes_extension_bindings.cc:113: if (frame) { I'd usually write: if (WebLocalFrame* frame = WebLocalFrame::frameForCurrentContext()) { // ... } but you can do this if you want :) https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/UseCounter.h:1274: ChromeLoadtimesConnectionInfo = 1498, So crazy we're this high on UseCounters now, I remember when we had like 200 of them. :) https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2164: if (metric == "startLoadTime") { else if, otherwise you're doing tons of comparisons on the way down this list when it can only match a single thing. https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebLocalFrame.h:427: virtual void usageCountChromeLoadtimes(const WebString& metric) = 0; LoadTimes, the api is loadTimes with a caps T :)
Thanks for the review. PTAL. https://codereview.chromium.org/2222573003/diff/1/chrome/renderer/loadtimes_e... File chrome/renderer/loadtimes_extension_bindings.cc (right): https://codereview.chromium.org/2222573003/diff/1/chrome/renderer/loadtimes_e... chrome/renderer/loadtimes_extension_bindings.cc:113: if (frame) { On 2016/08/05 20:57:36, esprehn wrote: > I'd usually write: > > if (WebLocalFrame* frame = WebLocalFrame::frameForCurrentContext()) { > // ... > } > > but you can do this if you want :) Sure, updated. https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/UseCounter.h:1274: ChromeLoadtimesConnectionInfo = 1498, On 2016/08/05 20:57:36, esprehn wrote: > So crazy we're this high on UseCounters now, I remember when we had like 200 of > them. :) Interesting. Curious - is deprecation the primary usecase here? If so I'd expect the list to grow, but also shrink sometimes :) https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2164: if (metric == "startLoadTime") { On 2016/08/05 20:57:36, esprehn wrote: > else if, otherwise you're doing tons of comparisons on the way down this list > when it can only match a single thing. Done. https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebLocalFrame.h:427: virtual void usageCountChromeLoadtimes(const WebString& metric) = 0; On 2016/08/05 20:57:36, esprehn wrote: > LoadTimes, the api is loadTimes with a caps T :) Good point :) updated.
lgtm https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/UseCounter.h:1274: ChromeLoadtimesConnectionInfo = 1498, On 2016/08/05 at 23:06:14, Shubhie wrote: > On 2016/08/05 20:57:36, esprehn wrote: > > So crazy we're this high on UseCounters now, I remember when we had like 200 of > > them. :) > > Interesting. > Curious - is deprecation the primary usecase here? If so I'd expect the list to grow, but also shrink sometimes :) The list might shrink, but the numbers can never go down or it'd mess of the historical UMA data. This enum will grow to infinity :)
Description was changed from ========== Add use counter for chrome.loadtimes BUG=621512 ========== to ========== Add use counter for chrome.loadTimes BUG=621512 ==========
Description was changed from ========== Add use counter for chrome.loadTimes BUG=621512 ========== to ========== Add use counter for chrome.loadTimes BUG=621512 ==========
panicker@chromium.org changed reviewers: + bmcquade@chromium.org
On 2016/08/05 23:07:53, esprehn wrote: > lgtm > > https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/UseCounter.h (right): > > https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/UseCounter.h:1274: > ChromeLoadtimesConnectionInfo = 1498, > On 2016/08/05 at 23:06:14, Shubhie wrote: > > On 2016/08/05 20:57:36, esprehn wrote: > > > So crazy we're this high on UseCounters now, I remember when we had like 200 > of > > > them. :) > > > > Interesting. > > Curious - is deprecation the primary usecase here? If so I'd expect the list > to grow, but also shrink sometimes :) > > The list might shrink, but the numbers can never go down or it'd mess of the > historical UMA data. This enum will grow to infinity :) I've asked about this in the past for other things, but it seems weird to plumb these stats into Blink, when Blink just ends up sending them back to content.
On 2016/08/07 00:16:50, dcheng (OOO Aug 1 - Aug 11) wrote: > On 2016/08/05 23:07:53, esprehn wrote: > > lgtm > > > > > https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/frame/UseCounter.h (right): > > > > > https://codereview.chromium.org/2222573003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/frame/UseCounter.h:1274: > > ChromeLoadtimesConnectionInfo = 1498, > > On 2016/08/05 at 23:06:14, Shubhie wrote: > > > On 2016/08/05 20:57:36, esprehn wrote: > > > > So crazy we're this high on UseCounters now, I remember when we had like > 200 > > of > > > > them. :) > > > > > > Interesting. > > > Curious - is deprecation the primary usecase here? If so I'd expect the list > > to grow, but also shrink sometimes :) > > > > The list might shrink, but the numbers can never go down or it'd mess of the > > historical UMA data. This enum will grow to infinity :) > > I've asked about this in the past for other things, but it seems weird to plumb > these stats into Blink, when Blink just ends up sending them back to content. Actually, it looks like this is a v8::Extension: I thought we were going to get rid of those?
We are, this project is actually trying to delete this entire api, so doing this funny thing seemed fine. It's weird to use a non UseCounter UMA outside blink for this purpose. It wouldn't show up in the chromestatus dashboard for example to go along with the Intent to Remove. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
We are, this project is actually trying to delete this entire api, so doing this funny thing seemed fine. It's weird to use a non UseCounter UMA outside blink for this purpose. It wouldn't show up in the chromestatus dashboard for example to go along with the Intent to Remove. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/07 05:24:03, esprehn wrote: > We are, this project is actually trying to delete this entire api, so doing > this funny thing seemed fine. It's weird to use a non UseCounter UMA > outside blink for this purpose. It wouldn't show up in the chromestatus > dashboard for example to go along with the Intent to Remove. > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I see, thanks. LGTM in that ase. Historically, having to do this has been a sign that this might be a feature might be a candidate for implementing within Blink instead (like window.external): is that something that would be an option for this API if deprecation turns out to be impossible for some reason?
On 2016/08/07 13:47:13, dcheng (OOO Aug 1 - Aug 11) wrote: > On 2016/08/07 05:24:03, esprehn wrote: > > We are, this project is actually trying to delete this entire api, so doing > > this funny thing seemed fine. It's weird to use a non UseCounter UMA > > outside blink for this purpose. It wouldn't show up in the chromestatus > > dashboard for example to go along with the Intent to Remove. > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > I see, thanks. LGTM in that ase. > > Historically, having to do this has been a sign that this might be a feature > might be a candidate for implementing within Blink instead (like > window.external): is that something that would be an option for this API if > deprecation turns out to be impossible for some reason? Thanks for the comments. Yes, if deprecation turns out to be impossible then this should be rejiggered to move within Blink (if that's fine with folks). I think we can deprecate many of the loadtimes values but it's certainly possible there are some fields that prove difficult to remove.
The CQ bit was checked by panicker@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 panicker@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
panicker@chromium.org changed reviewers: + isherman@chromium.org
Bryan, Ilya -- could you please review for OWNERS for loadtimes and histograms respectively? Thank you!
bmcquade@chromium.org changed reviewers: + dcheng@chromium.org
LGTM for loadtimes_extension_bindings, thanks!
metrics lgtm
The CQ bit was checked by panicker@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add use counter for chrome.loadTimes BUG=621512 ========== to ========== Add use counter for chrome.loadTimes BUG=621512 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add use counter for chrome.loadTimes BUG=621512 ========== to ========== Add use counter for chrome.loadTimes BUG=621512 Committed: https://crrev.com/3fcbfceb613b77295f543c55a9147b3343986571 Cr-Commit-Position: refs/heads/master@{#410427} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3fcbfceb613b77295f543c55a9147b3343986571 Cr-Commit-Position: refs/heads/master@{#410427} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
