[Webkit-unassigned] [Bug 78404] [chromium] Replace RefPtr with OwnPtr for CCLayerImpl tree structure

[bugzilla-daemon at webkit.org]

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





--- Comment #21 from Tien-Ren Chen <trchen at chromium.org>  2012-02-24 18:42:07 PST ---
(In reply to comment #18)
> (From update of attachment 128835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128835&action=review
> 
> I like this patch and think it's really close but we definitely do not want to add a bunch of leakPtr() calls to the test. leakPtr() pretty much always indicates a bug or a boundary with an API that isn't 100% compatible with the WTF smart pointer types.  For instance, today we use leakPtr() in one place to pass a value between threads and in another place to interface with the WebKit API which has a different set of smart pointers.  Otherwise we should never use it.  I commented on two specific instances with what I think would be a better approach, I believe that one or the other can be extended to the rest of the tests.

Ok I'll eliminate the leakPtr().

> > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:46
> > +    CCLayerImpl* ccLayerImpl = popCCLayerImpl.get();
> 
> the normal idiom for functions that take a PassOwnPtr<> parameter is to immediately put the parameter in a local OwnPtr<> and then never use the POP<> parameter again.  I do not think the code you've written here is incorrect, but I think the idiomatic way would be equally correct and less error-prone.
> 
> IOW, have ccLayerImpl be an OwnPtr<CCLayerImpl>, leave the null-check-and-return alone, and move the map.set() call to the end of the function and have it take ccLayerImpl.release()

Wow this is brilliant! I love this solution. done

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:46
> > +    , m_maskLayerID(-1)
> > +    , m_replicaLayerID(-1)
> 
> nit we use "xxxId" in other places (like m_layerId below). I don't have strong feelings on which is better but we should be consistent, and "xxxId" seems to be winning.
done

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIteratorPosition.h:1
> > +/*
> 
> i think you have a bad merge here - this file was deleted upstream in http://trac.webkit.org/changeset/108678
done

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:347
> > +
> 
> nit: don't need the newline here
done

> > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:323
> > +    CCLayerImpl* child2 = CCLayerImpl::create(3).leakPtr();
> 
> This is unnecessarily confusing - leakPtr() pretty much always means that there's a leak and requires special care, in this case the code looks buggy unless you look down to the adoptPtr() call 9 lines below.  I think what you really want here is to have child2 be an OwnPtr<> and .release() it in the addChild() call.

Then again we have the problem that child2 will be null after the release() call...
I'll limit the scope of child2 in cases like this, then declare another raw pointer child2 later on if used. Sounds better?

> 
> > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:359
> > +    CCLayerImpl* child2 = CCLayerImpl::create(3).leakPtr();
> 
> again the leakPtr() is unnecessarily scary. In this instance, it's probably simpler to add child2 to root's children here and then grab a raw pointer to it via .get() since you need to dereference it later on in the test

ditto

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