[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 #27 from Eric Seidel <eric at webkit.org>  2012-07-02 09:36:49 PST ---
(From update of attachment 149024)
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.

> Source/WebCore/dom/Document.h:1495
> +    RefPtr<WebKitNamedFlowCollection> m_namedFlows;

Yup, the m_namedFlows(0) is not needed, since RefPtr has a default constructor.

> 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.

> 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.

> 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).

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:53
> +    return m_createdFlowsCount;

When is this different from m_namedFlows.length?

> 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.

> 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?

> Source/WebCore/dom/WebKitNamedFlowCollection.h:49
> +    WebKitNamedFlow* item(unsigned index) const;

I never know if unsigned or size_t is right.

> 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

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:193
> +    if (canBeDestroyed()) {
> +        destroy();
> +        return;
> +    }

NamedFlowThreads manage their own lifetime?  That's a bit odd to me.

> 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.

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