Modified the pepper file chooser API to support filtering files by extensions.

Modified the pepper file chooser API to support filtering files by extensions.

Previously you could filter only by MIME type. This adds support for filtering by specific extensions as well, e.g. .txt,.html. This change is aligned with the web platform which now allows filtering by file extension for <input> elements (http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#attr-input-accept).

This also changes the linux implementation of the file dialog (SelectFileDialogImplGTK). In the past, it would turn file extensions to filter into MIME types. However this is a bit silly because in FileSelectHelper we do the reverse (turn MIME types into a list of file extensions to filter by). It also prevents us from filtering by a specific extensions when this is really what is desired.

BUG=129251
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139434

[raymes, 2012-05-25 17:12:24]

brettw for ppapi related stuff
estade for file chooser related stuff

[raymes, 2012-05-25 17:14:00]

On 2012/05/25 17:12:24, raymes wrote:
> brettw for ppapi related stuff
> estade for file chooser related stuff

to be more specific:
estade for chrome/*
brettw for the rest.

[Evan Stade, 2012-05-25 20:19:57]

https://chromiumcodereview.appspot.com/10414085/diff/17001/chrome/browser/fil...
File chrome/browser/file_select_helper.cc (right):

https://chromiumcodereview.appspot.com/10414085/diff/17001/chrome/browser/fil...
chrome/browser/file_select_helper.cc:259: DCHECK(ascii_type[0] == '.' ?
ascii_type.length() > 1 : true)
DCHECK(ascii_type != ".")

https://chromiumcodereview.appspot.com/10414085/diff/17001/chrome/browser/fil...
chrome/browser/file_select_helper.cc:263: DCHECK(TrimWhitespaceASCII(ascii_type,
TRIM_ALL, &ascii_type)
unit tests > DCHECKs?

[raymes, 2012-05-25 21:00:23]

http://codereview.chromium.org/10414085/diff/17001/chrome/browser/file_select...
File chrome/browser/file_select_helper.cc (right):

http://codereview.chromium.org/10414085/diff/17001/chrome/browser/file_select...
chrome/browser/file_select_helper.cc:259: DCHECK(ascii_type[0] == '.' ?
ascii_type.length() > 1 : true)
On 2012/05/25 20:19:57, Evan Stade wrote:
> DCHECK(ascii_type != ".")

Umm yes :P. Done.

http://codereview.chromium.org/10414085/diff/17001/chrome/browser/file_select...
chrome/browser/file_select_helper.cc:263: DCHECK(TrimWhitespaceASCII(ascii_type,
TRIM_ALL, &ascii_type)
On 2012/05/25 20:19:57, Evan Stade wrote:
> unit tests > DCHECKs?

I agree, I was just extending the current behavior which is to do the
precondition test. Since this is relying on data which is being passed from a
different layer (i.e. either Webkit or Pepper) it seems ok to have the
precondition test here or to just re-check the extension is valid and skip it if
not.

In any case, I'm happy to add a pepper unit test for this if you want but less
happy to add a webkit test =P.

[Evan Stade, 2012-05-25 21:10:15]

http://codereview.chromium.org/10414085/diff/17001/chrome/browser/file_select...
File chrome/browser/file_select_helper.cc (right):

http://codereview.chromium.org/10414085/diff/17001/chrome/browser/file_select...
chrome/browser/file_select_helper.cc:263: DCHECK(TrimWhitespaceASCII(ascii_type,
TRIM_ALL, &ascii_type)
On 2012/05/25 21:00:23, raymes wrote:
> On 2012/05/25 20:19:57, Evan Stade wrote:
> > unit tests > DCHECKs?
> 
> I agree, I was just extending the current behavior which is to do the
> precondition test. Since this is relying on data which is being passed from a
> different layer (i.e. either Webkit or Pepper) it seems ok to have the
> precondition test here or to just re-check the extension is valid and skip it
if
> not.
> 
> In any case, I'm happy to add a pepper unit test for this if you want but less
> happy to add a webkit test =P.

the function should be robust to failures of these preconditions, and you should
add chrome tests to make sure the expected thing happens (like "VIDEO/*" is
ignored). You can leave the DCHECKs but I'm not sure how useful they are (as you
point out they should be tested at the webkit or pepper level).

[raymes, 2012-05-25 23:40:36]

On 2012/05/25 21:10:15, Evan Stade wrote:
>
http://codereview.chromium.org/10414085/diff/17001/chrome/browser/file_select...
> File chrome/browser/file_select_helper.cc (right):
> 
>
http://codereview.chromium.org/10414085/diff/17001/chrome/browser/file_select...
> chrome/browser/file_select_helper.cc:263:
DCHECK(TrimWhitespaceASCII(ascii_type,
> TRIM_ALL, &ascii_type)
> On 2012/05/25 21:00:23, raymes wrote:
> > On 2012/05/25 20:19:57, Evan Stade wrote:
> > > unit tests > DCHECKs?
> > 
> > I agree, I was just extending the current behavior which is to do the
> > precondition test. Since this is relying on data which is being passed from
a
> > different layer (i.e. either Webkit or Pepper) it seems ok to have the
> > precondition test here or to just re-check the extension is valid and skip
it
> if
> > not.
> > 
> > In any case, I'm happy to add a pepper unit test for this if you want but
less
> > happy to add a webkit test =P.
> 
> the function should be robust to failures of these preconditions, and you
should
> add chrome tests to make sure the expected thing happens (like "VIDEO/*" is
> ignored). You can leave the DCHECKs but I'm not sure how useful they are (as
you
> point out they should be tested at the webkit or pepper level).

Done.

[Evan Stade, 2012-05-25 23:52:05]

thanks. chrome/ lgtm

[raymes, 2012-05-29 16:14:27]

ping brettw

On Fri, May 25, 2012 at 4:52 PM,  <estade@chromium.org> wrote:
> thanks. chrome/ lgtm
>
> http://codereview.chromium.org/10414085/

[brettw, 2012-05-29 17:57:27]

Sorry I took so long. LGTM

http://codereview.chromium.org/10414085/diff/13003/chrome/browser/file_select...
File chrome/browser/file_select_helper_unittest.cc (right):

http://codereview.chromium.org/10414085/diff/13003/chrome/browser/file_select...
chrome/browser/file_select_helper_unittest.cc:10: TEST_F(FileSelectHelperTest,
IsAcceptTypeValid) {
Just use TEST (no _F) and remove the typedef above.

[raymes, 2012-05-29 20:06:47]

On 2012/05/29 17:57:27, brettw wrote:
> Sorry I took so long. LGTM
> 
>
http://codereview.chromium.org/10414085/diff/13003/chrome/browser/file_select...
> File chrome/browser/file_select_helper_unittest.cc (right):
> 
>
http://codereview.chromium.org/10414085/diff/13003/chrome/browser/file_select...
> chrome/browser/file_select_helper_unittest.cc:10: TEST_F(FileSelectHelperTest,
> IsAcceptTypeValid) {
> Just use TEST (no _F) and remove the typedef above.

Done.

[commit-bot: I haz the power, 2012-05-29 20:10:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10414085/12003

[commit-bot: I haz the power, 2012-05-30 00:05:43]

Change committed as 139434