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

Fri Feb 24 18:17:18 PST 2012

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

James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128835|review?                     |review-
               Flag|                            |

--- Comment #18 from James Robinson <jamesr at chromium.org>  2012-02-24 18:17:18 PST ---
(From update of attachment 128835)
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

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