[Webkit-unassigned] [Bug 132024] [iOS] WebKit2 File Upload Support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=132024





--- Comment #4 from Darin Adler <darin at apple.com>  2014-04-24 17:30:24 PST ---
(From update of attachment 229913)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.