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

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org>  2014-04-24 19:19:44 PST ---
(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).

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