[CSS Regions] Elements in a region should be assignable to a named flow

DOM children of a region must not be rendered as children of the region, but can be collected in flow threads.

Ported and improved from https://bugs.webkit.org/show_bug.cgi?id=74144

BUG=228566
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153439

[stavila, 2013-06-28 17:37:39]

Please review. If it looks good, please LGTM but DO NOT commit. I want to run
more tests before committing, for the moment I would like you to validate the
approach (I'm not expecting to change it again before committing if it looks
good to you, but still, just to be sure).

[Mihai Maerean, 2013-06-28 18:38:40]

On 2013/06/28 17:37:39, stavila wrote:
> Please review. If it looks good, please LGTM but DO NOT commit. I want to run
> more tests before committing, for the moment I would like you to validate the
> approach (I'm not expecting to change it again before committing if it looks
> good to you, but still, just to be sure).

this is already in progress at https://codereview.chromium.org/15027005/

[stavila, 2013-06-28 19:43:39]

@Mihai I know, Mihnea & Catalin wanted me to finish this as you will not be back
at the office for some time.

[esprehn, 2013-06-28 20:29:36]

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/flow...
File LayoutTests/fast/regions/flow-body-in-html.html (right):

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/flow...
LayoutTests/fast/regions/flow-body-in-html.html:9: -webkit-flow-from: foo;
Whitespace is still wrong in these tests. I had the same comments in the
original patch.

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/regi...
File LayoutTests/fast/regions/region-content-flown-into-region-expected.html
(right):

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/regi...
LayoutTests/fast/regions/region-content-flown-into-region-expected.html:21: <div
class="region"><template>PASS r0: content that goes in the
region.</template></div>
<template> doesn't make sense like this, please remove.

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/regi...
File LayoutTests/fast/regions/region-content-flown-into-region.html (right):

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/regi...
LayoutTests/fast/regions/region-content-flown-into-region.html:42: <template
class="content">PASS r0: content that goes in the region.</template>
Same thing, this is the same as an empty <div>. If you want to test template and
regions (which seems pretty weird, but you can if you want) it should be in a
separate test.

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/univ...
File
LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html
(right):

https://codereview.chromium.org/18080016/diff/1/LayoutTests/fast/regions/univ...
LayoutTests/fast/regions/universal-selector-children-to-the-same-region.html:6:
-webkit-flow-into: foo;
ws

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2337: return; // Will always be called during
attaching, don't create rare data structure if not necessary.
Remove the comment. This kind of oldValue == newValue is standard practice for
all rareData stuff, there's no reason to mention attaching.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2647: Document* doc = document();
Remove this, just call document() below. You do it anyway for renderView().

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2649: if
(!RuntimeEnabledFeatures::cssRegionsEnabled())
This check should probably be in NodeRenderingContext not inside here.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2654: return false;
Remove this, if you're asserting you shouldn't be guarding.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2657: if (doc->webkitIsFullScreen() &&
doc->webkitCurrentFullScreenElement() == this)
FullScreenController::isActiveFullScreenElement(this) I think is what you want.
You don't need the webkitIsFullScreen() check.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2662: return false;
Interesting, so nothing inside a shadow is ever inside regions?

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.h#new...
Source/core/dom/Element.h:485: bool isDOMChildOfRegion() const;
Remove "DOM" from the name. This is an Element so it's all DOM.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/ElementRareDa...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/ElementRareDa...
Source/core/dom/ElementRareData.h:66: bool isDOMChildOfRegion() const { return
m_isDOMChildOfRegion; }
This should be "isInsideRegion"

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:190: Element* element =
toElement(m_node);
You don't need the ASSERT( before this since toElement does it for you.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:204: void
NodeRenderingContext::moveToFlowThread()
Why is this a separate method now? Lets just merge it into
mergeToFlowThreadIfNeeded()

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:236: // Check the specific case of
elements that are children of regions but are flowed into a flow thread
themselves.
This whole check should be in a method.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:242: m_style =
element->styleForRenderer();
This doesn't seem right, We should put  if (!m_style) m_style =
element->styleForRenderer(); in one single place, not in lots of methods.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:244: shouldBeInNamedFlow = element &&
element->shouldMoveToFlowThread(m_style.get());
element can't be null, you don't need element &&

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:248:
element->setIsDOMChildOfRegion(true);
You never reset this to false so if I remove a child and put it somewhere else
in the tree it's still marked inside regions.

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Text.cpp#newc...
Source/core/dom/Text.cpp:307: return;
Why did you change this code? What you have looks identical.

https://codereview.chromium.org/18080016/diff/1/Source/core/rendering/FlowThr...
File Source/core/rendering/FlowThreadController.cpp (right):

https://codereview.chromium.org/18080016/diff/1/Source/core/rendering/FlowThr...
Source/core/rendering/FlowThreadController.cpp:228: bool
FlowThreadController::isContentNodeRegisteredWithAnyNamedFlow(const Node*
contentNode) const
Lets put this on Node instead so you do node->isRegisteredWithNamedFlow()

[stavila, 2013-06-28 20:37:46]

Thanks for the suggestions, I'll implement them monday.

[esprehn, 2013-06-28 20:41:19]

On 2013/06/28 20:37:46, stavila wrote:
> Thanks for the suggestions, I'll implement them monday.

btw, architecturally this patch looks fine if you can figure out where to
setIsInsideRegion(false). You probably want to look at the ::removedFrom
callback.

[stavila, 2013-06-28 21:20:02]

On 2013/06/28 20:41:19, esprehn wrote:
> On 2013/06/28 20:37:46, stavila wrote:
> > Thanks for the suggestions, I'll implement them monday.
> 
> btw, architecturally this patch looks fine if you can figure out where to
> setIsInsideRegion(false). You probably want to look at the ::removedFrom
> callback.

Isn't it enough to do it when attaching?

[stavila, 2013-07-01 15:33:39]

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/18080016/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:2662: return false;
On 2013/06/28 20:29:36, esprehn wrote:
> Interesting, so nothing inside a shadow is ever inside regions?
For the moment, no. It is still under discussion at the spec-level whether it
should be allowed or not.

[esprehn, 2013-07-01 18:27:19]

LGTM. Please fix comments before landing.

https://codereview.chromium.org/18080016/diff/18001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/18080016/diff/18001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:2660: return
!this->isRegisteredWithNamedFlow(document()->renderView()->flowThreadController());
I don't think you need "this->"

https://codereview.chromium.org/18080016/diff/18001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/18080016/diff/18001/Source/core/dom/Node.cpp#...
Source/core/dom/Node.cpp:1200: return
flowThreadController->isContentNodeRegisteredWithAnyNamedFlow(this);
Don't pass the flowThreadContorller in, just get it inside here.

document()->renderView()->flowThreadController()

https://codereview.chromium.org/18080016/diff/18001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/18080016/diff/18001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:241: element->setIsInsideRegion(false);
You need to do this inside Node::detach too. Otherwise if I remove an element
from the tree it'll still claim it's inside a region.

https://codereview.chromium.org/18080016/diff/18001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:246: return;
if (!shouldCreateRenderer() && !elementInsideRegionNeedsRenderer())
  return;

Put the comment above the elementInsideRegionNeedsRenderer method.

[commit-bot: I haz the power, 2013-07-01 18:58:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/18080016/32001

[Mihai Maerean, 2013-07-01 19:03:35]

Please run the PerformanceTests/Parser/* tests (especially
html5-full-render.html) and post the results to make sure this runs ok.

https://codereview.chromium.org/18080016/diff/18001/LayoutTests/fast/regions/...
File LayoutTests/fast/regions/region-content-flown-into-region.html (right):

https://codereview.chromium.org/18080016/diff/18001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/region-content-flown-into-region.html:1: <html>
Julien asked at https://codereview.chromium.org/15027005/ that this test
verifies the behavior of svg nodes too.
Please take a look at the latest version of this test in that review.

[stavila, 2013-07-01 19:21:07]

On 2013/07/01 19:03:35, Mihai Maerean wrote:
> Please run the PerformanceTests/Parser/* tests (especially
> html5-full-render.html) and post the results to make sure this runs ok.
> 
>
https://codereview.chromium.org/18080016/diff/18001/LayoutTests/fast/regions/...
> File LayoutTests/fast/regions/region-content-flown-into-region.html (right):
> 
>
https://codereview.chromium.org/18080016/diff/18001/LayoutTests/fast/regions/...
> LayoutTests/fast/regions/region-content-flown-into-region.html:1: <html>
> Julien asked at https://codereview.chromium.org/15027005/ that this test
> verifies the behavior of svg nodes too.
> Please take a look at the latest version of this test in that review.

I will but there's no reason why any penalty would appear.

[commit-bot: I haz the power, 2013-07-01 20:20:44]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[stavila, 2013-07-01 21:19:52]

Performance results:
html5-full-render -> 0.3%  slower          WITH patch
css-parser-yui ->                                   IDENTICAL
html-parser -> 1.5% slower                    WITH patch
iframe-append-remove -> 0.7% slower  WITH patch
innerHTML-setter -> 1.5% slower           WITH patch
query-selector-first -> 1.2% slower        WITH patch
query-selector-id-last -> 0.2% slower    WITHOUT patch
quert-selector-last ->                            IDENTICAL
simple-url ->                                         IDENTICAL
textarea-parsing ->                               IDENTICAL
tiny-innerHTML -> 1.3% slower              WITH PATCH
url-parser -> 0.8% slower                      WITHOUT patch
xml-parser -> 0.5% slower                     WITHOUT patch

[stavila, 2013-07-01 21:26:58]

Performance looks good but there's a fullscreen test failing, I'll check
tomorrow if it's related (might be).

[stavila, 2013-07-02 12:24:38]

Adding the SVG test uncovered an unrelated issue. As such,
https://code.google.com/p/chromium/issues/detail?id=256611 must land before
committing this (otherwise a test will fail).

[commit-bot: I haz the power, 2013-07-02 15:22:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/18080016/50001

[commit-bot: I haz the power, 2013-07-02 17:43:34]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-07-02 17:44:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stavila@adobe.com/18080016/50001

[commit-bot: I haz the power, 2013-07-02 20:02:14]

Change committed as 153439