Bug 144486 – Purge PassRefPtr from MaskImageOperation

• Component: WebKit Misc.
• Status: RESOLVED INVALID

[Joonghun Park]

Purge PassRefPtr from MaskImageOperation.

[Joonghun Park]

Created attachment 252133 [details]
Patch

[Darin Adler]

Comment on attachment 252133 [details]
Patch

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

> Source/WebCore/platform/graphics/MaskImageOperation.h:51
> +    static Ref<MaskImageOperation> create(RefPtr<WebKitCSSResourceValue>&& cssValue, const String& url, const String& fragment, bool isExternalDocument, RefPtr<CachedResourceLoader>&&);

Are both cssValue and CachedResourceLoader arguments that can be null? Moreover, are they objects where we are handing off ownership. If we are sharing ownership then we could just use types of raw pointers or raw references. Only need RefPtr or Ref if we are transferring ownership.

> Source/WebCore/platform/graphics/MaskImageOperation.h:52
> +    static Ref<MaskImageOperation> create(RefPtr<StyleImage>&& generatedImage);

Same question here.

> Source/WebCore/platform/graphics/MaskImageOperation.h:55
> +    RefPtr<CSSValue> cssValue();

Can this ever return null? Does this need to hand off ownership of the return value (in other words, is it ever a newly computed value rather than just returning something already stored in some data member)?

> Source/WebCore/platform/graphics/MaskImageOperation.h:81
> +    MaskImageOperation(RefPtr<WebKitCSSResourceValue>&& cssValue, const String& url, const String& fragment, bool isExternalDocument, RefPtr<CachedResourceLoader>&&);
> +    MaskImageOperation(RefPtr<StyleImage>&& generatedImage);

Same questions. RefPtr&& is the correct type for something where we are handing off ownership and that can be null. Are these all in that category? Or should some be pointers, references, or Ref&&?

[Gyuyoung Kim]

(In reply to comment #2)
> Comment on attachment 252133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252133&action=review
> 
> > Source/WebCore/platform/graphics/MaskImageOperation.h:51
> > +    static Ref<MaskImageOperation> create(RefPtr<WebKitCSSResourceValue>&& cssValue, const String& url, const String& fragment, bool isExternalDocument, RefPtr<CachedResourceLoader>&&);
> 
> Are both cssValue and CachedResourceLoader arguments that can be null?
> Moreover, are they objects where we are handing off ownership. If we are
> sharing ownership then we could just use types of raw pointers or raw
> references. Only need RefPtr or Ref if we are transferring ownership.
> 
> > Source/WebCore/platform/graphics/MaskImageOperation.h:52
> > +    static Ref<MaskImageOperation> create(RefPtr<StyleImage>&& generatedImage);
> 
> Same question here.
> 
> > Source/WebCore/platform/graphics/MaskImageOperation.h:55
> > +    RefPtr<CSSValue> cssValue();
> 
> Can this ever return null? Does this need to hand off ownership of the
> return value (in other words, is it ever a newly computed value rather than
> just returning something already stored in some data member)?
> 
> > Source/WebCore/platform/graphics/MaskImageOperation.h:81
> > +    MaskImageOperation(RefPtr<WebKitCSSResourceValue>&& cssValue, const String& url, const String& fragment, bool isExternalDocument, RefPtr<CachedResourceLoader>&&);
> > +    MaskImageOperation(RefPtr<StyleImage>&& generatedImage);
> 
> Same questions. RefPtr&& is the correct type for something where we are
> handing off ownership and that can be null. Are these all in that category?
> Or should some be pointers, references, or Ref&&?

Joonghun, any update ?

[Joonghun Park]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 252133 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=252133&action=review
> > 
> > > Source/WebCore/platform/graphics/MaskImageOperation.h:51
> > > +    static Ref<MaskImageOperation> create(RefPtr<WebKitCSSResourceValue>&& cssValue, const String& url, const String& fragment, bool isExternalDocument, RefPtr<CachedResourceLoader>&&);
> > 
> > Are both cssValue and CachedResourceLoader arguments that can be null?
> > Moreover, are they objects where we are handing off ownership. If we are
> > sharing ownership then we could just use types of raw pointers or raw
> > references. Only need RefPtr or Ref if we are transferring ownership.
> > 
> > > Source/WebCore/platform/graphics/MaskImageOperation.h:52
> > > +    static Ref<MaskImageOperation> create(RefPtr<StyleImage>&& generatedImage);
> > 
> > Same question here.
> > 
> > > Source/WebCore/platform/graphics/MaskImageOperation.h:55
> > > +    RefPtr<CSSValue> cssValue();
> > 
> > Can this ever return null? Does this need to hand off ownership of the
> > return value (in other words, is it ever a newly computed value rather than
> > just returning something already stored in some data member)?
> > 
> > > Source/WebCore/platform/graphics/MaskImageOperation.h:81
> > > +    MaskImageOperation(RefPtr<WebKitCSSResourceValue>&& cssValue, const String& url, const String& fragment, bool isExternalDocument, RefPtr<CachedResourceLoader>&&);
> > > +    MaskImageOperation(RefPtr<StyleImage>&& generatedImage);
> > 
> > Same questions. RefPtr&& is the correct type for something where we are
> > handing off ownership and that can be null. Are these all in that category?
> > Or should some be pointers, references, or Ref&&?
> 
> Joonghun, any update ?

MaskImageOeration related function had been removed by https://bugs.webkit.org/show_bug.cgi?id=146653.
It means that this patch doesn't valid anymore, so I resolve this bug as invalid.
By the way, I found this blog post is very helpful, https://webkit.org/blog/5381/refptr-basics/.