[webkit-reviews] review denied: [Bug 78404] [chromium] Replace RefPtr with OwnPtr for CCLayerImpl tree structure : [Attachment 128835] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Tien-Ren Chen
<trchen at chromium.org>'s request for review:
Bug 78404: [chromium] Replace RefPtr with OwnPtr for CCLayerImpl tree structure
https://bugs.webkit.org/show_bug.cgi?id=78404

Attachment 128835: Patch
https://bugs.webkit.org/attachment.cgi?id=128835&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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.

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

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

> 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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:347
> +

nit: don't need the newline here

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

> 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