Bug 80134

Created attachment 130467 [details] Patch

Comment on attachment 130467 [details] Patch r=me

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 on attachment 130467 [details] Patch Implementing it using a static node list instead of a dynamic node list.

Created attachment 132487 [details] Patch 2

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.

Created attachment 132602 [details] Patch 3 Addressing comments and simplify the test. The node list is still implemented as a static node list.

Comment on attachment 132602 [details] Patch 3 r=me

Comment on attachment 132602 [details] Patch 3 Clearing flags on attachment: 132602 Committed r111229: <http://trac.webkit.org/changeset/111229>

All reviewed patches have been landed. Closing bug.

This caused bug 81679.

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.

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

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.

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 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.

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 on attachment 136677 [details] Patch 5 r=me

Comment on attachment 136677 [details] Patch 5 Clearing flags on attachment: 136677 Committed r114189: <http://trac.webkit.org/changeset/114189>

All reviewed patches have been landed. Closing bug.