Bug 171977 – Stop using PassRefPtr in platform/graphics

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Dumez]

Stop using PassRefPtr in platform/graphics.

[Chris Dumez]

Created attachment 309753 [details]
WIP Patch

[Chris Dumez]

Created attachment 309755 [details]
WIP Patch

[Chris Dumez]

Created attachment 309757 [details]
WIP Patch

[Chris Dumez]

Created attachment 309760 [details]
WIP Patch

[Chris Dumez]

Created attachment 309762 [details]
Patch

[Chris Dumez]

Created attachment 309765 [details]
Patch

[Chris Dumez]

Created attachment 309771 [details]
Patch

[Chris Dumez]

Created attachment 309777 [details]
Patch

[Chris Dumez]

Created attachment 309791 [details]
Patch

[Alex Christensen]

Comment on attachment 309777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309777&action=review

> Source/WebCore/platform/graphics/filters/FilterOperation.h:182
>          ASSERT_NOT_REACHED();
> -        return nullptr;
> +        return *static_cast<FilterOperation*>(nullptr);

This looks gross.  It should return a RefPtr or RELEASE_ASSERT_NOT_REACHED().  We shouldn't have to check a valid Ref for a nullptr.

> Source/WebCore/rendering/RenderLayer.cpp:3717
>      if (renderer().document().deviceScaleFactor() >= 2) {
> -        static NeverDestroyed<Image*> resizeCornerImageHiRes(Image::loadPlatformResource("textAreaResizeCorner@2x").leakRef());
> +        static NeverDestroyed<Image*> resizeCornerImageHiRes(&Image::loadPlatformResource("textAreaResizeCorner@2x").leakRef());

Don't we have 3x resources now?  Maybe we should update this.

[Chris Dumez]

(In reply to Alex Christensen from comment #10)
> Comment on attachment 309777 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309777&action=review
> 
> > Source/WebCore/platform/graphics/filters/FilterOperation.h:182
> >          ASSERT_NOT_REACHED();
> > -        return nullptr;
> > +        return *static_cast<FilterOperation*>(nullptr);
> 
> This looks gross.  It should return a RefPtr or
> RELEASE_ASSERT_NOT_REACHED().  We shouldn't have to check a valid Ref for a
> nullptr.

Sure, I do not mind making it a release assertion. It is going to crash on the next line anyway.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:3717
> >      if (renderer().document().deviceScaleFactor() >= 2) {
> > -        static NeverDestroyed<Image*> resizeCornerImageHiRes(Image::loadPlatformResource("textAreaResizeCorner@2x").leakRef());
> > +        static NeverDestroyed<Image*> resizeCornerImageHiRes(&Image::loadPlatformResource("textAreaResizeCorner@2x").leakRef());
> 
> Don't we have 3x resources now?  Maybe we should update this.

Seems out of scope.

[Chris Dumez]

Created attachment 309793 [details]
Patch

[Alex Christensen]

> > Don't we have 3x resources now?  Maybe we should update this.
> 
> Seems out of scope.

Definitely out of scope.

[Alex Christensen]

Comment on attachment 309793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309793&action=review

> Source/WebCore/platform/graphics/filters/FilterOperation.h:182
> +        return *static_cast<FilterOperation*>(nullptr);

I would prefer dereferencing this.  Dereferencing nullptr feels really bad.

[Chris Dumez]

(In reply to Alex Christensen from comment #14)
> Comment on attachment 309793 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309793&action=review
> 
> > Source/WebCore/platform/graphics/filters/FilterOperation.h:182
> > +        return *static_cast<FilterOperation*>(nullptr);
> 
> I would prefer dereferencing this.  Dereferencing nullptr feels really bad.

If I return |this|, then I would need to const_cast it. Note that dereferencing nullptr is the pattern used here already:
Ref<Node> ShadowRoot::cloneNodeInternal(Document&, CloningOperation)
{
    RELEASE_ASSERT_NOT_REACHED();
    return *static_cast<Node*>(nullptr); // ShadowRoots should never be cloned.
}

[Ryosuke Niwa]

Comment on attachment 309793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309793&action=review

>>> Source/WebCore/platform/graphics/filters/FilterOperation.h:182
>>> +        return *static_cast<FilterOperation*>(nullptr);
>> 
>> I would prefer dereferencing this.  Dereferencing nullptr feels really bad.
> 
> If I return |this|, then I would need to const_cast it. Note that dereferencing nullptr is the pattern used here already:
> Ref<Node> ShadowRoot::cloneNodeInternal(Document&, CloningOperation)
> {
>     RELEASE_ASSERT_NOT_REACHED();
>     return *static_cast<Node*>(nullptr); // ShadowRoots should never be cloned.
> }

Yeah, I think returning casted nullptr is fine.
clang would be eliminating this code anyway due to reachability.

[Chris Dumez]

Comment on attachment 309793 [details]
Patch

Clearing flags on attachment: 309793

Committed r216702: <http://trac.webkit.org/changeset/216702>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

Committed r216722: <http://trac.webkit.org/changeset/216722>