Bug 115314 - Simplify WebHTMLRepresentation supportedMIMETypes methods
Summary: Simplify WebHTMLRepresentation supportedMIMETypes methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-27 16:01 PDT by Benjamin Poulain
Modified: 2013-05-01 14:17 PDT (History)
1 user (show)

See Also:

Attachments
Patch (4.28 KB, patch)
2013-04-27 16:05 PDT, Benjamin Poulain 		no flags Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2013-04-29 21:01 PDT, Benjamin Poulain 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2013-04-27 16:01:06 PDT 
Simplify WebHTMLRepresentation supportedMIMETypes methods
Comment 1 Benjamin Poulain 2013-04-27 16:05:08 PDT 
Created attachment 199920 [details]
Patch
Comment 2 Darin Adler 2013-04-27 19:33:45 PDT 
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.
Comment 3 Anders Carlsson 2013-04-29 13:15:32 PDT 
(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.
Comment 4 Benjamin Poulain 2013-04-29 21:01:00 PDT 
Created attachment 200078 [details]
Patch
Comment 5 Benjamin Poulain 2013-05-01 14:01:12 PDT 
Comment on attachment 200078 [details]
Patch

Clearing flags on attachment: 200078

Committed r149453: <http://trac.webkit.org/changeset/149453>
Comment 6 Benjamin Poulain 2013-05-01 14:01:15 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2013-05-01 14:10:49 PDT 
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.
Comment 8 Benjamin Poulain 2013-05-01 14:17:42 PDT 
(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.