Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2084583002:
Update chrome.loadTimes() to use timings from WebPerformance. (Closed)
|
Created:
4 years, 6 months ago by Bryan McQuade Modified:
4 years, 5 months ago Reviewers:
Pat Meenan CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate chrome.loadTimes() to use timings from WebPerformance.
Update chrome.loadTimes() to use timings from WebPerformance, which backs the
standardized navigation timing API, instead of the deprecated DocumentState
timings.
BUG=621512
Committed: https://crrev.com/0d38f4e0ac28e2af5feec1fe93085037f9eb8d45
Cr-Commit-Position: refs/heads/master@{#403570}
Patch Set 1 #Patch Set 2 : use first contentful paint #Patch Set 3 : revert to old start_load_time. #Patch Set 4 : fix #
Total comments: 10
Patch Set 5 : address pmeenan comments #Patch Set 6 : address comments #Patch Set 7 : address comments #Patch Set 8 : address comment #Messages
Total messages: 11 (4 generated)
bmcquade@chromium.org changed reviewers: + pmeenan@chromium.org
PTAL
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... File chrome/renderer/loadtimes_extension_bindings.cc (right): https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:147: double commit_load_time = document_state->commit_load_time().ToDoubleT(); Maybe use web_performance.responseStart() since that's functionally what it is and would keep everything consistent and tied to the monotonic clock. https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:149: web_performance.domContentLoadedEventEnd(); Sanity check and make sure this should be the end and not the start (i.e. includes the duration of script execution). Looking at the implementation of finish_document_load_time() it looks like the time was set before the notifications are passed to the observers. https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:150: double finish_load_time = web_performance.loadEventEnd(); Same as with DCL, I think this needs to be tied to the start instead of the end to match existing behavior. https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:151: double first_paint_time = web_performance.firstContentfulPaint(); Shouldn't this be firstPaint() to keep it consistent with existing behavior (understanding that we may be trying to push sites to look at contentful paint instead, not sure we want to switch it out from under them). https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:156: document_state->first_paint_after_load_time().ToDoubleT(); Can we just zero this out instead (or remove it)? The clock used by first_paint_after_load_time() is Time::now() and not the same monotonic clock as the rest of the timers.
Thanks for the review! Addressed comments. https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... File chrome/renderer/loadtimes_extension_bindings.cc (right): https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:147: double commit_load_time = document_state->commit_load_time().ToDoubleT(); On 2016/06/27 at 16:23:39, Pat Meenan wrote: > Maybe use web_performance.responseStart() since that's functionally what it is and would keep everything consistent and tied to the monotonic clock. Agree. Done. https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:149: web_performance.domContentLoadedEventEnd(); On 2016/06/27 at 16:23:39, Pat Meenan wrote: > Sanity check and make sure this should be the end and not the start (i.e. includes the duration of script execution). Looking at the implementation of finish_document_load_time() it looks like the time was set before the notifications are passed to the observers. Yes, good suggestion - I had eyeballed which of start or end was closer in the timings but hadn't actually traced the calls to see which was right. Here's what I found: The existing code sets finish_document_load_time in RenderFrameImpl::didFinishDocumentLoad: https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?dr... didFinishDocumentLoad is called via FrameLoader::finishedParsing which is called from Document::finishedParsing: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... This is slightly after m_documentTiming.markDomContentLoadedEventEnd(); is called, earlier in Document::finishedParsing, so using domContentLoadedEventEnd seems right here. https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:150: double finish_load_time = web_performance.loadEventEnd(); On 2016/06/27 at 16:23:39, Pat Meenan wrote: > Same as with DCL, I think this needs to be tied to the start instead of the end to match existing behavior. Here's the trace for this one: finish_load_time is set in RenderFrameImpl::didFinishLoad, which gets called via FrameLoader::checkCompleted: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... This call is shortly after the load event gets fired, above, at the call to m_frame->document()->implicitClose(), which eventually fires both load event start and end, via this->domWindow()->documentWasClosed() LocalDOMWindow::dispatchWindowLoadEvent() LocalDOMWindow::dispatchLoadEvent(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc... So it looks like loadEventEnd is a better match than loadEventStart. https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:151: double first_paint_time = web_performance.firstContentfulPaint(); On 2016/06/27 at 16:23:39, Pat Meenan wrote: > Shouldn't this be firstPaint() to keep it consistent with existing behavior (understanding that we may be trying to push sites to look at contentful paint instead, not sure we want to switch it out from under them). Yep, agree. Updated to firstPaint(). https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... chrome/renderer/loadtimes_extension_bindings.cc:156: document_state->first_paint_after_load_time().ToDoubleT(); On 2016/06/27 at 16:23:39, Pat Meenan wrote: > Can we just zero this out instead (or remove it)? The clock used by first_paint_after_load_time() is Time::now() and not the same monotonic clock as the rest of the timers. Sure, I decided to zero it out for now. Any code that looks this value up will continue to at least get a sane numeric value. Longer term, we'll remove chrome.loadTimes() timings entirely, but that's a more involved change.
On 2016/06/27 20:35:14, Bryan McQuade wrote: > Thanks for the review! Addressed comments. > > https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... > File chrome/renderer/loadtimes_extension_bindings.cc (right): > > https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... > chrome/renderer/loadtimes_extension_bindings.cc:147: double commit_load_time = > document_state->commit_load_time().ToDoubleT(); > On 2016/06/27 at 16:23:39, Pat Meenan wrote: > > Maybe use web_performance.responseStart() since that's functionally what it is > and would keep everything consistent and tied to the monotonic clock. > > Agree. Done. > > https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... > chrome/renderer/loadtimes_extension_bindings.cc:149: > web_performance.domContentLoadedEventEnd(); > On 2016/06/27 at 16:23:39, Pat Meenan wrote: > > Sanity check and make sure this should be the end and not the start (i.e. > includes the duration of script execution). Looking at the implementation of > finish_document_load_time() it looks like the time was set before the > notifications are passed to the observers. > > Yes, good suggestion - I had eyeballed which of start or end was closer in the > timings but hadn't actually traced the calls to see which was right. Here's what > I found: > > The existing code sets finish_document_load_time in > RenderFrameImpl::didFinishDocumentLoad: > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?dr... > > didFinishDocumentLoad is called via FrameLoader::finishedParsing which is called > from Document::finishedParsing: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... > This is slightly after m_documentTiming.markDomContentLoadedEventEnd(); is > called, earlier in Document::finishedParsing, so using domContentLoadedEventEnd > seems right here. > > https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... > chrome/renderer/loadtimes_extension_bindings.cc:150: double finish_load_time = > web_performance.loadEventEnd(); > On 2016/06/27 at 16:23:39, Pat Meenan wrote: > > Same as with DCL, I think this needs to be tied to the start instead of the > end to match existing behavior. > > Here's the trace for this one: > finish_load_time is set in RenderFrameImpl::didFinishLoad, which gets called via > FrameLoader::checkCompleted: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... > > This call is shortly after the load event gets fired, above, at the call to > m_frame->document()->implicitClose(), which eventually fires both load event > start and end, via > this->domWindow()->documentWasClosed() > LocalDOMWindow::dispatchWindowLoadEvent() > LocalDOMWindow::dispatchLoadEvent(): > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc... > > So it looks like loadEventEnd is a better match than loadEventStart. > > https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... > chrome/renderer/loadtimes_extension_bindings.cc:151: double first_paint_time = > web_performance.firstContentfulPaint(); > On 2016/06/27 at 16:23:39, Pat Meenan wrote: > > Shouldn't this be firstPaint() to keep it consistent with existing behavior > (understanding that we may be trying to push sites to look at contentful paint > instead, not sure we want to switch it out from under them). > > Yep, agree. Updated to firstPaint(). > > https://codereview.chromium.org/2084583002/diff/60001/chrome/renderer/loadtim... > chrome/renderer/loadtimes_extension_bindings.cc:156: > document_state->first_paint_after_load_time().ToDoubleT(); > On 2016/06/27 at 16:23:39, Pat Meenan wrote: > > Can we just zero this out instead (or remove it)? The clock used by > first_paint_after_load_time() is Time::now() and not the same monotonic clock as > the rest of the timers. > > Sure, I decided to zero it out for now. Any code that looks this value up will > continue to at least get a sane numeric value. Longer term, we'll remove > chrome.loadTimes() timings entirely, but that's a more involved change. non-OWNER lgtm
The CQ bit was checked by bmcquade@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.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Update chrome.loadTimes() to use timings from WebPerformance. Update chrome.loadTimes() to use timings from WebPerformance, which backs the standardized navigation timing API, instead of the deprecated DocumentState timings. BUG=621512 ========== to ========== Update chrome.loadTimes() to use timings from WebPerformance. Update chrome.loadTimes() to use timings from WebPerformance, which backs the standardized navigation timing API, instead of the deprecated DocumentState timings. BUG=621512 Committed: https://crrev.com/0d38f4e0ac28e2af5feec1fe93085037f9eb8d45 Cr-Commit-Position: refs/heads/master@{#403570} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0d38f4e0ac28e2af5feec1fe93085037f9eb8d45 Cr-Commit-Position: refs/heads/master@{#403570} |
