Bug 74144 - [CSS Regions] Elements in a region should be assignable to a named flow
Summary: [CSS Regions] Elements in a region should be assignable to a named flow
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Maerean
URL:
Keywords: AdobeTracked
: 103685 (view as bug list)
Depends on: 74132 114176 117637
Blocks: 57312 115861
  Show dependency treegraph
 
Reported: 2011-12-08 17:08 PST by Alan Stearns
Modified: 2014-04-04 00:48 PDT (History)
21 users (show)

See Also:


Attachments
reftest (854 bytes, application/zip)
2011-12-08 17:08 PST, Alan Stearns
no flags Details
patch (31.91 KB, patch)
2013-04-02 07:03 PDT, Mihai Maerean
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64 (935.72 KB, application/zip)
2013-04-02 08:35 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 (1.22 MB, application/zip)
2013-04-02 09:42 PDT, WebKit Review Bot
no flags Details
patch that incorporates the feedback. (24.63 KB, patch)
2013-04-05 02:09 PDT, Mihai Maerean
no flags Details | Formatted Diff | Diff
patch that's 0.7-1.0% slower in the performance tests instead of 9.5% (24.99 KB, patch)
2013-04-19 09:03 PDT, Mihai Maerean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Stearns 2011-12-08 17:08:25 PST
Created attachment 118497 [details]
reftest

It should be possible to redirect the children of an element that becomes a region to a different named flow. Currently this does not work - all of the region children are not displayed, even if there is flow-into and flow-from styling that should cause them to be displayed.
Comment 2 Mihai Balan 2013-02-21 10:02:59 PST
*** Bug 103685 has been marked as a duplicate of this bug. ***
Comment 3 Mihai Maerean 2013-04-02 07:03:02 PDT
Created attachment 196131 [details]
patch
Comment 4 Stephen Chenney 2013-04-02 07:38:32 PDT
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 5 Mihai Maerean 2013-04-02 07:52:15 PDT
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 6 Andrei Bucur 2013-04-02 08:14:00 PDT
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 7 Mihai Maerean 2013-04-02 08:26:03 PDT
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 8 Mihnea Ovidenie 2013-04-02 08:28:37 PDT
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 9 WebKit Review Bot 2013-04-02 08:35:38 PDT
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
Comment 10 WebKit Review Bot 2013-04-02 08:35:42 PDT
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 11 Mihnea Ovidenie 2013-04-02 09:00:21 PDT
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 12 WebKit Review Bot 2013-04-02 09:42:52 PDT
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
Comment 13 WebKit Review Bot 2013-04-02 09:42:57 PDT
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 14 Mihai Maerean 2013-04-04 06:03:51 PDT
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.
Comment 15 Mihai Maerean 2013-04-05 02:09:34 PDT
Created attachment 196601 [details]
patch that incorporates the feedback.
Comment 16 Mihnea Ovidenie 2013-04-05 09:21:22 PDT
(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 17 Dave Hyatt 2013-04-05 09:25:59 PDT
Comment on attachment 196601 [details]
patch that incorporates the feedback.

r=me
Comment 18 WebKit Commit Bot 2013-04-05 10:13:33 PDT
Comment on attachment 196601 [details]
patch that incorporates the feedback.

Clearing flags on attachment: 196601

Committed r147756: <http://trac.webkit.org/changeset/147756>
Comment 19 WebKit Commit Bot 2013-04-05 10:13:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Thiago Marcos P. Santos 2013-04-08 08:52:19 PDT
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
Comment 21 Mihai Maerean 2013-04-08 23:01:25 PDT
I'm rolling out the patch as part of https://bugs.webkit.org/show_bug.cgi?id=114176
Comment 22 Mihai Maerean 2013-04-19 09:03:42 PDT
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 23 Dave Hyatt 2013-04-19 11:56:13 PDT
Comment on attachment 198890 [details]
patch that's 0.7-1.0% slower in the performance tests instead of 9.5%

r=me
Comment 24 WebKit Commit Bot 2013-04-22 02:04:42 PDT
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>
Comment 25 WebKit Commit Bot 2013-04-22 02:04:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Simon Fraser (smfr) 2013-05-28 22:57:28 PDT
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.
Comment 27 Simon Fraser (smfr) 2013-05-28 22:59:23 PDT
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.
Comment 28 Mihai Maerean 2013-05-29 07:47:24 PDT
(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)?
Comment 29 Simon Fraser (smfr) 2013-05-29 09:16:29 PDT
Why does moveToFlowThreadIsNeeded() need to every compute style itself? When does the renderer not have a style?
Comment 30 Mihai Maerean 2013-06-07 08:50:16 PDT
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).
Comment 31 Mihai Maerean 2013-06-14 06:01:24 PDT
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
Comment 32 Mihai Maerean 2013-07-15 04:42:43 PDT
Closing the bug, as Simon's concerns are not applicable on the latest code.
Comment 33 Mihnea Ovidenie 2014-04-04 00:48:25 PDT
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.