Bug 70003 - [Chromium, WebKit API]: Move from FileChooserSettings deprecatedAcceptType to acceptMIMETypes
Summary: [Chromium, WebKit API]: Move from FileChooserSettings deprecatedAcceptType to...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 70095
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-12 23:39 PDT by Joseph Pecoraro
Modified: 2011-10-19 18:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.60 KB, patch)
2011-10-13 23:03 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (3.60 KB, patch)
2011-10-13 23:09 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (3.60 KB, patch)
2011-10-13 23:10 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (3.53 KB, patch)
2011-10-14 02:05 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2011-10-12 23:39:23 PDT
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
Comment 1 Joseph Pecoraro 2011-10-13 16:40:20 PDT
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.
Comment 2 Kent Tamura 2011-10-13 23:03:39 PDT
Created attachment 110964 [details]
Patch
Comment 3 WebKit Review Bot 2011-10-13 23:06:15 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Darin Fisher (:fishd, Google) 2011-10-13 23:06:40 PDT
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?
Comment 5 Kent Tamura 2011-10-13 23:09:29 PDT
Created attachment 110966 [details]
Patch 2

acceptTypeList -> acceptMIMETypes
Comment 6 Kent Tamura 2011-10-13 23:10:25 PDT
Created attachment 110967 [details]
Patch 3

acceptTypeList -> acceptMIMETypes in a comment
Comment 7 Kent Tamura 2011-10-13 23:11:25 PDT
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.
Comment 8 Joseph Pecoraro 2011-10-13 23:45:15 PDT
(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?
Comment 9 Darin Fisher (:fishd, Google) 2011-10-13 23:54:33 PDT
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?
Comment 10 Kent Tamura 2011-10-13 23:58:50 PDT
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.
Comment 11 Darin Fisher (:fishd, Google) 2011-10-14 00:11:47 PDT
> 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.
Comment 12 Kent Tamura 2011-10-14 02:05:03 PDT
Created attachment 110982 [details]
Patch 4

Update a comment.
Comment 13 Joseph Pecoraro 2011-10-14 10:58:40 PDT
(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.
Comment 14 Joseph Pecoraro 2011-10-14 11:04:31 PDT
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
Comment 15 WebKit Review Bot 2011-10-19 18:46:55 PDT
Comment on attachment 110982 [details]
Patch 4

Clearing flags on attachment: 110982

Committed r97910: <http://trac.webkit.org/changeset/97910>
Comment 16 WebKit Review Bot 2011-10-19 18:47:01 PDT
All reviewed patches have been landed.  Closing bug.