Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2333263002:
Document::haveImportsLoaded() should return true when ignoring pending sheets (Closed)
DescriptionDocument::haveImportsLoaded() should return true when ignoring pending sheets
BUG=646323
TESTS=webkit_unit_tests DocumentLoadingRenderingTest.ShouldNotPerformRepeatedLayoutWithPendingImport DocumentLoadingRenderingTest.ShouldClearPlaceholderStyleWhenIgnoringPendingStylesheet
Committed: https://crrev.com/cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf
Cr-Commit-Position: refs/heads/master@{#419067}
Patch Set 1 #Patch Set 2 : haveImportsLoaded() should consider ignoring pending stylesheets #Patch Set 3 : Tests added #Patch Set 4 : Tests really added... #
Total comments: 3
Patch Set 5 : Remove EXPECT_FALSE(document().hasNodesWithPlaceholderStyle()) from test #Patch Set 6 : Use StyleEngine::styleForElementCount #
Dependent Patchsets: Messages
Total messages: 44 (28 generated)
The CQ bit was checked by xiaochengh@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: This issue passed the CQ dry run.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
That sucks we have poor test coverage here, this patch doesn't actually work unfortunately. This code is required to make sure we recompute the style of nodes with placeholder styles. If you remove it you'll break pages requesting layout information (ex. offsetTop) if the rendering system had already computed their style (search for any caller of updateLayout() without the pending suffix, this manifests more often with iframes since they don't do deferred commits too). There's a bigger project to fix all of this if you want to help, it's not as easy as just deleting the code here. :)
On 2016/09/13 at 17:40:19, esprehn wrote: > That sucks we have poor test coverage here, this patch doesn't actually work unfortunately. This code is required to make sure we recompute the style of nodes with placeholder styles. If you remove it you'll break pages requesting layout information (ex. offsetTop) if the rendering system had already computed their style (search for any caller of updateLayout() without the pending suffix, this manifests more often with iframes since they don't do deferred commits too). > > There's a bigger project to fix all of this if you want to help, it's not as easy as just deleting the code here. :) Would adding a test for this be a good first step? :)
On 2016/09/13 at 17:41:34, dglazkov wrote:
> On 2016/09/13 at 17:40:19, esprehn wrote:
> > That sucks we have poor test coverage here, this patch doesn't actually work
unfortunately. This code is required to make sure we recompute the style of
nodes with placeholder styles. If you remove it you'll break pages requesting
layout information (ex. offsetTop) if the rendering system had already computed
their style (search for any caller of updateLayout() without the pending suffix,
this manifests more often with iframes since they don't do deferred commits
too).
> >
> > There's a bigger project to fix all of this if you want to help, it's not as
easy as just deleting the code here. :)
>
> Would adding a test for this be a good first step? :)
Yeah, this should be a not too hard SimTest to write. This system has very
little test coverage because pre-SimTest it was super hard to write a test.
What you want to do is probably something like:
1) Load a page and insert the <body> so we undefer commits:
resource.write("<body>");
2) Trigger a frame so we do a layout: compositor().beginFrame();
3) Insert an external stylesheet but don't let it finish loading:
resource.write("<link rel=stylesheet href=...>");
4) Insert a <div id="target">Some Content Here</div>: resource.write("...")
5) Call document().updateStyleAndLayout(); the *not the ignore one*.
7) ASSERT document.getElementById("target")->offsetHeight() > 0.
Calling updateStyleAndLayout should have given the <div> a placeholder style
which means we pretend it's display none. The code you removed here should then
make us update the style inside offsetHeight when we call the
*IgnorePendingStylesheets() version of the update method inside offsetHeight()
so you get a non-zero height. With this patch applied you should fail the test
since the element would remain with the placeholder display: none style.
Also I would add this to DocumentLoadingRenderingTest, it's a SimTest that already tests other parts of this FOUC avoidance system.
The CQ bit was checked by xiaochengh@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: This issue passed the CQ dry run.
Description was changed from ========== Remove an unnecessary forced layout from updateStyleAndLayoutTreeIgnorePendingStylesheets BUG=646323 ========== to ========== Document::haveImportsLoaded() should return true when ignoring pending sheets BUG=646323 TESTS=still working... ==========
Updated the patch following the crbug discussion. Still working on test cases...
I tried to make a test case following #8 but didn't succeed, as
|updateStyleAndLayout()| didn't set a placeholder style for the <div>. The test
looks like:
TEST_F(DocumentLoadingRenderingTest,
ShouldClearPlaceholderStyleWhenIgnoringPendingStylesheet)
{
SimRequest mainResource("https://example.com/test.html", "text/html");
SimRequest cssResource("https://example.com/test.css", "text/css");
loadURL("https://example.com/test.html");
mainResource.start();
mainResource.write("<!DOCTYPE html><body>");
compositor().beginFrame();
mainResource.write("<link rel=stylesheet href=test.css>");
cssResource.start();
mainResource.write("<div id=target>foo</div>");
document().updateStyleAndLayout();
// This assertion fails :(
EXPECT_TRUE(document().hasNodesWithPlaceholderStyle());
document().updateStyleAndLayoutIgnorePendingStylesheets();
EXPECT_FALSE(document().hasNodesWithPlaceholderStyle());
mainResource.finish();
cssResource.finish();
EXPECT_TRUE(document().isRenderingReady());
}
The CQ bit was checked by xiaochengh@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 checked by xiaochengh@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 ========== Document::haveImportsLoaded() should return true when ignoring pending sheets BUG=646323 TESTS=still working... ========== to ========== Document::haveImportsLoaded() should return true when ignoring pending sheets BUG=646323 TESTS=webkit_unit_tests DocumentLoadingRenderingTest.ShouldNotPerformRepeatedLayoutWithPendingImport DocumentLoadingRenderingTest.ShouldClearPlaceholderStyleWhenIgnoringPendingStylesheet ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. The patch is updated with two tests, one covering the code path that I thought removable, the other serving as a regression test.
rune@opera.com changed reviewers: + rune@opera.com
https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp (right): https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp:375: // nodes with placeholder style. Not sure if I understood completely, but you mean that if the EXPECT_FALSE above fails, the updateStyle... below will crash without the DisallowTransitionScope? If so, the EXPECT_FALSE can be changed to an ASSERT_FALSE which will stop execution for this test.
https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp (right): https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp:375: // nodes with placeholder style. On 2016/09/15 at 10:48:26, rune wrote: > Not sure if I understood completely, but you mean that if the EXPECT_FALSE above fails, the updateStyle... below will crash without the DisallowTransitionScope? > > If so, the EXPECT_FALSE can be changed to an ASSERT_FALSE which will stop execution for this test. Sorry for the confusion. I just want to check if the second updateStyle... is a no-op or not, and the only way that I know is to put it in a DisallowTransitionScope and see if it crashes... The EXPECT_FALSE above the scope is not necessary and should be removed. I didn't thought about it very thoroughly. Is there any better way to check if the second updateStyle... is a no-op?
The CQ bit was checked by xiaochengh@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...
Following rune@'s comment, the test has been updated to only check if the second updateStyleAndLayoutIgnorePendingStylesheet() is a no-op.
https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp (right): https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp:375: // nodes with placeholder style. On 2016/09/15 11:28:30, Xiaocheng wrote: > Sorry for the confusion. I just want to check if the second updateStyle... is a > no-op or not, and the only way that I know is to put it in a > DisallowTransitionScope and see if it crashes... > > The EXPECT_FALSE above the scope is not necessary and should be removed. I > didn't thought about it very thoroughly. > > Is there any better way to check if the second updateStyle... is a no-op? Compare StyleEngine::styleForElementCount() before and after.
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 xiaochengh@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/09/15 at 12:37:54, rune wrote: > https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp (right): > > https://codereview.chromium.org/2333263002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/DocumentLoadingRenderingTest.cpp:375: // nodes with placeholder style. > On 2016/09/15 11:28:30, Xiaocheng wrote: > > > Sorry for the confusion. I just want to check if the second updateStyle... is a > > no-op or not, and the only way that I know is to put it in a > > DisallowTransitionScope and see if it crashes... > > > > The EXPECT_FALSE above the scope is not necessary and should be removed. I > > didn't thought about it very thoroughly. > > > > Is there any better way to check if the second updateStyle... is a no-op? > > Compare StyleEngine::styleForElementCount() before and after. It makes the test much better. Thanks a lot!
lgtm
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 xiaochengh@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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Document::haveImportsLoaded() should return true when ignoring pending sheets BUG=646323 TESTS=webkit_unit_tests DocumentLoadingRenderingTest.ShouldNotPerformRepeatedLayoutWithPendingImport DocumentLoadingRenderingTest.ShouldClearPlaceholderStyleWhenIgnoringPendingStylesheet ========== to ========== Document::haveImportsLoaded() should return true when ignoring pending sheets BUG=646323 TESTS=webkit_unit_tests DocumentLoadingRenderingTest.ShouldNotPerformRepeatedLayoutWithPendingImport DocumentLoadingRenderingTest.ShouldClearPlaceholderStyleWhenIgnoringPendingStylesheet Committed: https://crrev.com/cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf Cr-Commit-Position: refs/heads/master@{#419067} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cd626f11b0ddb8fc655ac2fd808ca3930e3e29cf Cr-Commit-Position: refs/heads/master@{#419067}
Message was sent while issue was closed.
This is a manual note that this patch has been reverted by https://codereview.chromium.org/2353243002. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
