Bug 132024 – [iOS] WebKit2 File Upload Support

• Component: WebKit2
• Status: RESOLVED FIXED

[Joseph Pecoraro]

iOS File Upload support (<input type="file">)

[Joseph Pecoraro]

<rdar://problem/16420629>

[Joseph Pecoraro]

Created attachment 229913 [details]
[PATCH] Proposed Fix

This is a faithful implementation matching iOS WK1 very closely while at the same time moving off of deprecated / incomplete APIs and reducing unnecessary complexity.

I expect plenty of comments, so don't hold back.

[WebKit Commit Bot]

Attachment 229913 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:490:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:510:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 229913 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/English.lproj/Localizable.strings:5
> +"# Photos and # Videos (file upload on page label for image and videos" = "%@ and %@";

Missing end parenthesis in comment.

> Source/WebCore/English.lproj/Localizable.strings:8
> +"# Videos (file upload on page label for multiple videos" = "%@ Videos";

Missing end parenthesis in comment.

> Source/WebCore/English.lproj/Localizable.strings:71
> +"1 Video (file upload on page label for one video" = "1 Video";

Missing end parenthesis in comment.

> Source/WebKit2/Shared/WebOpenPanelParameters.h:46
> -    bool allowMultipleFiles() const { return m_settings.allowsMultipleFiles; } 
> +    bool allowMultipleFiles() const { return m_settings.allowsMultipleFiles; }

Do we really need this whitespace change in an otherwise untouched file in the patch?

> Source/WebKit2/UIProcess/PageClient.h:256
> +    virtual void handleRunOpenPanel(WebPageProxy*, WebFrameProxy*, WebOpenPanelParameters*, WebOpenPanelResultListenerProxy*) = 0;

Do these all have to be pointers? Can’t they be references?

> Source/WebKit2/UIProcess/WebOpenPanelResultListenerProxy.cpp:48
> +static Vector<String> filePathsFromFileURLs(API::Array* fileURLs)

Can this take a reference instead of a pointer?

> Source/WebKit2/UIProcess/WebOpenPanelResultListenerProxy.cpp:59
>              URL url(URL(), apiURL->string());
>              filePaths.uncheckedAppend(url.fileSystemPath());

Not sure this needs a local variable. Just write it like this:

    filePaths.uncheckedAppend(URL(URL(), apiURL->string()).fileSystemPath());

> Source/WebKit2/UIProcess/WebOpenPanelResultListenerProxy.cpp:83
> +    Vector<String> filePaths = filePathsFromFileURLs(fileURLsArray);
>      m_page->didChooseFilesForOpenPanel(filePaths);

Not sure we need a local variable here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3022
> +#if !PLATFORM(IOS)
>      if (!m_uiClient->runOpenPanel(this, frame, parameters.get(), m_openPanelResultListener.get()))
>          didCancelForOpenPanel();
> +#else
> +    m_pageClient.handleRunOpenPanel(this, frame, parameters.get(), m_openPanelResultListener.get());
> +#endif

Do we really need this dichotomy?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.h:26
> +#if PLATFORM(IOS)

Do we really need this in an iOS-specific header?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:216
> +    RetainPtr<CGImageRef> imageRef = [generator copyCGImageAtTime:kCMTimeZero actualTime:nil error:&error];

I think this leaks, because there is no adoptNS.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:585
> +        RetainPtr<_WKFileUploadItem> item = adoptNS([[_WKVideoFileUploadItem alloc] initWithFilePath:filePath mediaURL:mediaURL]);

I don’t think we need a local variable here. I suggest just doing the adoptNS.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:602
> +            static NSString *kTemporaryDirectoryName = @"WKWebFileUpload";
> +            static NSString *kUploadImageName = @"image.jpg";

These should be const or static const rather than static.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:638
> +            RetainPtr<_WKFileUploadItem> item = adoptNS([[_WKImageFileUploadItem alloc] initWithFilePath:filePath originalImage:originalImage]);

I don’t think we need a local variable here. I suggest just doing the adoptNS.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:696
> +        title = [NSString stringWithFormat:WEB_UI_STRING_KEY("%@ and %@", "# Photos and # Videos (file upload on page label for image and videos", "File Upload images and videos label"), imageString, videoString];

Combining the two localization strings here with “and” does not seem good for localization. We should always build a whole string. Maybe it always worked this way, but it’s normally not acceptable to do that. We should have four different combinations.

> Source/WebKit2/WebProcess/WebPage/WebOpenPanelResultListener.h:48
> +    void didChooseFilesWithDisplayStringAndIcon(const Vector<String>&, const String& displayString, WebCore::Icon*);

Can this take a reference instead of a pointer?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3005
> +#if USE(CG)

Why this #if? When would PLATFORM(IOS) be true, but USE(CG) false?

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 229913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229913&action=review

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:3022
> > +#if !PLATFORM(IOS)
> >      if (!m_uiClient->runOpenPanel(this, frame, parameters.get(), m_openPanelResultListener.get()))
> >          didCancelForOpenPanel();
> > +#else
> > +    m_pageClient.handleRunOpenPanel(this, frame, parameters.get(), m_openPanelResultListener.get());
> > +#endif
> 
> Do we really need this dichotomy?

On iOS WebKit I don't think it makes sense to let a client handle the file open dialog. iOS wants to do its own handling in WebKit2. So I think it does make sense to ignore the client and do default handling.

An alternative could be always trying both the uiclient, falling back to the page client, and then falling back to canceling. I will look into that.


> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.h:26
> > +#if PLATFORM(IOS)
> 
> Do we really need this in an iOS-specific header?

This is the dominating pattern right now.

All Mac and iOS files are included in the same Xcode project and included in both builds. Inside each file there is an #if PLATFORM if it really is for one platform or the other. Despite this being in an "ios" directory it is included in the Mac build.

Maybe we should find a cleaner way to specify which files are included in which builds.


> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:216
> > +    RetainPtr<CGImageRef> imageRef = [generator copyCGImageAtTime:kCMTimeZero actualTime:nil error:&error];
> 
> I think this leaks, because there is no adoptNS.

Good catch.


> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:696
> > +        title = [NSString stringWithFormat:WEB_UI_STRING_KEY("%@ and %@", "# Photos and # Videos (file upload on page label for image and videos", "File Upload images and videos label"), imageString, videoString];
> 
> Combining the two localization strings here with “and” does not seem good for localization. We should always build a whole string. Maybe it always worked this way, but it’s normally not acceptable to do that. We should have four different combinations.

So you are suggesting a new string for the complete combination?

    "%@ Photos and %@ Videos"


> > Source/WebKit2/WebProcess/WebPage/WebOpenPanelResultListener.h:48
> > +    void didChooseFilesWithDisplayStringAndIcon(const Vector<String>&, const String& displayString, WebCore::Icon*);
> 
> Can this take a reference instead of a pointer?

It would be valid to pass a null icon here, that would just tell WebCore there is no display icon. We should always under normal circumstances have an icon, but it could be possible (e.g. failing to produce a thumbnail image somehow in the UIProcess) and null is handled gracefully in WebCore (by just not showing an icon).

[Joseph Pecoraro]

Created attachment 230403 [details]
[PATCH] Addressed Comments

This addresses the majority of Darin's comments.

[Enrica Casucci]

Comment on attachment 230403 [details]
[PATCH] Addressed Comments

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

Just one initial comment.

> Source/WebKit2/ChangeLog:9
> +        Include MobileCoreServices on iOS for kUTTypeImage/kUTTypeMovie.

Couldn't you soft link MobileCoreServices? We already do that in WebCore and I think it would be more performant.

> Source/WebKit2/Configurations/WebKit2.xcconfig:34
>  FRAMEWORK_AND_LIBRARY_LDFLAGS_iphoneos = $(FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator) -framework IOSurface;

See my comment in the ChangeLog.

[Joseph Pecoraro]

(In reply to comment #7)
> (From update of attachment 230403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230403&action=review
> 
> Just one initial comment.
> 
> > Source/WebKit2/ChangeLog:9
> > +        Include MobileCoreServices on iOS for kUTTypeImage/kUTTypeMovie.
> 
> Couldn't you soft link MobileCoreServices? We already do that in WebCore and I think it would be more performant.
> 
> > Source/WebKit2/Configurations/WebKit2.xcconfig:34
> >  FRAMEWORK_AND_LIBRARY_LDFLAGS_iphoneos = $(FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator) -framework IOSurface;
> 
> See my comment in the ChangeLog.

I figure there is no advantage to soft linking it if UIKit is hard linking it and this hard links UIKit, but maybe my understanding of this is flawed.

[Enrica Casucci]

Comment on attachment 230403 [details]
[PATCH] Addressed Comments

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

The patch looks good. One remaining question: don't you need to handle rotation? I know we do it for select elements and for the airplay picker.

>>> Source/WebKit2/ChangeLog:9
>>> +        Include MobileCoreServices on iOS for kUTTypeImage/kUTTypeMovie.
>> 
>> Couldn't you soft link MobileCoreServices? We already do that in WebCore and I think it would be more performant.
> 
> I figure there is no advantage to soft linking it if UIKit is hard linking it and this hard links UIKit, but maybe my understanding of this is flawed.

You are right, I forgot we link UIKit.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:653
> +    NSString *imageCountString, *videoCountString;

Split over two lines.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:654
> +    NSString *imageString, *videoString;

Ditto

[Joseph Pecoraro]

Comment on attachment 230403 [details]
[PATCH] Addressed Comments

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:695
> +        title = [NSString stringWithFormat:WEB_UI_STRING_KEY("%@ Photos and %@ Videos", "# Photos and # Videos (file upload on page label for image and videos)", "File Upload images and videos label"), imageCountString, videoCountString];
> +        break;

Arg this won't work for 1 photo and 1 video this would say "1 Photos and 1 Videos". I think I misinterpreted Darin's comment. It sounds like, if I want to make explicit strings to handle the "and" case I would need a bunch just for that:

    "1 Photo and 1 Video"
    "1 Photo and # Videos"
    "# Photos and 1 Video"
    "# Photos and # Videos"

A bit lengthy, but I can do that. I'll do it in a follow-up. And then I can clean up this code since we wouldn't really need the earlier switches.

[Joseph Pecoraro]

<https://trac.webkit.org/r168180>