[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 #34 from Eric Seidel <eric at webkit.org>  2012-07-11 16:12:59 PST ---
(From update of attachment 151436)
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?

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