Bug 136035 - Use NSURLFileTypeMappings directly instead of depending on WebKitSystemInterface wrappers for it
Summary: Use NSURLFileTypeMappings directly instead of depending on WebKitSystemInterf...
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: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-17 23:25 PDT by Maciej Stachowiak
Modified: 2014-08-19 01:37 PDT (History)
7 users (show)

See Also:

Attachments
Patch (23.63 KB, patch)
2014-08-18 00:46 PDT, Maciej Stachowiak 		no flags Details | Formatted Diff | Diff
Attempt #2 (23.66 KB, patch)
2014-08-18 02:44 PDT, Maciej Stachowiak 		no flags Details | Formatted Diff | Diff
Attempt #3 (23.62 KB, patch)
2014-08-18 13:10 PDT, Maciej Stachowiak 		no flags Details | Formatted Diff | Diff
Patch (24.78 KB, patch)
2014-08-18 21:28 PDT, Maciej Stachowiak 		no flags Details | Formatted Diff | Diff
New version to avoid breaking non-clang platforms (24.79 KB, patch)
2014-08-18 22:35 PDT, Maciej Stachowiak 		mitz: review+
Details | Formatted Diff | Diff
Show Obsolete (3) 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 Maciej Stachowiak 2014-08-17 23:25:23 PDT 
Use NSURLFileTypeMappings directly instead of depending on WebKitSystemInterface wrappers for it
Comment 1 Maciej Stachowiak 2014-08-18 00:46:43 PDT 
Created attachment 236746 [details]
Patch
Comment 2 Maciej Stachowiak 2014-08-18 02:44:44 PDT 
Created attachment 236751 [details]
Attempt #2

Try again to see if this actually builds without internal headers
Comment 3 Maciej Stachowiak 2014-08-18 13:10:07 PDT 
Created attachment 236777 [details]
Attempt #3

Trying yet another version (can't easily test locally because precompiled headers).
Comment 4 Mark Rowe (bdash) 2014-08-18 15:41:27 PDT 
Comment on attachment 236777 [details]
Attempt #3

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

> Source/WebCore/platform/spi/cocoa/NSURLFileTypeMappings.h:27
> +#ifndef NSURLFileTypeMappings_h
> +#define NSURLFileTypeMappings_h

It seems a bit sketchy to define NS-prefixed macros.
Comment 5 Maciej Stachowiak 2014-08-18 21:28:18 PDT 
Created attachment 236797 [details]
Patch
Comment 6 Maciej Stachowiak 2014-08-18 22:35:10 PDT 
Created attachment 236801 [details]
New version to avoid breaking non-clang platforms
Comment 7 mitz 2014-08-18 22:44:43 PDT 
Comment on attachment 236797 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1046
> +#if PLATFORM(COCOA) &&  defined __has_include && __has_include(<CoreFoundation/CFPriv.h>)

Extra space after the &&. I see that sometimes we use parentheses after the 'defined' keyword and sometimes we don’t.

I wonder if instead of checking for the presence of a specific header we shouldn’t be using something that’s directly tied to how our configuration files define SDKROOT, but this seems fine for now.

> Source/WebCore/platform/ios/MIMETypeRegistryIOS.mm:36
>      return wkGetMIMETypeForExtension(ext);
> +    return [[NSURLFileTypeMappings sharedMappings] MIMETypeForExtension:(NSString *)ext];

You forgot to delete the first return statement!

> Source/WebCore/platform/mac/MIMETypeRegistryMac.mm:37
> +String MIMETypeRegistry::getMIMETypeForExtension(const String& ext)

Good opportunity to rename the parameter to “extension”.
Comment 8 mitz 2014-08-18 22:53:48 PDT 
Comment on attachment 236801 [details]
New version to avoid breaking non-clang platforms

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

r=me assuming you fix MIMETypeRegistryIOS.mm prior to landing.

> Source/WebCore/platform/ios/MIMETypeRegistryIOS.mm:35
>      return wkGetMIMETypeForExtension(ext);

Need to delete this!
Comment 9 Maciej Stachowiak 2014-08-19 00:43:17 PDT 
(In reply to comment #8)
> (From update of attachment 236801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236801&action=review
> 
> r=me assuming you fix MIMETypeRegistryIOS.mm prior to landing.

I fixed it and confirmed it builds on iOS now.

> 
> > Source/WebCore/platform/ios/MIMETypeRegistryIOS.mm:35
> >      return wkGetMIMETypeForExtension(ext);
> 
> Need to delete this!
Comment 10 Maciej Stachowiak 2014-08-19 01:37:28 PDT 
Committed r172749: <http://trac.webkit.org/changeset/172749>