Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1423453007:
Let RenderFrameImpl set navigationStart based on CommonNavigationParams (Closed)
|
Created:
5 years, 1 month ago by Charlie Harrison Modified:
5 years, 1 month ago CC:
blink-reviews, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, jam, Nate Chapin, loading-reviews_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@navigation_start_common_params Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet RenderFrameImpl set navigationStart based on CommonNavigationParams
This entails setting the timestamp before we set it in blink, so DocumentLoadTiming had to be modified to allow for that. We set the value in didCreateDataSource.
didCreateDataSource was also refactored slightly so that same-page navigations don't need to call it anymore.
This is 2/3 of the effort to add a navigation_start timestamp to DidStartProvisional load. The full CL we're breaking up is here:
https://codereview.chromium.org/1425823002/
BUG=548335
Committed: https://crrev.com/5f61ecb107fd7118621a99fe03b094f37ce04394
Cr-Commit-Position: refs/heads/master@{#358144}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Comments and whitespace (trybots previous) #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (9 generated)
Description was changed from ========== Let RenderFrameImpl set navigationStart based on CommonNavigationParams BUG=548335 ========== to ========== Let RenderFrameImpl set navigationStart based on CommonNavigationParams This entails setting the timestamp before we set it in blink, so DocumentLoadTiming had to be modified to allow for that. We set the value in didCreateDataSource. didCreateDataSource was also refactored slightly so that same-page navigations don't need to call it anymore. This is 2/3 of the effort to add a navigation_start timestamp to DidStartProvisional load. The full CL we're breaking up is here: https://codereview.chromium.org/1425823002/ BUG=548335 ==========
csharrison@chromium.org changed reviewers: + clamy@chromium.org, jochen@chromium.org
Please take a look. The most major decision is avoiding special casing same-page navigations in didCreateDataSource by refactoring out the logic that same-page navs need there. It seemed like other code in didCreateDataSource is also special casing same-page navs, and it isn't really intuitive that it will be called for them.
Thanks! A few comments, mostly nits. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:466: // The browser provides the navigation_start time to bootstrap the Can you move the comment above the function (and maybe rephrase it so that it stands as a description of what the function is doing)? That would be easier to read. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:471: // can't use InterProcessTimeTicksConverter. We need at least to ensure nit: we prefer to avoid "we" in comments. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:477: // TODO(clamy) We need to provide additional timing values for the Navigation This TODO should be moved where we actually set the value in the NavigationTiming object. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4624: // Lower bound for browser initiated navigation start time. nit: empty line before comment. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4651: // We don't know for sure that this load type is WebFrameLoadType::Standard. nit: empty line before comment. Also see if you can avoid using "we" in comments. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:5164: // If this is a browser-initiated load that we didn't want to override nit: rephrase "...that doesn't override the navigation_start..." https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.h:862: // the value of pending_navigation_params_. nit: s/pending_navigation_params_/|pending_navigation_params_|
Thanks! https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:466: // The browser provides the navigation_start time to bootstrap the On 2015/11/04 13:33:44, clamy wrote: > Can you move the comment above the function (and maybe rephrase it so that it > stands as a description of what the function is doing)? That would be easier to > read. Done. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:471: // can't use InterProcessTimeTicksConverter. We need at least to ensure On 2015/11/04 13:33:44, clamy wrote: > nit: we prefer to avoid "we" in comments. Done. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:477: // TODO(clamy) We need to provide additional timing values for the Navigation On 2015/11/04 13:33:44, clamy wrote: > This TODO should be moved where we actually set the value in the > NavigationTiming object. Done. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4624: // Lower bound for browser initiated navigation start time. On 2015/11/04 13:33:44, clamy wrote: > nit: empty line before comment. Done. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4651: // We don't know for sure that this load type is WebFrameLoadType::Standard. On 2015/11/04 13:33:44, clamy wrote: > nit: empty line before comment. Also see if you can avoid using "we" in > comments. Done. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:5164: // If this is a browser-initiated load that we didn't want to override On 2015/11/04 13:33:44, clamy wrote: > nit: rephrase "...that doesn't override the navigation_start..." Done. https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1423453007/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.h:862: // the value of pending_navigation_params_. On 2015/11/04 13:33:44, clamy wrote: > nit: s/pending_navigation_params_/|pending_navigation_params_| Done.
Thanks! Lgtm.
lgtm
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423453007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423453007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423453007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423453007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423453007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423453007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423453007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423453007/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5f61ecb107fd7118621a99fe03b094f37ce04394 Cr-Commit-Position: refs/heads/master@{#358144}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1427243003/ by nasko@chromium.org. The reason for reverting is: Test fails on the bots: http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28... RenderViewImplTest.NavigationStartOverride (run #1): [ RUN ] RenderViewImplTest.NavigationStartOverride [3788:2828:1105/152333:16934314:WARNING:resource_bundle.cc(305)] locale_file_path.empty() for locale c:uild\slave\win_builder__dbg_uild\src\content\renderer\render_view_browsertest.cc(2282): error: Expected: (early_nav_reported_start) < (before_navigation), actual: 2015-11-05 23:23:33.730 UTC vs 2015-11-05 23:23:33.728 UTC [ FAILED ] RenderViewImplTest.NavigationStartOverride (663 ms).
Message was sent while issue was closed.
On 2015/11/06 00:04:14, nasko (slow to review) wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/1427243003/ by mailto:nasko@chromium.org. > > The reason for reverting is: Test fails on the bots: > > http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28... > > RenderViewImplTest.NavigationStartOverride (run #1): > [ RUN ] RenderViewImplTest.NavigationStartOverride > [3788:2828:1105/152333:16934314:WARNING:resource_bundle.cc(305)] > locale_file_path.empty() for locale > c:uild\slave\win_builder__dbg_uild\src\content\renderer\render_view_browsertest.cc(2282): > error: Expected: (early_nav_reported_start) < (before_navigation), actual: > 2015-11-05 23:23:33.730 UTC vs 2015-11-05 23:23:33.728 UTC > [ FAILED ] RenderViewImplTest.NavigationStartOverride (663 ms). I'm thinking that the first part of this test is flawed (NavigationStartOverride). Blink reports navigationStart as |m_referenceWallTime + navigationStart - m_referenceMonotonicTime| But m_referenceMonotonicTime = navigationStart, so we're really just reporting m_referenceWallTime. m_referenceWallTime is essentially base::Time::Now() when we first log navigation start. I'm guessing that by logging this a little earlier (in didCreateDataSource rather than after the load is issued) we make this test flaky. Because the value reported by blink is not related to the timeticks sent in (they get subtracted out), I think we should just remove the first part of this test.
Message was sent while issue was closed.
When developping PlzNavigate, we also found that this test was very flaky. We had to disable it on our bot, even though it routinely passes locally. We also found that even with the current architecture it is somewhat flaky on the bots right now. Removing the flaky part sgtm. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
