[Webkit-unassigned] [Bug 89000] [CSS Regions] Fix the lifecycle for the flow objects and their renderers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89000





--- Comment #13 from Andrei Bucur <abucur at adobe.com>  2012-06-15 16:42:49 PST ---
(From update of attachment 147790)
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 :).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.