Chromium Code Reviews
|
|
|
Created:
3 years, 1 month ago by dcheng Modified:
3 years ago Reviewers:
Nate Chapin CC:
alexmos, blink-reviews, chromium-reviews, gavinp+loader_chromium.org, haraken, Nate Chapin, jochen (gone - plz use gerrit), kinuko, kinuko+watch, kouhei (in TOK), loading-reviews_chromium.org, Mike West, Devlin, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefer loads in new pages/frames if ScopedPageLoadDeferral is active
BUG=628942
Committed: https://crrev.com/19ad54cb204cde45db95e773c5d54b04b2f178d4
Cr-Commit-Position: refs/heads/master@{#408354}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : hammer #
Total comments: 3
Patch Set 4 : Make comment more accurate. #Patch Set 5 : Don't leak pages #Patch Set 6 : Simplify #
Total comments: 4
Patch Set 7 : Make the unfortunate hack slightly less ugly. #
Messages
Total messages: 37 (22 generated)
Description was changed from ========== Moo Moo Moo Moo Hammertime BUG= ========== to ========== Defer loads in new pages/frames if ScopedPageLoadDeferral is active BUG=628942 ==========
dcheng@chromium.org changed reviewers: + dominicc@chromium.org, japhet@chromium.org
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
I'm not a huge fan of this patch but it does seem to work. +japhet / +dominicc, what are your thoughts? In theory, this shouldn't be needed but thanks to it's necessary atm due to the interface we currently provide to the embedder for running scripts. https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:278: DCHECK(m_frame->loader().stateMachine()->creatingInitialEmptyDocument() This is called during FrameLoader::init() to create the initial empty document. It "should" be OK to allow this, as the document will always be same origin. https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:211: setDefersLoading(true); This makes me pretty sad. When we initially create a Page, we don't actually have a frame associated with it. Thus, we have to tell the page to self-defer once the initial empty page is synchronously loaded. I'm also uncertain of the correctness of doing it here: I believe startLoadingMainResource() also runs events, including the iframe load event. I think it's OK because the navigations will get scheduled, and the defers loading call here will suspend them.. but it's pretty dodgy. https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:3238: TEST_F(WebViewTest, NestedLoadDeferrals) I haven't figured out why, but this test (unexpectedly) passes without my patch even though there's no tracking of the defer count...
The CQ bit was checked by dcheng@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...
What's not to like? Deferral should nest. Getting rid of that unused exclusion thing looks like a win. Is the problem with the test that the m_webViewHelper ends up reusing something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dcheng@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...
On 2016/07/25 07:40:14, dominicc wrote: > What's not to like? Deferral should nest. Getting rid of that unused exclusion > thing looks like a win. The unused exclusion thing was just incidental cleanup. I think my main issue with this patch is twofold: 1) when we have to nest deferrals, we're already solidly into the dangerous zone: script is executing synchronously when it shouldn't be. 2) "deferred" now means "mostly deferred": we can't really defer the initial empty document, so we have to allow it... which means more script running. > > Is the problem with the test that the m_webViewHelper ends up reusing something? That should be OK... in theory. I'll investigate more later, but this isn't the highest priority for me atm.
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 japhet@chromium.org to run a CQ dry run
The CQ bit was unchecked by japhet@chromium.org
PTAL. I simplified things a little bit: since we only defer 'ordinary pages', I just use Page::ordinaryPages() directly now rather than keeping a separate set. It was also a bit weird, because the set of deferred pages could keep a Page object alive. While we could try deferring all pages, I'm not certain of the correctness of that change... so I'd prefer to make the more conservative change for now. FWIW, I think we'll need this anyway because of things like https://crbug.com/631521.
+alexmos as FYI
LGTM...I guess. https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:210: if (m_frame->page() && m_frame->page()->defersLoading()) This makes me saddest of all :( https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right): https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:57: if (--s_deferralCount == 0) { Nit: it bothers my sense of aesthetics that the constructor early-exits and the destructor doesn't :)
dominicc@chromium.org changed reviewers: - dominicc@chromium.org
-r=me, japhet is a better person to review this!
https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:210: if (m_frame->page() && m_frame->page()->defersLoading()) On 2016/07/27 17:30:47, Nate Chapin wrote: > This makes me saddest of all :( Yes... I would have to completely agree =( https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right): https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:57: if (--s_deferralCount == 0) { On 2016/07/27 17:30:47, Nate Chapin wrote: > Nit: it bothers my sense of aesthetics that the constructor early-exits and the > destructor doesn't :) Done.
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2174263002/#ps120001 (title: "Make the unfortunate hack slightly less ugly.")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by dcheng@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dcheng@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 ========== Defer loads in new pages/frames if ScopedPageLoadDeferral is active BUG=628942 ========== to ========== Defer loads in new pages/frames if ScopedPageLoadDeferral is active BUG=628942 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Defer loads in new pages/frames if ScopedPageLoadDeferral is active BUG=628942 ========== to ========== Defer loads in new pages/frames if ScopedPageLoadDeferral is active BUG=628942 Committed: https://crrev.com/19ad54cb204cde45db95e773c5d54b04b2f178d4 Cr-Commit-Position: refs/heads/master@{#408354} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/19ad54cb204cde45db95e773c5d54b04b2f178d4 Cr-Commit-Position: refs/heads/master@{#408354} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
