Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2859543003:
Don't run IntersectionObserver lifecycle in frames with dirty layout. (Closed)
DescriptionDon't run IntersectionObserver lifecycle in frames with dirty layout.
IntersectionObserver calls into layout geometry code, and it's bogus to run that code while layout is dirty.
BUG=682307
Review-Url: https://codereview.chromium.org/2859543003
Cr-Commit-Position: refs/heads/master@{#469124}
Committed: https://chromium.googlesource.com/chromium/src/+/edf83955248f5f88792cec8a4e2618e2670a33cc
Patch Set 1 #Patch Set 2 : Skip IO algorithm for dirty frames. #Patch Set 3 : test expectation #
Total comments: 1
Patch Set 4 : Run RecordDeferredLoadingStats even if layout is dirty. #Messages
Total messages: 31 (23 generated)
The CQ bit was checked by szager@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...
Description was changed from ========== Don't run IntersectionObserver lifecycle for throttled child frames. BUG=682307 ========== to ========== Don't run IntersectionObserver lifecycle for throttled child frames. This uses the same logic as UpdateLifecyclePhasesInternal to set a max lifecycle target state for a throttled frame. BUG=682307 ==========
eae@chromium.org changed reviewers: + eae@chromium.org
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by szager@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by szager@chromium.org to run a CQ dry run
szager@chromium.org changed reviewers: + dgrogan@chromium.org
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.
PTAL: rather than setting a cap on the target_state, simply skip running IO lifecycle for any FrameView with dirty layout. +dgrogan@ for the test change. It makes sense to me that setting the iframe to display='none' should not cause additional loading activity, but I don't really understand the test.
It's actually the opposite: setting iframe to display:none DOES cause additional hypothetical loading activity. See inline comment for more details. Note that I say hypothetical because we are only collecting data for this future intervention, there's been no change to real loading behavior yet. https://codereview.chromium.org/2859543003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp (right): https://codereview.chromium.org/2859543003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp:306: main_resource->Write("<script>theFrame.style.display='none'</script>"); Deferred frame loading would load the document in all display:none frames. The test is simulating script making a previously offscreen and unloaded frame display:none and ensuring that it's counted as having to load. With the !NeedsLayout() change, display:none frames would only be counted as must load if there's another change to the DOM, as line 312 below does. It's probably a very cornery case, a page changing a frame to display:none and then not touching the DOM anymore, but it is a difference in how we count frames for loading, so we should bump up the version number of the histogram. Could you change Navigation.DeferredDocumentLoading.StatesV4 to V5: - at the top of this file - https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... - and in histograms.xml In histograms.xml, deprecate V4, add a new V5 with the <summary> """Why and if cross-origin documents would be loaded if we were to defer loading as long as possible. Changes from V4: frames made display:none will only be counted as must load after a subsequent change to the DOM. """ Sorry for the busy work and for punishing a good deed :(
Description was changed from ========== Don't run IntersectionObserver lifecycle for throttled child frames. This uses the same logic as UpdateLifecyclePhasesInternal to set a max lifecycle target state for a throttled frame. BUG=682307 ========== to ========== Don't run IntersectionObserver lifecycle in frames with dirty layout. IntersectionObserver calls into layout geometry code, and it's bogus to run that code while layout is dirty. BUG=682307 ==========
The CQ bit was checked by szager@chromium.org to run a CQ dry run
PTAL: I changed it to run the deferred loading code even when layout is dirty.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm This makes way more sense than changing the meaning of the histogram values.
lgtm This makes way more sense than changing the meaning of the histogram values.
The CQ bit was unchecked by szager@chromium.org
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2859543003/#ps60001 (title: "Run RecordDeferredLoadingStats even if layout is dirty.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493845461682310,
"parent_rev": "211a809c2f7c44071c551edc14a25eb8cf9602c3", "commit_rev":
"edf83955248f5f88792cec8a4e2618e2670a33cc"}
Message was sent while issue was closed.
Description was changed from ========== Don't run IntersectionObserver lifecycle in frames with dirty layout. IntersectionObserver calls into layout geometry code, and it's bogus to run that code while layout is dirty. BUG=682307 ========== to ========== Don't run IntersectionObserver lifecycle in frames with dirty layout. IntersectionObserver calls into layout geometry code, and it's bogus to run that code while layout is dirty. BUG=682307 Review-Url: https://codereview.chromium.org/2859543003 Cr-Commit-Position: refs/heads/master@{#469124} Committed: https://chromium.googlesource.com/chromium/src/+/edf83955248f5f88792cec8a4e26... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/edf83955248f5f88792cec8a4e26... |
