Bug 80134 – [CSSRegions]Implement NamedFlow::contentNodes attribute

• Component: CSS
• Status: RESOLVED FIXED

[Mihnea Ovidenie]

http://www.w3.org/TR/css3-regions/#the-namedflow-interface

[Mihnea Ovidenie]

Created attachment 130467 [details]
Patch

[Dave Hyatt]

Comment on attachment 130467 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 130467 [details]
Patch

Rejecting attachment 130467 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ing file Source/WebCore/dom/Node.cpp
patching file Source/WebCore/dom/Node.h
patching file Source/WebCore/dom/NodeRareData.h
patching file Source/WebCore/dom/RegionNodeList.cpp
patching file Source/WebCore/dom/WebKitNamedFlow.cpp
patching file Source/WebCore/dom/WebKitNamedFlow.h
patching file Source/WebCore/dom/WebKitNamedFlow.idl

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Hyatt']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11851155

[Mihnea Ovidenie]

Comment on attachment 130467 [details]
Patch

Implementing it using a static node list instead of a dynamic node list.

[Mihnea Ovidenie]

Created attachment 132487 [details]
Patch 2

[Ryosuke Niwa]

Comment on attachment 132487 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=132487&action=review

> LayoutTests/fast/regions/webkit-named-flow-content-nodes-expected.txt:7
> +PASS
> +PASS
> +PASS

Please consider including some information about which test cases are passing. It makes debugging really hard if one of these passes turn into fail on some bots.

> Source/WebCore/dom/Document.cpp:1040
> +PassRefPtr<WebKitNamedFlow> Document::webkitGetFlowByName(const String& flowName, bool checkFlowName)

Please use enum instead of bool.

> Source/WebCore/dom/Document.cpp:1050
> +        CSSParser p(true);

Please don't use one-letter variables.

[Mihnea Ovidenie]

Created attachment 132602 [details]
Patch 3

Addressing comments and simplify the test. The node list is still implemented as a static node list.

[Dave Hyatt]

Comment on attachment 132602 [details]
Patch 3

r=me

[WebKit Review Bot]

Comment on attachment 132602 [details]
Patch 3

Clearing flags on attachment: 132602

Committed r111229: <http://trac.webkit.org/changeset/111229>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

This caused bug 81679.

[Brady Eidson]

It also caused <rdar://problem/11078925>, which is not in bugzilla yet (and I won't bother filing since we're already rolling it out.

<rdar://problem/11078925> is a crash closing a tab that has Reddit Enhancement Suite installed, in Safari.

The cruz was the computeStyle() in Element::detach, which seems like a very bad idea!

Reverted in <http://trac.webkit.org/r111419>.

(In reply to comment #11)
> This caused bug 81679.

Sorry, this should have said bug 81684.

[Brady Eidson]

(In reply to comment #12)
> The cruz was the computeStyle() in Element::detach, which seems like a very bad idea!

s/cruz/crux/

[Brady Eidson]

I'm not sure why it started that layout test failing, but the computeStyle() causing the crash seems like it should be easy to factor out.

Using the style as a way of tracking if the node needs to register/unregister itself from a collection seems extremely fragile.  There almost certainly needs to be a more explicit way of doing that.

[Mihnea Ovidenie]

Created attachment 133747 [details]
Patch 4

New version of the patch without relying on the style() to test whether an element is part of a named flow.

[Dave Hyatt]

Comment on attachment 133747 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=133747&action=review

r=me

> Source/WebCore/rendering/RenderFlowThread.h:167
> +    NamedFlowContentNodes m_contentNodes;
> +
>      AtomicString m_flowThread;
>      RenderRegionList m_regionList;

This isn't really something you should do for this patch, but going forward I think we might want to pull all the flow thread stuff in RenderView into a helper class.

[Mihnea Ovidenie]

Created attachment 136677 [details]
Patch 5

Rework the patch after https://bugs.webkit.org/show_bug.cgi?id=83464, by moving data/methods from RenderView into FlowThreadController.

[Dave Hyatt]

Comment on attachment 136677 [details]
Patch 5

r=me

[WebKit Review Bot]

Comment on attachment 136677 [details]
Patch 5

Clearing flags on attachment: 136677

Committed r114189: <http://trac.webkit.org/changeset/114189>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.