Bug 80134

Summary: [CSSRegions]Implement NamedFlow::contentNodes attribute
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, beidson, mitz, ojan, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3
none
Patch 4
hyatt: review+
Patch 5 none

Comment 1 Mihnea Ovidenie 2012-03-06 16:30:21 PST
Created attachment 130467 [details]
Patch
Comment 2 Dave Hyatt 2012-03-07 09:10:47 PST
Comment on attachment 130467 [details]
Patch

r=me
Comment 3 WebKit Review Bot 2012-03-07 09:16:28 PST
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
Comment 4 Mihnea Ovidenie 2012-03-16 00:40:33 PDT
Comment on attachment 130467 [details]
Patch

Implementing it using a static node list instead of a dynamic node list.
Comment 5 Mihnea Ovidenie 2012-03-18 02:48:04 PDT
Created attachment 132487 [details]
Patch 2
Comment 6 Ryosuke Niwa 2012-03-19 00:04:02 PDT
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.
Comment 7 Mihnea Ovidenie 2012-03-19 10:20:09 PDT
Created attachment 132602 [details]
Patch 3

Addressing comments and simplify the test. The node list is still implemented as a static node list.
Comment 8 Dave Hyatt 2012-03-19 12:47:42 PDT
Comment on attachment 132602 [details]
Patch 3

r=me
Comment 9 WebKit Review Bot 2012-03-19 13:24:37 PDT
Comment on attachment 132602 [details]
Patch 3

Clearing flags on attachment: 132602

Committed r111229: <http://trac.webkit.org/changeset/111229>
Comment 10 WebKit Review Bot 2012-03-19 13:24:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 mitz 2012-03-20 11:25:22 PDT
This caused bug 81679.
Comment 12 Brady Eidson 2012-03-20 11:37:37 PDT
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!
Comment 13 mitz 2012-03-20 11:38:40 PDT
Reverted in <http://trac.webkit.org/r111419>.
Comment 14 mitz 2012-03-20 11:39:46 PDT
(In reply to comment #11)
> This caused bug 81679.

Sorry, this should have said bug 81684.
Comment 15 Brady Eidson 2012-03-20 11:42:24 PDT
(In reply to comment #12)
> The cruz was the computeStyle() in Element::detach, which seems like a very bad idea!

s/cruz/crux/
Comment 16 Brady Eidson 2012-03-20 11:43:57 PDT
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.
Comment 17 Mihnea Ovidenie 2012-03-26 01:29:49 PDT
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.
Comment 18 Dave Hyatt 2012-04-04 12:25:31 PDT
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.
Comment 19 Mihnea Ovidenie 2012-04-11 09:31:46 PDT
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.
Comment 20 Dave Hyatt 2012-04-13 10:35:47 PDT
Comment on attachment 136677 [details]
Patch 5

r=me
Comment 21 WebKit Review Bot 2012-04-13 17:36:30 PDT
Comment on attachment 136677 [details]
Patch 5

Clearing flags on attachment: 136677

Committed r114189: <http://trac.webkit.org/changeset/114189>
Comment 22 WebKit Review Bot 2012-04-13 17:36:41 PDT
All reviewed patches have been landed.  Closing bug.