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

Wed Jul 11 16:45:09 PDT 2012

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


--- Comment #39 from Andrei Bucur <abucur at adobe.com>  2012-07-11 16:45:08 PST ---
(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).

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