Bug 144486 - Purge PassRefPtr from MaskImageOperation
Summary: Purge PassRefPtr from MaskImageOperation
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords:
Depends on:
Blocks: 144092
  Show dependency treegraph
 
Reported: 2015-04-30 19:07 PDT by Joonghun Park
Modified: 2016-02-22 02:24 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.96 KB, patch)
2015-04-30 21:39 PDT, Joonghun Park
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joonghun Park 2015-04-30 19:07:37 PDT
Purge PassRefPtr from MaskImageOperation.
Comment 1 Joonghun Park 2015-04-30 21:39:07 PDT
Created attachment 252133 [details]
Patch
Comment 2 Darin Adler 2015-05-01 15:27:34 PDT
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&&?
Comment 3 Gyuyoung Kim 2016-02-16 16:30:57 PST
(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 ?
Comment 4 Joonghun Park 2016-02-22 02:24:56 PST
(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/.