Bug 89000 – [CSS Regions] Fix the lifecycle for the flow objects and their renderers

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Bucur]

Currently, the Regions implementation is WebKit doesn't respect the state machine described in the latest spec draft http://www.w3.org/TR/css3-regions/#cssom_view_and_css_regions . For example, getFlowByName will return a non-null value for un-existing named flows (neither flow-into/flow-from are specified for that name). Even more, a renderer will be created for that flow.

[Andrei Bucur]

Created attachment 147353 [details]
Patch

[Andrei Bucur]

(In reply to comment #1)
> Created an attachment (id=147353) [details]
> Patch

The patch has still some work to do. I've uploaded it to get feedback from the community on the approach and style.

[Andrei Bucur]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=147353) [details] [details]
> > Patch
> 
> The patch has still some work to do. I've uploaded it to get feedback from the community on the approach and style.

Some "cosmetic" work. I think it's ready from a functional point of view.

[WebKit Review Bot]

Comment on attachment 147353 [details]
Patch

Attachment 147353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12956148

New failing tests:
fast/regions/webkit-named-flow-overset.html
fast/regions/region-style-block-background-color2.html

[WebKit Review Bot]

Created attachment 147448 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Andrei Bucur]

Created attachment 147590 [details]
Patch

[Andrei Bucur]

Created attachment 147600 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147600 [details]
Patch

Attachment 147600 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12959371

New failing tests:
fast/regions/region-style-block-background-color2.html

[WebKit Review Bot]

Created attachment 147652 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Andrei Bucur]

Created attachment 147776 [details]
Patch

[Andrei Bucur]

Created attachment 147790 [details]
Patch

[Alexandru Chiculita]

Comment on attachment 147790 [details]
Patch

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

I don't see the need for having two lists. You only need to make sure that getFlowByName returns null if there's no Renderer set on the NamedFlow and when the NamedFlow is destroyed you have to remove it from the list.

> Source/WebCore/dom/Document.cpp:499
> +    , m_namedFlows(WebKitNamedFlowCollection::create(this))

Can we allocate this only when needed?

> Source/WebCore/dom/Document.cpp:1152
>              return 0;

Do we still need to check for a valid flow name? It seems like getFlowByName will not create a flow if it doesn't exist already anyway.

> Source/WebCore/dom/WebKitNamedFlow.cpp:41
> +: m_flowThreadName(flowThreadName)

Nit: there needs to be 4 spaces before the colon.

> Source/WebCore/dom/WebKitNamedFlow.cpp:42
> +, m_document(doc)

Why can't we just store a RefPtr to the named flow collection directly?

> Source/WebCore/dom/WebKitNamedFlow.cpp:61
> +    if (m_parentFlowThread)
> +        m_parentFlowThread->document()->updateLayoutIgnorePendingStylesheets();

nit: It would be easier to read if it was: 
if (!m_parentFlowThread)
   return true;
m_parentFlowThread->document()->updateLayoutIgnorePendingStylesheets();

> Source/WebCore/dom/WebKitNamedFlow.cpp:65
> +        return m_parentFlowThread->overset();

nit: I would go for return m_parentFlowThread ? m_parentFlowThread->overset() : true; instead.

> Source/WebCore/dom/WebKitNamedFlow.cpp:75
>      Vector<RefPtr<Node> > regionNodes;

nit: If you do the following changes, move regionNodes where it is actually used (do not allocate it without having to).

> Source/WebCore/dom/WebKitNamedFlow.cpp:77
> +    if (m_parentFlowThread)

nit: maybe just "if (!m_parentFlowThread)\n    return 0;" instead.

> Source/WebCore/dom/WebKitNamedFlow.cpp:81
> +    if (m_parentFlowThread) {

Ditto

> Source/WebCore/dom/WebKitNamedFlow.cpp:101
>      Vector<RefPtr<Node> > contentNodes;

Ditto - move it where it is needed

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:86
> +    // The document is not valid anymore so the collection will be destroyed anyway

It seems like this check would not be necessary. I don't see the need for a m_document reference at all.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:94
> +    m_createdFlows.remove(m_createdFlows.find(namedFlow));

I don't really like using the un-managed lists.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:106
> +    m_nullFlows.remove(m_nullFlows.find(namedFlow));
> Source/WebCore/dom/WebKitNamedFlowCollection.h:65
> +    typedef Vector<RefPtr<WebKitNamedFlow> > CreatedNamedFlowVector;

Try HashSet instead of Vector. I suppose a HashSet would do a better job at adding/removing/searching.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> +    if (canBeDestroyed())
> +        destroy();
> +    else
> +        invalidateRegions();

Usually it's easier to read the code if you return early.

if (canBeDestroyed()) {
   destroy();
   return;
}
invalidateRegions();

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:-25
> -            var namedFlowContentNodes = document.webkitGetFlowByName("flow").contentNodes;

Maybe you could check that document.webkitGetFlowByName("flow") == null;

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:-34
> -            var namedFlowContentNodes2 = document.webkitGetFlowByName("flow").contentNodes;

Ditto.

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:117
>              var namedFlowContentNodes13 = document.webkitGetFlowByName("flow").contentNodes;

I suppose this one needs to return "null" even though you have a reference to the old flow on the stack.

> LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:25
> +            // It should not be possible to get the named flow object for a valid flow name.

The comment seems to be incomplete :D -> It should not be possible to get the named flow object for a flow name *which has no region or content*?

[Andrei Bucur]

Comment on attachment 147790 [details]
Patch

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

The not-so-strong reason there are two lists is inline.

>> Source/WebCore/dom/Document.cpp:499
>> +    , m_namedFlows(WebKitNamedFlowCollection::create(this))
> 
> Can we allocate this only when needed?

I suppose :).

>> Source/WebCore/dom/Document.cpp:1152
>>              return 0;
> 
> Do we still need to check for a valid flow name? It seems like getFlowByName will not create a flow if it doesn't exist already anyway.

Sounds good.

>> Source/WebCore/dom/WebKitNamedFlow.cpp:41
>> +: m_flowThreadName(flowThreadName)
> 
> Nit: there needs to be 4 spaces before the colon.

Will do.

>> Source/WebCore/dom/WebKitNamedFlow.cpp:42
>> +, m_document(doc)
> 
> Why can't we just store a RefPtr to the named flow collection directly?

I thought I'll need the document for when this becomes an EventTarget, but that may not be true (I'll need to see what's the best way to keep the JS wrapper from being destroyed while listeners are present on the NamedFlow; maybe just mark it as a root for GC if hasEventListeners is true).

>> Source/WebCore/dom/WebKitNamedFlow.cpp:61
>> +        m_parentFlowThread->document()->updateLayoutIgnorePendingStylesheets();
> 
> nit: It would be easier to read if it was: 
> if (!m_parentFlowThread)
>    return true;
> m_parentFlowThread->document()->updateLayoutIgnorePendingStylesheets();

Will do.

>> Source/WebCore/dom/WebKitNamedFlow.cpp:65
>> +        return m_parentFlowThread->overset();
> 
> nit: I would go for return m_parentFlowThread ? m_parentFlowThread->overset() : true; instead.

Will do.

>> Source/WebCore/dom/WebKitNamedFlow.cpp:75
>>      Vector<RefPtr<Node> > regionNodes;
> 
> nit: If you do the following changes, move regionNodes where it is actually used (do not allocate it without having to).

This API will always return a list if I read the spec correctly. If the NamedFlow is in the NULL state, that will be an empty list. So I need that list anyway.

>> Source/WebCore/dom/WebKitNamedFlow.cpp:77
>> +    if (m_parentFlowThread)
> 
> nit: maybe just "if (!m_parentFlowThread)\n    return 0;" instead.

Will do. This was mostly copy/pasted from the old code without wrapping it up nicely in the new design. I'll fix it.

>> Source/WebCore/dom/WebKitNamedFlow.cpp:101
>>      Vector<RefPtr<Node> > contentNodes;
> 
> Ditto - move it where it is needed

Same as above, the return value is never null.

>> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:86
>> +    // The document is not valid anymore so the collection will be destroyed anyway
> 
> It seems like this check would not be necessary. I don't see the need for a m_document reference at all.

I wanted to use the Document reference to keep the JS wrapper alive while the document is alive (see JSGenerateIsReachable attribute on IDL interfaces).

>> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:94
>> +    m_createdFlows.remove(m_createdFlows.find(namedFlow));
> 
> I don't really like using the un-managed lists.

You suggested using only one list for all the NamedFlows. In this case the list should be non-managed and assume that the renderer will have a strong ref to his named flow object. The idea for the non-managed list is to allow destruction for NULL state NamedFlows that are neither referenced from JS nor have any listeners on them. Once this happens, they can be removed from the NULL flows bucket.
I've used two lists to make this concepts clearer. If you think it's not necessary, I'll remove it and make only one, non-managed (or if you have another suggestion :) ).

>> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:106
>> +    m_nullFlows.remove(m_nullFlows.find(namedFlow));
> 
> Try HashSet instead of Vector. I suppose a HashSet would do a better job at adding/removing/searching.

Been there, done that. The problem with the HashSet (ListHashSet more precisely) is that item(index) will not be O(1). From my understanding item() is used to implement the JS collection so having a for through the collection in JS will actually be O(n^2).

>> Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
>> +        invalidateRegions();
> 
> Usually it's easier to read the code if you return early.
> 
> if (canBeDestroyed()) {
>    destroy();
>    return;
> }
> invalidateRegions();

Will do :).

>> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:-25
>> -            var namedFlowContentNodes = document.webkitGetFlowByName("flow").contentNodes;
> 
> Maybe you could check that document.webkitGetFlowByName("flow") == null;

This test is for something else, I didn't want to mix testing NULL/CREATED behavior with contentNodes API. I'll add it anyway.

>> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:117
>>              var namedFlowContentNodes13 = document.webkitGetFlowByName("flow").contentNodes;
> 
> I suppose this one needs to return "null" even though you have a reference to the old flow on the stack.

Yes, the API call returns null.

>> LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:25
>> +            // It should not be possible to get the named flow object for a valid flow name.
> 
> The comment seems to be incomplete :D -> It should not be possible to get the named flow object for a flow name *which has no region or content*?

Eh... rush hour :).

[Andrei Bucur]

Also, a NULL named flow that has listeners and goes back in the CREATED state needs to be reused. This is why keeping track of them is important.

[Andrei Bucur]

Comment on attachment 147790 [details]
Patch

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

>>> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:117
>>>              var namedFlowContentNodes13 = document.webkitGetFlowByName("flow").contentNodes;
>> 
>> I suppose this one needs to return "null" even though you have a reference to the old flow on the stack.
> 
> Yes, the API call returns null.

No, because the regionNode is still in the DOM. The return value is non-null.

[Andrei Bucur]

Created attachment 148108 [details]
Patch

[Andrei Bucur]

Created attachment 148118 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 148118 [details]
Patch

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

Just a quick style scan, but the code seems clean to my eye.

> Source/WebCore/ChangeLog:6
> +        This patch adds the concept of a NamedFlowCollection, owned by the document, that keeps track of

Reviewers say that the verbose description should reside AFTER the "Reviewed by" line (you may want to ask for another opinion, since I'm also used to the text immediately following the bug summary).

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28243
> +				SYMROOT = /Users/abucur/NonPerforce/WebKit/WebKitBuild/namedflow_lifecycle;

I believe this line is somewhat wrong...

> Source/WebCore/dom/Document.cpp:1129
> +    // It's possible to have pendiding styles not applied that affect the existing flows

Comments should end with a period (http://www.webkit.org/coding/coding-style.html#comments-sentences). This should be fixed in all comments you have added.

> LayoutTests/ChangeLog:6
> +        These tests expected that getFlowByName() to return a valid object even for the NULL flows. They've been modified to

stray "that" before "getFlowByName()"

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:28
> +			

FYI: there's a lot of leading whitespace here

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:29
> +			shouldBeNull('document.webkitGetFlowByName("flow")');

Is this out-of-order indentation intentional?

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:120
> +			shouldBeNull('document.webkitGetFlowByName("flow2")');

Ditto.

[Andrei Bucur]

Created attachment 148346 [details]
Patch

[Tim Horton]

Comment on attachment 148346 [details]
Patch

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

So, this patch is way beyond me, but I had a few little nitpicks:

> Source/WebCore/CMakeLists.txt:667
> +    dom/WebKitNamedFlowCollection.cpp

It seems vaguely odd to me that the filenames are WebKit-prefixed as well, but it seems like this isn't even remotely the first one, so I guess it's OK. Class names, too.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:54468
> +				<FileConfiguration
> +					Name="Debug|Win32"
> +					ExcludedFromBuild="true"
> +					>
> +					<Tool
> +						Name="VCCLCompilerTool"
> +					/>
> +				</FileConfiguration>
> +				<FileConfiguration
> +					Name="Release|Win32"
> +					ExcludedFromBuild="true"
> +					>
> +					<Tool
> +						Name="VCCLCompilerTool"
> +					/>
> +				</FileConfiguration>
> +				<FileConfiguration
> +					Name="Debug_Cairo_CFLite|Win32"
> +					ExcludedFromBuild="true"
> +					>
> +					<Tool
> +						Name="VCCLCompilerTool"
> +					/>
> +				</FileConfiguration>
> +				<FileConfiguration
> +					Name="Release_Cairo_CFLite|Win32"
> +					ExcludedFromBuild="true"
> +					>
> +					<Tool
> +						Name="VCCLCompilerTool"
> +					/>
> +				</FileConfiguration>
> +				<FileConfiguration
> +					Name="Debug_All|Win32"
> +					ExcludedFromBuild="true"
> +					>
> +					<Tool
> +						Name="VCCLCompilerTool"
> +					/>
> +				</FileConfiguration>
> +				<FileConfiguration
> +					Name="Production|Win32"
> +					ExcludedFromBuild="true"
> +					>
> +					<Tool
> +						Name="VCCLCompilerTool"
> +					/>
> +				</FileConfiguration>
> +			</File>

I wonder if you meant for all this VCCLCompilerTool stuff to be here?

> Source/WebCore/dom/Document.cpp:1129
> +    // It's possible to have pendiding styles not applied that affect the existing flows.

s/pendiding/pending/, I think.

> Source/WebCore/dom/WebKitNamedFlow.cpp:106
> +        for (NamedFlowContentNodes::const_iterator it = m_parentFlowThread->contentNodes().begin(); it != m_parentFlowThread->contentNodes().end(); ++it) {

Could grab contentNodes() just once and make this line somewhat less long.

> Source/WebCore/dom/WebKitNamedFlow.cpp:107
> +            Node* node = const_cast<Node*>(*it);

Can you reassure my innate fear of const_cast in this case? (i.e. explain why it's safe)

> Source/WebCore/dom/WebKitNamedFlow.h:62
> +        STATE_CREATED,
> +        STATE_NULL

I think we usually use UpperCamelCase for enum members. And maybe nicer names, too. (FlowCreatedState, FlowNullState, maybe? I don't know)

> Source/WebCore/rendering/FlowThreadController.cpp:72
> +    // Sanity check for the inexistence of a named flow in the "CREATED" state with the same name.

"inexistence" is not a particularly common word in English (to the extent that my spell-check doesn't even accept it). I would go with "absence" or something like that.

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:21
> -                window.layoutTestController.dumpAsText();
> +            window.layoutTestController.dumpAsText();

I think you broke the indentation here.

> LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:25
> +            // It should not be possible to get the NamedFlow object for an inexistent flow.

Ditto on the inexistent thing.

> LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:27
> -            assert(namedFlow != null, "Named flow for flow should not be null");
> +            assert(namedFlow == null, "Named flow for flow should not be null");

Did you mean to change the assert comment string here?

[Andrei Bucur]

(In reply to comment #20)
> (From update of attachment 148346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148346&action=review
> 
> So, this patch is way beyond me, but I had a few little nitpicks:
> 
> > Source/WebCore/CMakeLists.txt:667
> > +    dom/WebKitNamedFlowCollection.cpp
> 
> It seems vaguely odd to me that the filenames are WebKit-prefixed as well, but it seems like this isn't even remotely the first one, so I guess it's OK. Class names, too.
I thought the decision to prefix the NamedFlow class had some good reasons behind it so I've made the same thing for the collection.

> 
> > Source/WebCore/WebCore.vcproj/WebCore.vcproj:54468
> > +				<FileConfiguration
> > +					Name="Debug|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Release|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Debug_Cairo_CFLite|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Release_Cairo_CFLite|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Debug_All|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Production|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +			</File>
> 
> I wonder if you meant for all this VCCLCompilerTool stuff to be here?
I hand-crafted this part to be similar with the configuration for the WebKitNamedFlow.* files :). You're saying that they are not actually required?

> 
> > Source/WebCore/dom/Document.cpp:1129
> > +    // It's possible to have pendiding styles not applied that affect the existing flows.
> 
> s/pendiding/pending/, I think.

Will do, thx!
> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:106
> > +        for (NamedFlowContentNodes::const_iterator it = m_parentFlowThread->contentNodes().begin(); it != m_parentFlowThread->contentNodes().end(); ++it) {
> 
> Could grab contentNodes() just once and make this line somewhat less long.
Yes.

> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:107
> > +            Node* node = const_cast<Node*>(*it);
> 
> Can you reassure my innate fear of const_cast in this case? (i.e. explain why it's safe)
This code was inherited from the old version :). However, I think the const_cast should be safe because the original Node* HashSet on the renderer doesn't have any const-ness attached to it.

> 
> > Source/WebCore/dom/WebKitNamedFlow.h:62
> > +        STATE_CREATED,
> > +        STATE_NULL
> 
> I think we usually use UpperCamelCase for enum members. And maybe nicer names, too. (FlowCreatedState, FlowNullState, maybe? I don't know)
Sounds good.

> 
> > Source/WebCore/rendering/FlowThreadController.cpp:72
> > +    // Sanity check for the inexistence of a named flow in the "CREATED" state with the same name.
> 
> "inexistence" is not a particularly common word in English (to the extent that my spell-check doesn't even accept it). I would go with "absence" or something like that.
Yea, this is why I had the feeling I've never heard that word before. I'll replace it.

> 
> > LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:21
> > -                window.layoutTestController.dumpAsText();
> > +            window.layoutTestController.dumpAsText();
> 
> I think you broke the indentation here.
Yep :).

> 
> > LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:25
> > +            // It should not be possible to get the NamedFlow object for an inexistent flow.
> 
> Ditto on the inexistent thing.
:)

> 
> > LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:27
> > -            assert(namedFlow != null, "Named flow for flow should not be null");
> > +            assert(namedFlow == null, "Named flow for flow should not be null");
> 
> Did you mean to change the assert comment string here?
I just wanted to confuse people reading that. J/k I'll update it :).

Thanks for the review! Update coming later today.

[Tim Horton]

(In reply to comment #21)
> > It seems vaguely odd to me that the filenames are WebKit-prefixed as well, but it seems like this isn't even remotely the first one, so I guess it's OK. Class names, too.
> I thought the decision to prefix the NamedFlow class had some good reasons behind it so I've made the same thing for the collection.

Ok, seems reasonable.

> > I wonder if you meant for all this VCCLCompilerTool stuff to be here?
> I hand-crafted this part to be similar with the configuration for the WebKitNamedFlow.* files :). You're saying that they are not actually required?

I'm not positive, but maybe! Would be worth checking briefly, but not much more than that.

> > Can you reassure my innate fear of const_cast in this case? (i.e. explain why it's safe)
> This code was inherited from the old version :). However, I think the const_cast should be safe because the original Node* HashSet on the renderer doesn't have any const-ness attached to it.

Sure!

> Thanks for the review! Update coming later today.

Excellent.

[Andrei Bucur]

Created attachment 148765 [details]
Patch

[Andrei Bucur]

Created attachment 149007 [details]
Patch

[Andrei Bucur]

Created attachment 149024 [details]
Patch

[Andrei Bucur]

Comment on attachment 149024 [details]
Patch

I resent the patch because the it didn't apply any more.

[Eric Seidel (no email)]

Comment on attachment 149024 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:499
> +    , m_namedFlows(0)

If m_namedFlows is a smart pointer, this is not needed.

> Source/WebCore/dom/Document.h:1495
> +    RefPtr<WebKitNamedFlowCollection> m_namedFlows;

Yup, the m_namedFlows(0) is not needed, since RefPtr has a default constructor.

> Source/WebCore/dom/WebKitNamedFlow.cpp:81
> +    if (m_parentFlowThread) {
> +        if (contentNode->renderer()

I'm always suspicious of long if blocks, and wonder if helper functions shouldn't be used instead (since they allow for easy early-return.

> Source/WebCore/dom/WebKitNamedFlow.cpp:110
> +            Node* node = const_cast<Node*>(*it);

Sad times.  Maybe we should be marking namedFlows() as const, and m_namedFlows as mutable?  Or maybe we should just use a non-const iterator here? const Nodes don't really make much sense anyway, since they're ref counted.  So the non-const iterator (if available) Seems to make the most sense.

> Source/WebCore/dom/WebKitNamedFlow.h:43
> +class WebKitNamedFlowCollection;

This is not needed if you have the include too.

> Source/WebCore/dom/WebKitNamedFlow.h:47
> +    static PassRefPtr<WebKitNamedFlow> create(PassRefPtr<WebKitNamedFlowCollection> manager, const AtomicString& flowThreadName)

This doesn't really need to be in the header, but is the reason why you need to include WebKitNamedFlowColection.h (to instantiate the PassRefPtr).

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:53
> +    return m_createdFlowsCount;

When is this different from m_namedFlows.length?

> Source/WebCore/dom/WebKitNamedFlowCollection.h:43
> +class WebKitNamedFlowCollection : public RefCounted<WebKitNamedFlowCollection> {

Why is this named WebKitNamedFlowCollection?  Normally we would just use "NamedFlowCollection", no?  Even if this is currently exposed to JS with WebKit in its name, we can easily do that in the IDL instead of renaming the c++ clas.

> Source/WebCore/dom/WebKitNamedFlowCollection.h:46
> +    ~WebKitNamedFlowCollection();

Are you declaring the destructor to avoid including headers in this header? or did you have other plans for an explicit destructor?

> Source/WebCore/dom/WebKitNamedFlowCollection.h:49
> +    WebKitNamedFlow* item(unsigned index) const;

I never know if unsigned or size_t is right.

> Source/WebCore/dom/WebKitNamedFlowCollection.h:51
> +    WebKitNamedFlow* getFlowByName(const String&);

We tend to be anti-"get" in function names.  It may make sense here, since this is a lookup.  But lookupFlowByName() or flowByName() or flowFromName() or flowWithName() might all be more webkitty. http://www.webkit.org/coding/coding-style.html#names-setter-getter

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> +    if (canBeDestroyed()) {
> +        destroy();
> +        return;
> +    }

NamedFlowThreads manage their own lifetime?  That's a bit odd to me.

> Source/WebCore/rendering/RenderNamedFlowThread.h:50
> +    AtomicString flowThreadName() const;

This could prossibly const AtomicString&, depending on if this is perf senstiive or not. (I doubt it is).  That would avoid one ref churn.

[Andrei Bucur]

(In reply to comment #27)
> (From update of attachment 149024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149024&action=review
> 
> > Source/WebCore/dom/Document.cpp:499
> > +    , m_namedFlows(0)
> 
> If m_namedFlows is a smart pointer, this is not needed.
Will fix.

> 
> > Source/WebCore/dom/Document.h:1495
> > +    RefPtr<WebKitNamedFlowCollection> m_namedFlows;
> 
> Yup, the m_namedFlows(0) is not needed, since RefPtr has a default constructor.
> 
Idem.

> > Source/WebCore/dom/WebKitNamedFlow.cpp:81
> > +    if (m_parentFlowThread) {
> > +        if (contentNode->renderer()
> 
> I'm always suspicious of long if blocks, and wonder if helper functions shouldn't be used instead (since they allow for easy early-return.
I'll wrap it up nicely.

> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:110
> > +            Node* node = const_cast<Node*>(*it);
> 
> Sad times.  Maybe we should be marking namedFlows() as const, and m_namedFlows as mutable?  Or maybe we should just use a non-const iterator here? const Nodes don't really make much sense anyway, since they're ref counted.  So the non-const iterator (if available) Seems to make the most sense.
Ok, I'll search for the best option to remove that cast.

> 
> > Source/WebCore/dom/WebKitNamedFlow.h:43
> > +class WebKitNamedFlowCollection;
> 
> This is not needed if you have the include too.
> 
> > Source/WebCore/dom/WebKitNamedFlow.h:47
> > +    static PassRefPtr<WebKitNamedFlow> create(PassRefPtr<WebKitNamedFlowCollection> manager, const AtomicString& flowThreadName)
> 
> This doesn't really need to be in the header, but is the reason why you need to include WebKitNamedFlowColection.h (to instantiate the PassRefPtr).
Makes sense, I'll move it outside the header.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:53
> > +    return m_createdFlowsCount;
> 
> When is this different from m_namedFlows.length?
The m_namedFlows set contains both the CREATE and NULL flows. The API is used to retrieve only the CREATED flows. The NULL flows are only cached until they are destroyed (e.g. when there are no more references from JS, there are no JS listeners for "regionLayoutUpdate" on the flows etc.). We need to keep track of the NULL flows because it's possible for a script to reactivate them and it shouldn't be required for an author to re-add the listeners.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:43
> > +class WebKitNamedFlowCollection : public RefCounted<WebKitNamedFlowCollection> {
> 
> Why is this named WebKitNamedFlowCollection?  Normally we would just use "NamedFlowCollection", no?  Even if this is currently exposed to JS with WebKit in its name, we can easily do that in the IDL instead of renaming the c++ clas.
I've named it similar to how WebKitNamedFlow is called. If I rename this one, should the same be done for WebKitNamedFlow. I'm not familiar with the reasons why WebKitNamedFlow was prefixed like this.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:46
> > +    ~WebKitNamedFlowCollection();
> 
> Are you declaring the destructor to avoid including headers in this header? or did you have other plans for an explicit destructor?
Just a leftover from a previous version. I'll remove it.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:49
> > +    WebKitNamedFlow* item(unsigned index) const;
> 
> I never know if unsigned or size_t is right.
I've inspired from the NodeList class :). Also, the JS wrappers are generated with unsigned (probably because this is how the IDL defines the param).

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:51
> > +    WebKitNamedFlow* getFlowByName(const String&);
> 
> We tend to be anti-"get" in function names.  It may make sense here, since this is a lookup.  But lookupFlowByName() or flowByName() or flowFromName() or flowWithName() might all be more webkitty. http://www.webkit.org/coding/coding-style.html#names-setter-getter
I'll rename it accordingly :). It was a left-over from the old version that was an IDL method.

> 
> > Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> > +    if (canBeDestroyed()) {
> > +        destroy();
> > +        return;
> > +    }
> 
> NamedFlowThreads manage their own lifetime?  That's a bit odd to me.
The flow thread renderers lifetimes depend on the style changes, on how content nodes/regions are added to the flow. This was a compromise between an easy to understand solution and over-engineering to delegate it to some other component (most adequate is probably the FlowThreadController). With this patch, when a region or content node is removed, we just check if the flow still exists in the Document. If not, the renderer is deleted. Maybe I can add comments in code that the renderer might be destroyed after the region chain is shrinked/content nodes are removed.

> 
> > Source/WebCore/rendering/RenderNamedFlowThread.h:50
> > +    AtomicString flowThreadName() const;
> 
> This could prossibly const AtomicString&, depending on if this is perf senstiive or not. (I doubt it is).  That would avoid one ref churn.
I'll update this as well.

[Andrei Bucur]

Created attachment 150589 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 150589 [details]
Patch

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

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:95
> +    m_namedFlows.add(newFlow.get());
> +    ++m_createdFlowsCount;

It's not clear to me why it's better to have a single unified list, and a side-cound, instead of two lists?  can flowByName() find non-created flows?  What advantages do we get by having them all in one set?

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:129
> +    static const bool safeToCompareToEmptyOrDeleted = true;

Why is this true?  Won't the equal function crash if passed an empty or deleted object?  I'm not really sure what this does.

> Source/WebCore/rendering/RenderNamedFlowThread.h:69
> +    NamedFlowContentNodes& contentNodes() { return m_contentNodes; }

I think normally we return things by pointer, but I'm also not sure it matters.  Why is this non-const now?

> Source/WebCore/rendering/RenderNamedFlowThread.h:83
> +    inline bool canBeDestroyed() const { return m_regionList.isEmpty() && m_contentNodes.isEmpty(); }

inline does nothing here.  all methods declared in the header are implicity "inline" as far as I know.  "inline" is just a hint to the compiler. :)

[Andrei Bucur]

(In reply to comment #30)
> (From update of attachment 150589 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150589&action=review
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:95
> > +    m_namedFlows.add(newFlow.get());
> > +    ++m_createdFlowsCount;
> 
> It's not clear to me why it's better to have a single unified list, and a side-cound, instead of two lists?  can flowByName() find non-created flows?  What advantages do we get by having them all in one set?
With two sets it would've been necessary to move named flows from a set to another when the state of the flows changed (basically keeping the sets in sync with the flowState value). Having only one set and a count for the created flows looks less convoluted to me.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:129
> > +    static const bool safeToCompareToEmptyOrDeleted = true;
> 
> Why is this true?  Won't the equal function crash if passed an empty or deleted object?  I'm not really sure what this does.
This is an optimization for the compiler when generating the template class to not check if the values in the HashSet (backed by a HashTable) are empty or 0. In our case, all the values in the set are always removed when destroyed (WebKitNamedFlow destructor). So this optimization applies. If you find it confusing, I can remove it or comment it.

> 
> > Source/WebCore/rendering/RenderNamedFlowThread.h:69
> > +    NamedFlowContentNodes& contentNodes() { return m_contentNodes; }
> 
> I think normally we return things by pointer, but I'm also not sure it matters.  Why is this non-const now?
Looking at it now, there's a better method to remove the const_cast in the previous patch. I'll remake it const.
> 
> > Source/WebCore/rendering/RenderNamedFlowThread.h:83
> > +    inline bool canBeDestroyed() const { return m_regionList.isEmpty() && m_contentNodes.isEmpty(); }
> 
> inline does nothing here.  all methods declared in the header are implicity "inline" as far as I know.  "inline" is just a hint to the compiler. :)
Learning everyday something new :).

[Andrei Bucur]

Created attachment 151436 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 151436 [details]
Patch

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

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:65
> +    NamedFlowSet::const_iterator end = m_namedFlows.end();
> +    for (NamedFlowSet::const_iterator it = m_namedFlows.begin(); it != end; ++it) {
> +        WebKitNamedFlow* namedFlow = *it;
> +        if (namedFlow->getFlowState() == WebKitNamedFlow::FlowStateCreated) {
> +            if (!index)
> +                return namedFlow;
> +            --index;
> +        }
> +    }

So this implementation has an odd quirk that if a flow is instantiated (NULL) and then later moved to (CREATED) it will push all the observable flows after it "down by one" in the index.  Basically, iterating this collection is no-where-near stable.  I guess that's generally true of live collections like this.  Seems like an odd spec/implementation choice.

Also seems like a behavior we could test.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:77
> +WebKitNamedFlow* WebKitNamedFlowCollection::flowByName(const String& flowName)
> +{
> +    NamedFlowSet::iterator it = m_namedFlows.find<String, NamedFlowHashTranslator>(flowName);
> +    if (it == m_namedFlows.end() || (*it)->getFlowState() == WebKitNamedFlow::FlowStateNull)
> +        return 0;
> +
> +    return *it;
> +}

I assume it's spec'd that these flows are ordered and flows with duplicate names return the first?

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> +    if (canBeDestroyed()) {
> +        destroy();
> +        return;
> +    }

Is this some sort of ref-counting solution?

[Eric Seidel (no email)]

Comment on attachment 151436 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1129
> +    // It's possible to have pending styles not applied that affect the existing flows.
> +    updateStyleIfNeeded();

Why do flows care if style is up to date?  Is that when the from-into stuff is resolved?

> Source/WebCore/dom/WebKitNamedFlow.cpp:50
> +    // The named flow is not "strong" referenced from anywhere at this time so it shouldn't be reused if the named flow is recreated.

This comment doesn't immediately make sense to me.  It seems we're just telling the flowManager to clear any weak pointers it has to us.

> Source/WebCore/dom/WebKitNamedFlow.cpp:70
> +    if (m_flowManager->document())
> +        m_flowManager->document()->updateLayoutIgnorePendingStylesheets();
> +
> +    // The renderer may be destroyed or created after the style update.
> +    // Because this is called from JS, where the wrapper keeps a reference to the NamedFlow, no guard is necessary.

It's still not clear to me why we care about renders or styles being up to date here?

> Source/WebCore/dom/WebKitNamedFlow.cpp:160
> +void WebKitNamedFlow::setRenderer(RenderNamedFlowThread* parentFlowThread)
> +{
> +    // The named flow can either go from a no_renderer->renderer or renderer->no_renderer state; anything else could indicate a bug.
> +    ASSERT((!m_parentFlowThread && parentFlowThread) || (m_parentFlowThread && !parentFlowThread));
> +
> +    m_parentFlowThread = parentFlowThread;
> +
> +    // A named flow that loses the renderer is in the "NULL" state.
> +    if (!m_parentFlowThread)
> +        m_flowManager->moveNamedFlowToNullState(this);
> +}

This is how we manage the CREATED vs. NULL states?

The named flow states are :

NULL: the named flow is not referenced by any ‘flow-into’ or ‘flow-from’ computed value.
CREATED: the named flow is referenced by at least one ‘flow-into’ or ‘flow-from’ computed value.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:50
> +unsigned WebKitNamedFlowCollection::length() const
> +{
> +    return m_createdFlowsCount;
> +}

It's not immediately clear to me that this is the right implementation choice (of keeping everything in one set and having a cached value of the number of "CREATED" flows).  Alternate implementations would include:
1.  Walking the set each time to count the number of created flows
2.  Keeping the NULL flows separate and moving them into the CREATED set when they transition.

Both would have observable consequences to JavaScript, so we'd need to better understnd what the intention of the spec is.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:105
> +    namedFlow->switchFlowState();

I think this is less clear than ->setFlowState(NullState)

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:113
> +    // The document is not valid anymore so the collection will be destroyed anyway.
> +    if (!m_document)
> +        return;

Really?  Is this going to be OK?  Are we careful to check m_document in all our accessors? Or is someone going to clear our document and then try and call something like item(0) and end up crashing when we hand them back a destroyed flow?

Could someone grab a pointer to this flow collection and then have it outlive the document?  Seems possible from JavaScript once this is exposed.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:130
> +    static bool equal(WebKitNamedFlow* a, WebKitNamedFlow* b) { return a->name() == b->name(); }

So WebKitNamedFlow objects are entirely governed by thier names?  Why would we ever have mor than one object if they're uniqued on name?

>> Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
>> +    }
> 
> Is this some sort of ref-counting solution?

Why does this occur before invalidating regions?

[Andrei Bucur]

(In reply to comment #33)
> (From update of attachment 151436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151436&action=review
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:65
> > +    NamedFlowSet::const_iterator end = m_namedFlows.end();
> > +    for (NamedFlowSet::const_iterator it = m_namedFlows.begin(); it != end; ++it) {
> > +        WebKitNamedFlow* namedFlow = *it;
> > +        if (namedFlow->getFlowState() == WebKitNamedFlow::FlowStateCreated) {
> > +            if (!index)
> > +                return namedFlow;
> > +            --index;
> > +        }
> > +    }
> 
> So this implementation has an odd quirk that if a flow is instantiated (NULL) and then later moved to (CREATED) it will push all the observable flows after it "down by one" in the index.  Basically, iterating this collection is no-where-near stable.  I guess that's generally true of live collections like this.  Seems like an odd spec/implementation choice.
When a flow is "created" it's in the CREATED state :). There's no point in creating one if it's not declared in CSS. I agree that iterating through this collection can bring up some unexpected results.
I haven't focused much on these as they are not exposed yet through the live collection. When their time comes and I'll write some layout tests for them I'll polish them as best I can.
> 
> Also seems like a behavior we could test.
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:77
> > +WebKitNamedFlow* WebKitNamedFlowCollection::flowByName(const String& flowName)
> > +{
> > +    NamedFlowSet::iterator it = m_namedFlows.find<String, NamedFlowHashTranslator>(flowName);
> > +    if (it == m_namedFlows.end() || (*it)->getFlowState() == WebKitNamedFlow::FlowStateNull)
> > +        return 0;
> > +
> > +    return *it;
> > +}
> 
> I assume it's spec'd that these flows are ordered and flows with duplicate names return the first?
It's not possible to have duplicate flows (it would indicate a serious bug in the implementation). This why this is a set and not a list.
> 
> > Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> > +    if (canBeDestroyed()) {
> > +        destroy();
> > +        return;
> > +    }
> 
> Is this some sort of ref-counting solution?
This makes sure renderers for NamedFlows that have no content and no regions are removed the render tree. This as well moves the NamedFlow object attached to it in the NULL state.

[Eric Seidel (no email)]

Comment on attachment 151436 [details]
Patch

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

So there will always be exactly one NamedFlow per name, likewise exactly one RenderNamedFlow per name.  And these will be associated via bi-directional weak pointers, correct?

> Source/WebCore/dom/WebKitNamedFlow.h:63
> +    FlowState getFlowState() const { return m_state; }

This should just be flowState(), no "get".

> Source/WebCore/dom/WebKitNamedFlow.h:64
> +    void switchFlowState() { m_state = m_state == FlowStateNull ? FlowStateCreated : FlowStateNull; }

Please make this setFlowState(), then callers will be more explicit (and hopefully more readable).  i don't think you ever need this "switch" behavior.  Callers always know what state they want the flow in.

[Eric Seidel (no email)]

Comment on attachment 151436 [details]
Patch

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

> Source/WebCore/dom/WebKitNamedFlow.cpp:159
> +    // A named flow that loses the renderer is in the "NULL" state.
> +    if (!m_parentFlowThread)
> +        m_flowManager->moveNamedFlowToNullState(this);

Why does the flow even have a m_state value then?  Can't the state be directly derived from whether m_renderer is set or not?

[Eric Seidel (no email)]

Comment on attachment 151436 [details]
Patch

I've read this another 3 times through. :)  I think I finally understand what you're going for here.  Please answer my above questions and upload a new copy.  I think this needs one more round.

Part of me would rather you did this in smaller pieces.  Like, for example, the item() and length() stuff doesn't need to be added until it's tested/used/exposed to the web.

None of these weak pointers are actually necessary, since the AtomicString representing the name of the flow is sufficient to look up any flow or flow renderer.  But the pointers may be easier to use in the end, unsure.

The test changes are not at all clear to me.  Are those just changes to the tests themselves?  Or is there a behavior change hidden in here?  Could we do the test chagnes separately?  Could we do whatever the behavior change is here in the smallest patch possible?

[Andrei Bucur]

(In reply to comment #34)
> (From update of attachment 151436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151436&action=review
> 
> > Source/WebCore/dom/Document.cpp:1129
> > +    // It's possible to have pending styles not applied that affect the existing flows.
> > +    updateStyleIfNeeded();
> 
> Why do flows care if style is up to date?  Is that when the from-into stuff is resolved?
NamedFlow objects (the ones that the spec exposes) can be created only from style sheets:
The style is modified (flow-from/flow-into), the style is computed and renderers (RenderNamedFlow) are created by the flow controller. When the renderer is created it requests a NamedFlow object in the CREATED state. If the NamedFlowCollection of the document already caches a NULL NamedFlow object for that flow, it will switch its state and pass it to the renderer. If not, a new NamedFlow is created.
When the style changes so that there's no flow-from/flow-into, after computing the style, the Renderer is destroyed and moves the NamedFlow object in the NULL state.

We need to call updateStyleIfNeeded to force a style recalculation. It's possible to have a script that modifies an element style to have flow-from/flow-into and the immediately call getFlowByName(). This call would fail because the style was not computed yet.
> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:50
> > +    // The named flow is not "strong" referenced from anywhere at this time so it shouldn't be reused if the named flow is recreated.
> 
> This comment doesn't immediately make sense to me.  It seems we're just telling the flowManager to clear any weak pointers it has to us.
Yes, that's true. It tells the flow manager to remove us from the cache because there's no renderer ref-ing us (thus the flow is in the NULL state) and there's no ref from JS either (e.g. event listeners) thus the flow object can't be used any more.
> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:70
> > +    if (m_flowManager->document())
> > +        m_flowManager->document()->updateLayoutIgnorePendingStylesheets();
> > +
> > +    // The renderer may be destroyed or created after the style update.
> > +    // Because this is called from JS, where the wrapper keeps a reference to the NamedFlow, no guard is necessary.
> 
> It's still not clear to me why we care about renders or styles being up to date here?
Imagine from JS calling getFlowByName(), holding the ref to that and then removing all the flow-from/flow-into values. When calling an API function, the flow internal state needs to be up to date.
> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:160
> > +void WebKitNamedFlow::setRenderer(RenderNamedFlowThread* parentFlowThread)
> > +{
> > +    // The named flow can either go from a no_renderer->renderer or renderer->no_renderer state; anything else could indicate a bug.
> > +    ASSERT((!m_parentFlowThread && parentFlowThread) || (m_parentFlowThread && !parentFlowThread));
> > +
> > +    m_parentFlowThread = parentFlowThread;
> > +
> > +    // A named flow that loses the renderer is in the "NULL" state.
> > +    if (!m_parentFlowThread)
> > +        m_flowManager->moveNamedFlowToNullState(this);
> > +}
> 
> This is how we manage the CREATED vs. NULL states?
> 
> The named flow states are :
> 
> NULL: the named flow is not referenced by any ‘flow-into’ or ‘flow-from’ computed value.
> CREATED: the named flow is referenced by at least one ‘flow-into’ or ‘flow-from’ computed value.
Yes, this is how the manager knows when a named flow doesn't have any flow-from/flow-into specified.
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:50
> > +unsigned WebKitNamedFlowCollection::length() const
> > +{
> > +    return m_createdFlowsCount;
> > +}
> 
> It's not immediately clear to me that this is the right implementation choice (of keeping everything in one set and having a cached value of the number of "CREATED" flows).  Alternate implementations would include:
> 1.  Walking the set each time to count the number of created flows
> 2.  Keeping the NULL flows separate and moving them into the CREATED set when they transition.
It seems clearer to have only one set. There's no special reason to have two indicators for the state of a flow (both the enum value and the set he belongs to). Having two sets just seems to add more complexity to the code. I think less data structures are a good thing on the long run.
The value is cached for performance reasons, so we don't need to count them every time the length is required.
> 
> Both would have observable consequences to JavaScript, so we'd need to better understnd what the intention of the spec is.
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:105
> > +    namedFlow->switchFlowState();
> 
> I think this is less clear than ->setFlowState(NullState)
Ok, I'll revise that.
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:113
> > +    // The document is not valid anymore so the collection will be destroyed anyway.
> > +    if (!m_document)
> > +        return;
> 
> Really?  Is this going to be OK?  Are we careful to check m_document in all our accessors? Or is someone going to clear our document and then try and call something like item(0) and end up crashing when we hand them back a destroyed flow?
I see where you are getting at. The NamedFlows are guarded because no document, means no renderers means the API will return default values.
The item and length accessors could use guarding when they get exposed.
> 
> Could someone grab a pointer to this flow collection and then have it outlive the document?  Seems possible from JavaScript once this is exposed.
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:130
> > +    static bool equal(WebKitNamedFlow* a, WebKitNamedFlow* b) { return a->name() == b->name(); }
> 
> So WebKitNamedFlow objects are entirely governed by thier names?  Why would we ever have mor than one object if they're uniqued on name?
This is mostly ListHashSet glue code. I'm not sure I understand the question. We need a quick lookup mechanism for NamedFlows using names and a HashSet seems like a good option.
> 
> >> Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> >> +    }
> > 
> > Is this some sort of ref-counting solution?
> 
> Why does this occur before invalidating regions?
Going into that if branch means the flow doesn't have any regions or content. Invalidation is not required at that step (and possibly can cause asserts if the RenderNamedFlow is marked as needing layout).

[Andrei Bucur]

(In reply to comment #38)
> (From update of attachment 151436 [details])
> I've read this another 3 times through. :)  I think I finally understand what you're going for here.  Please answer my above questions and upload a new copy.  I think this needs one more round.
> 
> Part of me would rather you did this in smaller pieces.  Like, for example, the item() and length() stuff doesn't need to be added until it's tested/used/exposed to the web.
I'll remove them.
> 
> None of these weak pointers are actually necessary, since the AtomicString representing the name of the flow is sufficient to look up any flow or flow renderer.  But the pointers may be easier to use in the end, unsure.
>
I don't understand what you're trying to suggest :(.
> The test changes are not at all clear to me.  Are those just changes to the tests themselves?  Or is there a behavior change hidden in here?  Could we do the test chagnes separately?  Could we do whatever the behavior change is here in the smallest patch possible?
The tests need to be modified to take into account that getFlowByName can return null. This was not possible before the patch.

[Andrei Bucur]

Created attachment 151916 [details]
Patch

[Andrei Bucur]

Comment on attachment 151916 [details]
Patch

Simplified the patch:
- removed index/length from the collection and the cached created count variable
- removed the flow state setter; the getter looks to the renderer now
- renamed getFlowState to flowState

[Eric Seidel (no email)]

Comment on attachment 151916 [details]
Patch

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

LGTM.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:68
> +    RefPtr<WebKitNamedFlow> newFlow = WebKitNamedFlow::create(this, flowName);
> +    m_namedFlows.add(newFlow.get());
> +
> +    return newFlow.release();

Some of the WTF add functions return what they've added.  It's possible you could make these 3 into one line if that's true.

> Source/WebCore/rendering/RenderNamedFlowThread.h:83
> +    bool canBeDestroyed() const { return m_regionList.isEmpty() && m_contentNodes.isEmpty(); }

Is this virtual?  Or specific to RenderNamedFlowThread?  What about when the whole RenderView tree goes down?  Will things correctly get cleaned up then?

[Andrei Bucur]

(In reply to comment #43)
> (From update of attachment 151916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151916&action=review
> 
> LGTM.
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:68
> > +    RefPtr<WebKitNamedFlow> newFlow = WebKitNamedFlow::create(this, flowName);
> > +    m_namedFlows.add(newFlow.get());
> > +
> > +    return newFlow.release();
> 
> Some of the WTF add functions return what they've added.  It's possible you could make these 3 into one line if that's true.
That's a good point. Unfortunately the "add" method returns a struct with an iterator and a bool result if the object was successfully added. I think chaining everything together would obfuscate the code and make it harder to read.

> 
> > Source/WebCore/rendering/RenderNamedFlowThread.h:83
> > +    bool canBeDestroyed() const { return m_regionList.isEmpty() && m_contentNodes.isEmpty(); }
> 
> Is this virtual?  Or specific to RenderNamedFlowThread?  What about when the whole RenderView tree goes down?  Will things correctly get cleaned up then?
This method is not virtual, it's specific to the RenderNamedFlowThread and self-describes what that condition means for the renderer.
I've thought this through and I can't see how this can have unpredictable results.
The render tree looks something like this:

RenderView
- Tree for normal flow
- RenderNamedFlowThread 1 - renderers for content nodes
- RenderNamedFlowThread 2 - renderers for content nodes
- etc

Before this patch, the RenderNamedFlowThread objects were destroyed when all the RenderTree was destroyed (e.g. when the Document is detached). Let's assume this is not buggy as no crash popped up until now :) (though I think there may be a situation that could cause a crash - not related to this change, I need to investigate a bit).
Now there's another case, when there are no content nodes or regions. The problems would've appear if it was possible to have a region child of it's own flow thread. However, this is not possible so I don't see any way how a crash can occur.

[WebKit Review Bot]

Comment on attachment 151916 [details]
Patch

Clearing flags on attachment: 151916

Committed r122556: <http://trac.webkit.org/changeset/122556>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.