[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 #12 from Tien-Ren Chen <trchen at chromium.org>  2012-02-22 09:37:13 PST ---
(In reply to comment #11)
> (From update of attachment 127271 [details])
> 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?

By using PassOwnPtr/OwnPtr we automatically prevent this from happening. Comparing two own pointers should always return false because an object can't be owned by two at the same time. (wtf emits compile error if you try to compare)

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

Got it

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