Bug 74144

Submitted test to W3C: https://hg.csswg.org/test/raw-file/d0faf2471ec7/contributors/adobe/incoming/css3-regions/regions-flow-descendants.xht and https://hg.csswg.org/test/raw-file/d0faf2471ec7/contributors/adobe/incoming/css3-regions/regions-flow-descendants-ref.xht

*** Bug 103685 has been marked as a duplicate of this bug. ***

Created attachment 196131 [details] patch

Comment on attachment 196131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196131&action=review SVG bits seem fine. Can't comment on the rest. > Source/WebCore/dom/Text.cpp:-305 > - if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) { Why did you change this? It was cleaner before and it seems functionally equivalent.

Comment on attachment 196131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196131&action=review >> Source/WebCore/dom/Text.cpp:-305 >> - if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) { > > Why did you change this? It was cleaner before and it seems functionally equivalent. The previous code here threw a compile time error saying that "a non-const l-value NodeRenderingContext cannot be passed on a temporary object on the stack". I have thought about how to keep the old code as it clearly more elegant, but the compiler disagreed.

Comment on attachment 196131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196131&action=review > LayoutTests/ChangeLog:8 > + * fast/regions/flow-body-in-html-expected.txt: Added. Let's try to make use of check-layout.js in the tests. If not possible, ref tests are also a possibility. Layout text dumps are not portable between platforms and should be avoided. > Source/WebCore/ChangeLog:8 > + DOM children of a region must not be rendered as children of the region, but can be collected in flow threads. An overview of the approach taken to solve the bug would be helpful. > Source/WebCore/dom/Text.cpp:305 > + if (!textRenderer) { What changed here? Why the refactoring? > Source/WebCore/rendering/RenderRegion.h:148 > + virtual bool canDOMChildrenHaveRenderParent() const OVERRIDE { return true; } This is basically equivalent with isRenderRegion, right? Why do we need a second accessor? We want to cover isRenderRegionSet as well?

Comment on attachment 196131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196131&action=review >> Source/WebCore/rendering/RenderRegion.h:148 >> + virtual bool canDOMChildrenHaveRenderParent() const OVERRIDE { return true; } > > This is basically equivalent with isRenderRegion, right? Why do we need a second accessor? We want to cover isRenderRegionSet as well? This method ensures that this bug is fixed for good and other features in webkit can avoid it just by overriding the method. Using a virtual functions ensures that other features (newmulticol/overflow fragments) can decide their behavior without having to find the line of code in NodeRenderingContext.cpp where they would otherwise need to add code for themselves.

Comment on attachment 196131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196131&action=review > LayoutTests/ChangeLog:14 > + For all the tests, you can use the dumpAsText() output. if (window.testRunner) testRunner.dumpAsText() In general, i would make all the tests for 74144, even those for the 103685. Specifying links to both bugs seems confusing to me. > Source/WebCore/ChangeLog:9 > + I would like to see a description of the general approach taken to solve this bug. > Source/WebCore/ChangeLog:16 > + (WebCore): I was told to remove the lines like the above from the Changelogs. > Source/WebCore/dom/NodeRenderingContext.cpp:181 > + return 0; This should follow the same pattern like: return m_renderingParent ? m_renderingParent->renderer() : 0;

Comment on attachment 196131 [details] patch Attachment 196131 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17394078 New failing tests: fast/regions/flow-body-in-html.html fast/regions/universal-selector-children-to-the-same-region.html fast/regions/region-content-flown-into-region.html

Created attachment 196148 [details] Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

Comment on attachment 196131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196131&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:173 > + if (m_node->isElementNode() && toElement(m_node)->moveToFlowThreadIsNeeded()) { Is there a way to cache the result of calling styleForRenderer() for m_node and pass it to moveToFlowThreadIsNeeded() and also initialize m_style member in this class? You are calling styleForRenderer() a lot and i think you can reduce the necessary calls.

Comment on attachment 196131 [details] patch Attachment 196131 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17240831 New failing tests: fast/regions/flow-body-in-html.html fast/regions/universal-selector-children-to-the-same-region.html fast/regions/region-content-flown-into-region.html

Created attachment 196167 [details] Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

Comment on attachment 196131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=196131&action=review > Source/WebCore/dom/Element.cpp:2573 > + // FIXME: Do not collect elements if they are in shadow tree. > if (isInShadowTree()) > return false; I have carried this FIXME text along but it seems the code below fixes the problem. I will remove this FIXME comment. Please comment if you think otherwise.

Created attachment 196601 [details] patch that incorporates the feedback.

(In reply to comment #15) > Created an attachment (id=196601) [details] > patch that incorporates the feedback. Ok, we need David Hyatt's input on this.

Comment on attachment 196601 [details] patch that incorporates the feedback. r=me

Comment on attachment 196601 [details] patch that incorporates the feedback. Clearing flags on attachment: 196601 Committed r147756: <http://trac.webkit.org/changeset/147756>

All reviewed patches have been landed. Closing bug.

This patch is causing the performance regression we can see today on the performance bots. On my tests, I could measure a slowdown of 9.5% on the Parser/html5-full-render.html test. These are my runs (ms, lower is better): r147911 4642.6008261 4641.89847605 4643.60051905 r147911 (with this patch reverted) 4239.35488146 4234.28493692 4251.07173051 For the record: This is the shortest range of "suspects" by crossing the information from the performance bots. http://trac.webkit.org/log/?rev=147758&stop_rev=147752&verbose=on

I'm rolling out the patch as part of https://bugs.webkit.org/show_bug.cgi?id=114176

Created attachment 198890 [details] patch that's 0.7-1.0% slower in the performance tests instead of 9.5% Running the performance tests on 2 machines gets only 0.7-1.0% bigger times instead of the 9.5% of the previous patch. The big performance impact of the previous patch was caused by always setting m_style = element->styleForRenderer() in NodeRenderingContext::createRendererForElementIfNeeded() even if the style was already computed and stored in m_style.

Comment on attachment 198890 [details] patch that's 0.7-1.0% slower in the performance tests instead of 9.5% r=me

Comment on attachment 198890 [details] patch that's 0.7-1.0% slower in the performance tests instead of 9.5% Clearing flags on attachment: 198890 Committed r148865: <http://trac.webkit.org/changeset/148865>

All reviewed patches have been landed. Closing bug.

The extra styleForRenderer() that this added in Element::moveToFlowThreadIsNeeded() is showing up on profiles. This code needs to be conditionalized to never run on pages that don't use regions.

Maybe RenderView::flowThreadController() should return nil until a named flow is actually encountered, and that can be used as the trigger to hit slow code.

(In reply to comment #26) > The extra styleForRenderer() that this added in Element::moveToFlowThreadIsNeeded() is showing up on profiles. This code needs to be conditionalized to never run on pages that don't use regions. When parsing a page without regions you need to compute the style to read the "flow" properties to know whether the page has regions. So this is a logical loop. In short, how do we detect that the page doesn't use regions without computing the style (note that NodeRenderingContext is used when parsing the page)?

Why does moveToFlowThreadIsNeeded() need to every compute style itself? When does the renderer not have a style?

I've been actively discussing this problem at https://codereview.chromium.org/15027005/ with Julien Chaffraix. When the discussion comes to a conclusion I will post the conclussions here too (and a patch).

As Simon Fraser suggested that there could be less calls to Element::styleForRenderer(), I'm working on it at https://bugs.webkit.org/show_bug.cgi?id=117637

Closing the bug, as Simon's concerns are not applicable on the latest code.

Reopened after http://trac.webkit.org/changeset/166353 temporarily removed support for this. Changeset http://trac.webkit.org/changeset/166632 added a test for that.

CSS Regions were removed in Bug 174978.