[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 #12 from Chiculita Alexandru <achicu at adobe.com>  2012-06-15 15:03:54 PST ---
(From update of attachment 147790)
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*?

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