[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 #28 from Andrei Bucur <abucur at adobe.com>  2012-07-02 10:10:30 PST ---
(In reply to comment #27)
> (From update of attachment 149024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149024&action=review
> 
> > Source/WebCore/dom/Document.cpp:499
> > +    , m_namedFlows(0)
> 
> If m_namedFlows is a smart pointer, this is not needed.
Will fix.

> 
> > Source/WebCore/dom/Document.h:1495
> > +    RefPtr<WebKitNamedFlowCollection> m_namedFlows;
> 
> Yup, the m_namedFlows(0) is not needed, since RefPtr has a default constructor.
> 
Idem.

> > Source/WebCore/dom/WebKitNamedFlow.cpp:81
> > +    if (m_parentFlowThread) {
> > +        if (contentNode->renderer()
> 
> I'm always suspicious of long if blocks, and wonder if helper functions shouldn't be used instead (since they allow for easy early-return.
I'll wrap it up nicely.

> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:110
> > +            Node* node = const_cast<Node*>(*it);
> 
> Sad times.  Maybe we should be marking namedFlows() as const, and m_namedFlows as mutable?  Or maybe we should just use a non-const iterator here? const Nodes don't really make much sense anyway, since they're ref counted.  So the non-const iterator (if available) Seems to make the most sense.
Ok, I'll search for the best option to remove that cast.

> 
> > Source/WebCore/dom/WebKitNamedFlow.h:43
> > +class WebKitNamedFlowCollection;
> 
> This is not needed if you have the include too.
> 
> > Source/WebCore/dom/WebKitNamedFlow.h:47
> > +    static PassRefPtr<WebKitNamedFlow> create(PassRefPtr<WebKitNamedFlowCollection> manager, const AtomicString& flowThreadName)
> 
> This doesn't really need to be in the header, but is the reason why you need to include WebKitNamedFlowColection.h (to instantiate the PassRefPtr).
Makes sense, I'll move it outside the header.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:53
> > +    return m_createdFlowsCount;
> 
> When is this different from m_namedFlows.length?
The m_namedFlows set contains both the CREATE and NULL flows. The API is used to retrieve only the CREATED flows. The NULL flows are only cached until they are destroyed (e.g. when there are no more references from JS, there are no JS listeners for "regionLayoutUpdate" on the flows etc.). We need to keep track of the NULL flows because it's possible for a script to reactivate them and it shouldn't be required for an author to re-add the listeners.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:43
> > +class WebKitNamedFlowCollection : public RefCounted<WebKitNamedFlowCollection> {
> 
> Why is this named WebKitNamedFlowCollection?  Normally we would just use "NamedFlowCollection", no?  Even if this is currently exposed to JS with WebKit in its name, we can easily do that in the IDL instead of renaming the c++ clas.
I've named it similar to how WebKitNamedFlow is called. If I rename this one, should the same be done for WebKitNamedFlow. I'm not familiar with the reasons why WebKitNamedFlow was prefixed like this.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:46
> > +    ~WebKitNamedFlowCollection();
> 
> Are you declaring the destructor to avoid including headers in this header? or did you have other plans for an explicit destructor?
Just a leftover from a previous version. I'll remove it.

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:49
> > +    WebKitNamedFlow* item(unsigned index) const;
> 
> I never know if unsigned or size_t is right.
I've inspired from the NodeList class :). Also, the JS wrappers are generated with unsigned (probably because this is how the IDL defines the param).

> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.h:51
> > +    WebKitNamedFlow* getFlowByName(const String&);
> 
> We tend to be anti-"get" in function names.  It may make sense here, since this is a lookup.  But lookupFlowByName() or flowByName() or flowFromName() or flowWithName() might all be more webkitty. http://www.webkit.org/coding/coding-style.html#names-setter-getter
I'll rename it accordingly :). It was a left-over from the old version that was an IDL method.

> 
> > Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> > +    if (canBeDestroyed()) {
> > +        destroy();
> > +        return;
> > +    }
> 
> NamedFlowThreads manage their own lifetime?  That's a bit odd to me.
The flow thread renderers lifetimes depend on the style changes, on how content nodes/regions are added to the flow. This was a compromise between an easy to understand solution and over-engineering to delegate it to some other component (most adequate is probably the FlowThreadController). With this patch, when a region or content node is removed, we just check if the flow still exists in the Document. If not, the renderer is deleted. Maybe I can add comments in code that the renderer might be destroyed after the region chain is shrinked/content nodes are removed.

> 
> > Source/WebCore/rendering/RenderNamedFlowThread.h:50
> > +    AtomicString flowThreadName() const;
> 
> This could prossibly const AtomicString&, depending on if this is perf senstiive or not. (I doubt it is).  That would avoid one ref churn.
I'll update this as well.

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