[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 #31 from Andrei Bucur <abucur at adobe.com>  2012-07-09 16:46:13 PST ---
(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 :).

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