[webkit-reviews] review denied: [Bug 78404] [chromium] Replace RefPtr with OwnPtr for CCLayerImpl tree structure : [Attachment 127271] 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 127271: Patch
https://bugs.webkit.org/attachment.cgi?id=127271&action=review

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