Bug 70003 – [Chromium, WebKit API]: Move from FileChooserSettings deprecatedAcceptType to acceptMIMETypes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Joseph Pecoraro]

In r97336 and r97338 WebCore::FileChooserSettings deprecated its "acceptType" string
in favor of a Vector<String> of MIME types "acceptMIMETypes".

If Chromium could transition to the new Vector that would be great. It would then share
the same parsing code as other ports. This would require coordination with the chromium
source which expects a string.

WebCore::FileChooserSettings is a struct of parameters for a <input type="file">
File Upload Dialog. Its passed to ports via ChomeClient::runOpenPanel. The deprecated
string value was the unparsed "accept" attribute from the <input>. The new Vector
value is the parsed MIME types from that "accept" attribute. This way all ports
share the same parsing code for the different MIME types listed in the "accept" attribute.

The changes:
<http://trac.webkit.org/changeset/97336>
<http://trac.webkit.org/changeset/97338>

Introduced in the following:
<http://webkit.org/b/69598> Pass Parsed Accept Attribute MIME Types to WebKit Clients

[Joseph Pecoraro]

EFL removed their usage in r97421 leaving Chromium as the only remaining user
of the deprecated string. So when switching you can remove the deprecated string.

[Kent Tamura]

Created attachment 110964 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110964 [details]
Patch

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

> Source/WebKit/chromium/public/WebFileChooserParams.h:64
> +    WebVector<WebString> acceptTypeList;

how about calling this acceptMIMETypes?

[Kent Tamura]

Created attachment 110966 [details]
Patch 2

acceptTypeList -> acceptMIMETypes

[Kent Tamura]

Created attachment 110967 [details]
Patch 3

acceptTypeList -> acceptMIMETypes in a comment

[Kent Tamura]

Comment on attachment 110964 [details]
Patch

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

>> Source/WebKit/chromium/public/WebFileChooserParams.h:64
>> +    WebVector<WebString> acceptTypeList;
> 
> how about calling this acceptMIMETypes?

Sure.

[Joseph Pecoraro]

(In reply to comment #1)
> EFL removed their usage in r97421 leaving Chromium as the only remaining user
> of the deprecated string. So when switching you can remove the deprecated string.

Patch looks good. Are you considering removing the now unused key,
or should I do that in a follow up?

[Darin Fisher (:fishd, Google)]

Comment on attachment 110967 [details]
Patch 3

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

> Source/WebKit/chromium/public/WebFileChooserParams.h:62
> +    // might contain incorrectly formatted strings. Leading and trailing whitespaces

oh, is this comment about containing incorrectly formatted strings still valid?
i had assumed, but not verified, that the WebCore parser would only pass valid
MIME types.  is that not the case?

[Kent Tamura]

Comment on attachment 110967 [details]
Patch 3

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

>> Source/WebKit/chromium/public/WebFileChooserParams.h:62
>> +    // might contain incorrectly formatted strings. Leading and trailing whitespaces
> 
> oh, is this comment about containing incorrectly formatted strings still valid?
> i had assumed, but not verified, that the WebCore parser would only pass valid
> MIME types.  is that not the case?

The comment is valid.
The parsing code simply split an accept attribute value and strip.  If an accept attribute is accept="!!!,,***,image/*", the list have ["!!!", "***", "image/*"].

http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1310
We might want to add more validation.

[Darin Fisher (:fishd, Google)]

> The comment is valid.
> The parsing code simply split an accept attribute value and strip.  If an accept attribute is accept="!!!,,***,image/*", the list have ["!!!", "***", "image/*"].

I see.


http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1310
> We might want to add more validation.

It does seem like it might be a good idea so that ports do not need to each do so themselves.  I don't know if we have existing MIME type parsing / validation code in WebKit.

[Kent Tamura]

Created attachment 110982 [details]
Patch 4

Update a comment.

[Joseph Pecoraro]

(In reply to comment #11)
> > The comment is valid.
> > The parsing code simply split an accept attribute value and strip.  If an accept attribute is accept="!!!,,***,image/*", the list have ["!!!", "***", "image/*"].
> 
> I see.
> 
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1310
> > We might want to add more validation.
> 
> It does seem like it might be a good idea so that ports do not need to each do so themselves.
> I don't know if we have existing MIME type parsing / validation code in WebKit.

The WebCore parsing I added was just the splitting and whitespace trimming.
Validating the MIME types would certainly be allowed:
http://www.whatwg.org/specs/web-apps/current-work/multipage/number-state.html#attr-input-accept

Yah, it doesn't look like there is any existing MIME type parsing / validation code.

[Joseph Pecoraro]

Ahh, I see you added it last night. Cool!
<http://webkit.org/b/70095> Reject invalid MIME type strings for a file selection dialog parameter

[WebKit Review Bot]

Comment on attachment 110982 [details]
Patch 4

Clearing flags on attachment: 110982

Committed r97910: <http://trac.webkit.org/changeset/97910>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.