Bug 115314 – Simplify WebHTMLRepresentation supportedMIMETypes methods

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Simplify WebHTMLRepresentation supportedMIMETypes methods

[Benjamin Poulain]

Created attachment 199920 [details]
Patch

[Darin Adler]

Comment on attachment 199920 [details]
Patch

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

> Source/WebKit/mac/WebView/WebHTMLRepresentation.mm:87
> +static NSMutableArray *createArrayWithStrings(const HashSet<String>& set)

It would be nice to use naming that tells the static analyzer this object is intentionally leaked. Or we could name this leak like we do other similar functions.

[Anders Carlsson]

(In reply to comment #2)
> (From update of attachment 199920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199920&action=review
> 
> > Source/WebKit/mac/WebView/WebHTMLRepresentation.mm:87
> > +static NSMutableArray *createArrayWithStrings(const HashSet<String>& set)
> 
> It would be nice to use naming that tells the static analyzer this object is intentionally leaked. Or we could name this leak like we do other similar functions.

I prefer just returning a RetainPtr.

[Benjamin Poulain]

Created attachment 200078 [details]
Patch

[Benjamin Poulain]

Comment on attachment 200078 [details]
Patch

Clearing flags on attachment: 200078

Committed r149453: <http://trac.webkit.org/changeset/149453>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 200078 [details]
Patch

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

> Source/WebKit/mac/WebView/WebHTMLRepresentation.mm:88
> +static NSMutableArray *createArrayWithStrings(const HashSet<String>& set) NS_RETURNS_RETAINED;
> +static NSMutableArray *createArrayWithStrings(const HashSet<String>& set)

I missed this in my original review. Wouldn’t it be better to follow the preferred naming scheme so we don’t have to do NS_RETURNS_RETAINED explicitly? Could use the word “copy” or “new” instead of “create” perhaps?

> Source/WebKit/mac/WebView/WebHTMLRepresentation.mm:98
> +static NSMutableArray *createArrayByConcatenatingArrays(NSArray *first, NSArray *second) NS_RETURNS_RETAINED;
> +static NSMutableArray *createArrayByConcatenatingArrays(NSArray *first, NSArray *second)

Ditto.

> Source/WebKit/mac/WebView/WebHTMLRepresentation.mm:107
> +    static __unsafe_unretained NSArray *staticSupportedMIMETypes = createArrayByConcatenatingArrays([self supportedNonImageMIMETypes], [self supportedImageMIMETypes]);

Seems a little strange to start using __unsafe_unretained here. I suppose some day when we switch to ARC it will be handy.

[Benjamin Poulain]

(In reply to comment #7)
> (From update of attachment 200078 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200078&action=review
> 
> > Source/WebKit/mac/WebView/WebHTMLRepresentation.mm:88
> > +static NSMutableArray *createArrayWithStrings(const HashSet<String>& set) NS_RETURNS_RETAINED;
> > +static NSMutableArray *createArrayWithStrings(const HashSet<String>& set)
> 
> I missed this in my original review. Wouldn’t it be better to follow the preferred naming scheme so we don’t have to do NS_RETURNS_RETAINED explicitly? Could use the word “copy” or “new” instead of “create” perhaps?

You did not miss that. I worked on the static analyzer and the ARC compatibility thingies and I forgot that particular comment.

I'll update with the name fixed.