[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


James Robinson <jamesr at chromium.org> changed:

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




--- Comment #11 from James Robinson <jamesr at chromium.org>  2012-02-21 22:21:10 PST ---
(From update of attachment 127271)
View in context: https://bugs.webkit.org/attachment.cgi?id=127271&action=review

I think this is looking quite good.  I would like to see the mask/replica issue resolved and I have a style quibble with the way many of the tests have been transformed, but otherwise this looks great and I'd like to make this change ASAP.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:-327
> -    if (m_maskLayer == maskLayer)
> -        return;
> -

does this mean that we'll always call noteLayerPropertyChangedForSubtree() even if we're setting the mask layer to the same thing it used to be? that'll pretty much completely disable scissoring and any other damage tracking based optimizations for pages that have masks or replicas :/

is there any way to preserve this?  maybe use IDs instead of pointers if we need to?

> Source/WebKit/chromium/tests/CCLayerImplTest.cpp:69
> +    OwnPtr<CCLayerImpl> opChild = CCLayerImpl::create(2);

it's fairly unusual to have an "op" prefix.  we use the "pop" prefix for PassOwnPtr<> when we put the result immediately into a local and never use the prefixed variable again. another issue with this construction is that the .release() call nulls out opChild, but it's still sitting in the scope tempting someone to write code that incorrectly tries to use it

here's another way to write this that would be more consistent with other WebKit code and a bit less error-prone:

OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1);
root->addChild(CCLayerImpl::create(2));
child->addChild(CCLayerImpl::create(3));
CCLayerImpl* child = root->children()[0];
CCLayerImpl* grandChild = child->children()[0];

etc.

same goes for rest of the tests

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