Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401713003:
Turn on render throttling for iframes with pending sheets. (Closed)
DescriptionTurn on render throttling for iframes with pending sheets.
This prevents us from doing layout, style, paint and requestAnimationFrame
for frames that are waiting on pending stylesheets and are not actually painting.
This puts the feature behind the --experimental-web-platform-features flag pending
approval of the Intent to Ship.
BUG=663193, 521692
Committed: https://crrev.com/1489b47d07ee66f530d0bb99fff53e0cdd84c8dd
Cr-Commit-Position: refs/heads/master@{#430794}
Patch Set 1 #Patch Set 2 : Update with new rendering throttling code. #Patch Set 3 : Fix DeferredLoadingTest and add a DocumentLoadingRenderingTest. #Patch Set 4 : fix debug. #
Total comments: 6
Patch Set 5 : ojan@ cr. #
Messages
Total messages: 33 (25 generated)
The CQ bit was checked by esprehn@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by esprehn@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_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 esprehn@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by esprehn@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Turn on render throttling for loading iframes. BUG= ========== to ========== Turn on render throttling for loading iframes. BUG=663193, 521692 ==========
Description was changed from ========== Turn on render throttling for loading iframes. BUG=663193, 521692 ========== to ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets are not actually painting. BUG=663193, 521692 ==========
Description was changed from ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets are not actually painting. BUG=663193, 521692 ========== to ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets and are not actually painting. BUG=663193, 521692 ==========
Description was changed from ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets and are not actually painting. BUG=663193, 521692 ========== to ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets and are not actually painting. This puts the feature behind the experimental web platform features flag pending approval of the Intent to Ship. BUG=663193, 521692 ==========
esprehn@chromium.org changed reviewers: + ojan@chromium.org, skyostil@chromium.org
Description was changed from ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets and are not actually painting. This puts the feature behind the experimental web platform features flag pending approval of the Intent to Ship. BUG=663193, 521692 ========== to ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets and are not actually painting. This puts the feature behind the --experimental-web-platform-features flag pending approval of the Intent to Ship. BUG=663193, 521692 ==========
lgtm once the intent to ship is approved. https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4461: void FrameView::maybeRecordLoadReason() { Nit: I have a pet peeve around putting "maybe" in method names. I think it provides less value than the cost of making the code hard to read. But, just my opinion, so feel free to ignore. https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:190: RenderingPipelineThrottlingLoadingIframes status=experimental I'd just mark this as status=stable from the beginning. We just need the flag so we have an easy way to turn it off in case it breaks content. https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp (right): https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp:273: // requires doing compositing and paint invalidation buttom up. buttom :) https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp:321: // Frame while the child frame still has pending sheets. This comment is confusing because of using the word frame to mean two different things here. I guess it's pretty clear once you actually read the code though, so, meh whatevs.
https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4461: void FrameView::maybeRecordLoadReason() { On 2016/11/08 at 21:27:06, ojan wrote: > Nit: I have a pet peeve around putting "maybe" in method names. I think it provides less value than the cost of making the code hard to read. But, just my opinion, so feel free to ignore. Yeah... the code this is calling is already called maybeRecordLoadReason, so I'd rather keep these two consistent since this is just a wrapper around the other one. You reviewed the other one too :P We should probably just rename them both. I added a TODO to rename them to recordDeferredLoadReason. https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2401713003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:190: RenderingPipelineThrottlingLoadingIframes status=experimental On 2016/11/08 at 21:27:06, ojan wrote: > I'd just mark this as status=stable from the beginning. We just need the flag so we have an easy way to turn it off in case it breaks content. I'm going to land as experimental now while I wait on the Intent thread so this doesn't rot as we discussed over chat. I'll land a stable flip later this week. :)
On 2016/11/08 at 21:27:06, ojan wrote: > lgtm once the intent to ship is approved. > As discussed over chat I'm going to land this for now behind the flag while I wait on the thread.
The CQ bit was checked by esprehn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org Link to the patchset: https://codereview.chromium.org/2401713003/#ps80001 (title: "ojan@ cr.")
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 #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets and are not actually painting. This puts the feature behind the --experimental-web-platform-features flag pending approval of the Intent to Ship. BUG=663193, 521692 ========== to ========== Turn on render throttling for iframes with pending sheets. This prevents us from doing layout, style, paint and requestAnimationFrame for frames that are waiting on pending stylesheets and are not actually painting. This puts the feature behind the --experimental-web-platform-features flag pending approval of the Intent to Ship. BUG=663193, 521692 Committed: https://crrev.com/1489b47d07ee66f530d0bb99fff53e0cdd84c8dd Cr-Commit-Position: refs/heads/master@{#430794} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1489b47d07ee66f530d0bb99fff53e0cdd84c8dd Cr-Commit-Position: refs/heads/master@{#430794}
Message was sent while issue was closed.
lgtm. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
