Closed Bug 565274 Opened 14 years ago Closed 12 years ago

Implement the accept attribute for the form and file upload controls for custom MIME types

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: mounir, Assigned: arnaud.bienner)

References

()

Details

(Keywords: dev-doc-needed, html5)

Attachments

(2 files, 8 obsolete files)

This is a follow-up from bug 377624 which is only implementing the image/*
part.

Ryan Jones wrote a patch implementing it:
https://bugzilla.mozilla.org/attachment.cgi?id=265675
Blocks: 566348
Blocks: 83749
This would not be hard to implement if we have a filter only for MIME types. That means if accept="audio/*,audio/vorbis" it would create a filter for audio and another one for audio/vorbis which may be a bit confusing for the user.
Actually, this is already the case with accept="audio/*,video/*" which creates two filters.

So, what do we want to do if we have MIME types mixed with foo/* filters? Showing one filter for MIME types and one for each foo/* filter in @accept? (the easiest solution) or showing only one filter mixing everything? (the api isn't ready for that AFAICT).
By the way, what should we do if we have too foo/* filters? It would loose all semantic aspect and would be quite useless actually.
Do we know of any websites or other authors that have asked for this? I really don't think we should spend time on this at this point. The risks of us filtering out the wrong files are very big and the reward here seems very small.
(In reply to comment #2)
> Do we know of any websites or other authors that have asked for this? I really
> don't think we should spend time on this at this point. The risks of us
> filtering out the wrong files are very big and the reward here seems very
> small.

I think that could be quite useful for websites requesting PDF, doc or stuff like that. For example, if you have to send a work to your teacher via a platform like Moodle.
Indeed, this is not reliable but probably not much more than audio/*, video/* and image/* if there is no specific UI.

Jonas, what would you think if we refuse mixing of audio/*, video/*, image/* and MIME types?
I see two bad sides with this proposition:
1. authors would not be able to mix audio/* with video/* or image/* (or any combination) but if we have a specific UI it would be a bid idea to do that and even if we don't there is no obvious use case.
2. authors would not be able to set audio/* and add some MIME types a UA do not manage.

IMO, 1 isn't a problem but 2 could be in a very low percentage of cases if UA do not all have a strong support of audio/*, video/* or image/* or if a website wants to be receive a really unusual format (but in that case why using a wildcard type?).

If we implement this solution it would be super easy, users should not be annoyed and most authors should be happy.
I still prefer if we don't spend time on implementing, reviewing and maintaining this. There are other things we can spend time on that will help authors a lot more.
No longer blocks: 566348
@Mounir, how about having the following filters in the choose file dialog:
1) all accepted files (default)
2 - n) each MIME types/type group separately
n+1) All files (*/*)

This would be pretty similar to what's being done in the File->Open dialog of most (Windows) applications, notably not Firefox (what?!).

@Jonas: I think you're underestimating usefulness of this feature. It's really handy for a huge lot of cases, like uploading avatars (any forum), media files (Youtube), documents (Moodle) etc.

The almost only cases where this would not be useful are webmail/forum attachment forms and file sharing websites.
(In reply to comment #5)
> @Mounir, how about having the following filters in the choose file dialog:
> 1) all accepted files (default)
> 2 - n) each MIME types/type group separately
> n+1) All files (*/*)
> 
> This would be pretty similar to what's being done in the File->Open dialog of
> most (Windows) applications, notably not Firefox (what?!).

I'm not sure our API is ready for this case actually. I should double-check that later.

> @Jonas: I think you're underestimating usefulness of this feature. It's really
> handy for a huge lot of cases, like uploading avatars (any forum), media files
> (Youtube), documents (Moodle) etc.

Hmmm, you can ask for "image/*" for avatars and "video/*" for Youtube media files. Only document do not match these use cases. I agree this is ignoring the fact that you might want specific image/video types.

Anyway, this can't be done for Firefox 4 and this _might_ be done for the following version.
(In reply to comment #6)
> (In reply to comment #5)
> > @Jonas: I think you're underestimating usefulness of this feature. It's really
> > handy for a huge lot of cases, like uploading avatars (any forum), media files
> > (Youtube), documents (Moodle) etc.
> 
> Hmmm, you can ask for "image/*" for avatars and "video/*" for Youtube media
> files. Only document do not match these use cases. I agree this is ignoring the
> fact that you might want specific image/video types.

Exactly. I personally don't think anyone would actually ever want what image/* means. In my particular case (which brought me to this but), we want jpg, png, or gif. Photoshop file is also an image, but it's certainly not something we would want or expect.

> Anyway, this can't be done for Firefox 4 and this _might_ be done for the
> following version.

It would be a really nice addition!
Would love to see this added, would be really useful to web devs such as myself, plus, I'm sure we'd all love to see it now rather than in ~3 years ;D

I guess it's not the most important thing...
Hi,

To complete previous comments, here is the specification of this attribute, from W3C: http://www.w3schools.com/tags/att_input_accept.asp
And I also believe it will be useful to have this added in Firefox, for all MIME types (as it is currently just partially implemented, for few MIME types).

I might have a look to the code to see if this can be added easily but I'm not very familiar with FF's code.
Please note that w3schools is not affiliated with W3C
@Jonas, thanks for the info. I didn't know that.
As Mounir told me the other day on IRC, there is a discussion on W3C about the opportunity to use file extensions in addition/instead of mime types: https://www.w3.org/Bugs/Public/show_bug.cgi?id=11482
IMHO, it would be nice to support mime types, as the web developer wouldn't have to know all the file extensions associated to a particular type of file.
Anyway, I wrote a small patch that address this problem.
It uses MIME service to discover file extensions associated with a mime type (which uses some predefined list + OS's list) so it should be accurate and consistent with other applications.

Let me know if it sounds of interest for you, and if so, if you need some things to be corrected in order to be (hopefully :) approved/integrated.
I don't think we want to use this approach, for several reasons.

First of all, as evident in the patch, the only way to do this on today's OSs is to map to a list of extensions. However that mapping will vary wildly from user to user. So something that works on the developer's machine is very likely to not work on some users machine.

I.e. if a website want to accept only word documents, listing a set of mime types might work on the developers machine. However the end user might not even have word installed (received the word document through email) and so won't have the mime type mappings leading to not being able to select the word document in the file picker.

Second, mime types are a pretty terrible way of filtering files. Do you even know the mime type of one of the most common document formats in use: Word? Turns out there's not one mimetype for word but rather 6. 6 very long ones. Not to mention that most sites that accept word documents probably also want to accept Excel documents and a few non MS-Office document formats. The value in the accept attribute for something like google docs would pretty quickly get out of hands.


Sure, we could add multiple filters to the dialog, but unless we default to */* it will mean that many users won't be able to select the file they want. And if we default to */*, how many users are really going to select a more restrictive filter in order to more easily choose a file?


I guess I really don't see a huge value here. If we enabled people to upload new types of files that would be one thing, but that's not what we're doing. You can already upload any type of file. This bug is only about being able to bring up a file picker which filters out certain types of files. Applications like youtube, Moogle, google docs and slideshare are already possible.


I'm fine with hard-coding more mime types and fixing this bug that way. But using the OS mime type mappings just doesn't feel like it'll be consistent enough that it won't get in users way.
Based on that I think we should WONTFIX this bug since as it's currently expressed it tries to deal with "custom" mime types which obviously can't be coded into our implementation.

In short, I'm totally ok with adding support for more mimetypes like we've done for image/* etc. For example adding support for application/pdf seems useful. But using the OS mime service is in my opinion WONTFIX.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
I guess I would be ok with defaulting to *.* as a filter, but having a secondary filter which is based on the OS mime service. I just don't know if that'll be used enough by websites and users to make it worth maintaining such a code path.
Hi,

Thanks for your feedbacks.
Few remarks:
First at all, as far as I remember, the idea of using a mime type resolver which also looks at the OS mapping was suggested by Mounir (when we were discussing together the other day, if I understood right what he meant) and I agree with this idea.
The advantage of this approach is that you will have less code to maintain in order to have a mapping up to date: the OS will do that for you.
Also, the mime service already has a hard-coded list that overrides the OS' list. And if the mime type isn't found in both lists, there is a 3rd list the service look at.

Second: from my point of view, the aim of this implementation is a helper for users: when available, the filter will be set, to avoid user from uploading wrong files. But I agree the "All files" filter should ALWAYS be available, but not as the default one. If an advanced user know that .abc file is actually a jpg picture, it can select "All files" filter and will able to select its file. And also, for non-advanced user, I think it's nice to have this feature: for instance, if (for some reason) our server requires only png picture, having this filter will avoid the user to take a jpg file we cannot process: this will avoid the user (and server) to waste time by uploading a wrong file, having the server returning an error, etc.
Most of desktop applications offer files filtering within file selection dialog, including Firefox when selecting "Opening a file". I see no reason for not implementing this behavior for Web applications.

About the "mime types are a pretty terrible way of filtering files". Letting the web developer having complete knowledge of file extensions mapping is also terrible IMO.
Anyway, I partially agree with this, and what I suggest is to also accept file extension filtering (but this can be implemented in the second time).

One last thing, Opera and Chrome already implement this. I know that "everybody do this, so let's do this too" is a awful argument, but that just to let you know ;)

I understand you do not want to waste time implementing this, but as I already wrote a patch, and that I'm OK to improve it if needed, I think it shouldn't take you so much time ;)
So there's a few assumptions that I've made:

1. We care more about non-advanced users than advanced ones since there are more non-advanced users.
2. Non-advanced users are unlikely to see that they can change the filter. Hence whatever is filtered out by the default filter is effectively not available to non-advanced users at all.
3. Most mimetype<->extension mappings are not pre-installed on OSs, but rather are only added to the OS mapping when a application is installed to handle the particular mime type. For example the mapping between "application/x-rtg" and ".mp" doesn't exist in the OS until the user installs Maya.
4. Users will want to upload files even if they don't have the software installed to create those files. For example users will want to upload Maya files even from computers where they don't have Maya installed.

The result of this is that if a website uses accept="application/x-rtg" and we use the patch currently attached to this patch, many users which would want to upload maya files won't be able to do so.


I know that native applications open filepickers what use filters. But there's a whole set of problems that they don't have to care about. First of all they are always installed on the system which means that they can ensure that whatever mimetypes that they need are in the OS mimetype mappings. Second, they quite likely don't use mimetypes at all but rather extensions.


I very much understand that you put the effort into writing this patch, and I'm very sorry if that ends up being wasted time. That's why I wanted to be clearer in our communications by marking this bug WONTFIX. I realize that it's essentially "free" for us to land the patch, but we'll also have to maintain it, possibly for all future, which is far from free. Removing features can be a very big pain, even if the feature works poorly, since there is always someone that really likes it. Hence I'm reluctant to put in features that I'm not convinced works well.


But given that chrome supports filtering, i'm ok with having the default filter be *.* but have a second filter which is constructed in the way your patch does. That way we'll get some feedback on if this feature is something that websites are interested in using.
Here's a suggestion which, I think, would solve Jonas' problem. First of all, I think it's safe to assume that in most cases, the filter specified will be really common. Such as "Images in JPG, GIF or PNG format". These are really basic MIME types that exist on every system. Plus, I'm sure these are types that are in our list anyway.


So, how about the following scenario:
* if all MIME types listed in the filter have mappings in our list or in this system, then use the filtered list by default
* otherwise default to *.*. Although I'm wondering if it makes sense to show filtered list in this case at all, when you don't exactly know what to filter.

Additionally, entries for each separate MIME type could be available in the drop-down, to make advanced users happier. Of course, I am only talking about types that are known AND accepted by the filter.
By the way, I suspect that this problem of unknown filters is more likely to manifest on Windows. Not sure about OS X, bug Linux and friends have this global mime.types file, which is quite extensive. Again, considering the fact that in most cases the filter will be really basic, I think this scenario would be an improvement in most cases, and would not degrade experience in those that are left.
I would be ok with that.

Hardcode types that we think are going to be common. If there are unknown types default to *.*.

This is basically what we do now, except that our list of known types are very short, and we don't build a non-default filter list when we default to *.*.
If I understood correctly what Arnaud said, the mime type service already has some hard coded association between mime types and extensions. So, we can assume that common mime types are hard coded (if not, we can add the missing ones) and that the hard coded list is used if nothing is found in the system.
If no extensions are found for a particular mime type we have to default to no filter.

Would that works for you Jonas?

Also, not to be done in that bug, but I think we should make nsIFilePicker accepting mime types filter.
The concern isn't common mime types. Those are easily solved either by hardcoding the extension mappings, or by hardcoding which ones we can look up in OS mime service and know we get reliable results across all OSs where we use a file picker.

The concern is uncommon mime types which are going work spottily.

Like I said, I'm ok with doing the mimetype service lookup and creating a non-default filter based on that. That seems like a good first step and it gives us a chance to get feedback on how well the mapping works.

If everyone is ok with that approach I can reopen the bug?
(In reply to Jonas Sicking (:sicking) from comment #21)
> Like I said, I'm ok with doing the mimetype service lookup and creating a
> non-default filter based on that. That seems like a good first step and it
> gives us a chance to get feedback on how well the mapping works.
For a first step, sounds good to me.
In a second step, it might be nice to have mime type filter as the default if the mime type is part of the common/hard-coded set?

Also, a remark which is a little bit out of scope: currently, only one filter is allowed (we cannot mix them together).
I used this assumption when writing my patch and check only for one mime type, and only if none pre-defined filter (audio/*|video/*|image/*) was already present.
But I saw that initially it wasn't done like this. IMHO it would be better to allow several filters, comma separated, like suggested here: http://dev.w3.org/html5/spec/states-of-the-type-attribute.html#attr-input-accept
But this might be discussed in a second time.
(In reply to Arnaud from comment #22)
> Also, a remark which is a little bit out of scope: currently, only one
> filter is allowed (we cannot mix them together).
> I used this assumption when writing my patch and check only for one mime
> type, and only if none pre-defined filter (audio/*|video/*|image/*) was
> already present.
> But I saw that initially it wasn't done like this. IMHO it would be better
> to allow several filters, comma separated, like suggested here:
> http://dev.w3.org/html5/spec/states-of-the-type-attribute.html#attr-input-
> accept
> But this might be discussed in a second time.

We could allow multiple mime types filter but I don't think we want to allow accept="video/* audio/*" for the moment. There "might" be valid use cases for that but image/* video/* and audio/* should be specialized tokens that even requires special UI if possible. Putting two of them would make things harder. The same way, one of those tokens associated with a mime type filter should fallback to no filter. However, multiple mime type filters should okay IMO.

REOPENING assuming we agreed on what should be done.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Yup, sounds like we agree on the first step which is great.

I would imagine that we can find a second step too, but let's worry about that in a separate bug.
Taking into account last comments, I wrote this patch.
Basically, I move all the logic about the filters in a "SetFilePickerFiltersFromAccept" method, which replaces "GetFilterFromAccept".
IMHO it's better that way because a "GetFilters" function might return an int (as previously) or/and a list of filters corresponding to mimetypes, for use with the filePicker.
Now, we don't have to care about this filePicker's technical details in run() function as it is handled in "SetFilePickerFiltersFromAccept".

If a correct combination of filters is found, they are set, otherwise not.
By "correct", I mean:
- a image/* or video/* or audio/* filter
- or one or more mime types filters.

The filter name is set to mime type name.
Maybe this can be changed (mime type name + extensions list around bracket, "Customized files" + extensions list like in Chrome, ...)

I tested it manually with a sample file I wrote for this occasion. But I would be curious to know how to run automatic tests for this case (I wasn't able to find any information about this in Mozilla's documentation). I guess it is possible as for bug 377624 (image/audio/video filter) tests have been added.
Attachment #600817 - Attachment is obsolete: true
Attachment #606417 - Flags: review?(jonas)
Comment on attachment 606417 [details] [diff] [review]
Add support for mime type filters

I think Jonas should definitely have a look. I will try to review the patch too in case of Jonas takes too long.
Attachment #606417 - Flags: review?(mounir)
For what it is worth - supporting the "accept" feature for arbitrary mime-types or file extensions would be an extremely useful addition to FF. We are supporting a web based submit tool for various HPC applications. Those have pretty fixed/established conventions about file extensions and we already had numerous requests to preselect on those.

At the moment the inability of most browsers to "do it right" (or at all) forces me to integrated completely overkill Java or Flash code. Or wait for IE10 ...

Cheers
Martin
Comment on attachment 606417 [details] [diff] [review]
Add support for mime type filters

Review of attachment 606417 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4164,5 @@
> +      //... if no image/audio/video filter if found, check mime types filters
> +      nsCOMPtr<nsIMIMEInfo> mimeInfo;
> +      if (mimeService->GetFromTypeAndExtension(NS_ConvertUTF16toUTF8(token),
> +                                               EmptyCString(),
> +                                               getter_AddRefs(mimeInfo)) == NS_OK) {

Don't check X == NS_OK. Check NS_SUCCEEDED(X)

@@ +4202,5 @@
> +          extensionListStr += "; ";
> +        }
> +        if (!extensionListStr.IsEmpty()) {
> +          filePicker->AppendFilter(NS_ConvertUTF8toUTF16(mimeTypeName),
> +                                   NS_ConvertUTF8toUTF16(extensionListStr));

Hmm... This is somewhat inconsistent. With your code, for something like:

"text/plain,application/msword"

We would add three filters: *.*
                            .txt
                            .doc

However for something like

"text/plain,image/*"

We would add just one filter: *.*


We should make this more consistent.

How these rules:

1. We always add a *.* filter
2. If there are multiple mimetypes listed, we add a filter which includes all extensions for all mimetypes ("all supported types")
3. We add one filter for each mime which includes all extensions for that mimetype.

If the only mimetype is one of the "whitelisted" ones (image/* etc) we use 2 as the default option, otherwise we use 1 as default option.

So for something like "text/plain,application/msword" we would use the following filters:
*.* (default)
*.txt; *.doc
*.txt
*.doc

For "text/plain,image/*" we would use
*.* (default)
*.txt; .jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp; *.ico; *.svg;...
*.txt
.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp; *.ico; *.svg;...

For "image/*" we would use
*.*
.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp; *.ico; *.svg;... (default)

For "text/plain" we would use
*.* (default)
.txt


(yes, this means that we'll have to hard-code the list of images here too :( )
Attachment #606417 - Flags: review?(jonas) → review-
Actually, you can copy some of the code from [1] to avoid having to hardcode the list of image extensions

[1] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseFilePicker.cpp#92
Attachment #606417 - Flags: review?(mounir)
(In reply to Jonas Sicking (:sicking) from comment #28)
> Don't check X == NS_OK. Check NS_SUCCEEDED(X)
I didn't know, thanks for pointing this out.

> Hmm... This is somewhat inconsistent. With your code, for something like:
> 
I did it this way because of comment 23.
But indeed, it sounds good to do it like you suggest :) i.e. allowing mixing filters without automatic fallback to only *.* if "whitelisted" filters are mixed with mime types ones. But I if understood well, Mounir suggests something different in comment 23.
Also, if I understand you well, multiple "whitelisted" filters should be allowed (even if it that case the default filter will be *.*), right?

Also, the idea of providing a "All supported types" filter is great :)

If we all agree on what should be done, I will try to rework my patch.
Indeed, I did suggest something different because I'm not sure what behavior we should have when we are mixing special values (image/*, audio/* or video/*) together and with regular mime types. For mixing them together, we chose to do nothing. That means accept="image/* audio/*" is equivalent to accept="". It would have been consistent to keep that behavior for mixing those special values with mime types. Jonas, what do you think?
I guess there could be some concern that doing what I suggest in comment 28 could "encourage" people to do things like accept="image/*,video/*". But it doesn't seem like a huge concern to me since we could on for example android be showing the intents for image plus the intents for videos.

So I wouldn't bee too worried about it. I think it's more important that we do the best thing we can and see if people will use this feature at all.
OK, so what behavior do you think should  be implemented finally?
IMHO, Jonas' solution (explained in comment 28) is great, even if it allows mixing filters together.
I understand that it may doesn't make sense in some cases, but I think this is responsibility of the web developer to choose "good" filters? And indeed, there can be some cases when mixing filters is legitimate.
(In reply to Jonas Sicking (:sicking) from comment #32)
> I guess there could be some concern that doing what I suggest in comment 28
> could "encourage" people to do things like accept="image/*,video/*". But it
> doesn't seem like a huge concern to me since we could on for example android
> be showing the intents for image plus the intents for videos.

Ok. But currently we have checks that make this impossible and the Android code isn't ready for that. Though, we could open a follow-up.

Arnaud, I guess you can go with Jonas' proposal.
FYI, there was a decision in the HTML WG to allow file extensions in the accept attribute:
http://lists.w3.org/Archives/Public/public-html/2012Mar/0788.html
Given that that was pushed through essentially by one guy (the only reason it went through was that no-one bothered speaking up against it), I'm not convinced that it'll stay in the spec for very long.

So I'd say let's continue on the plan in comment 28 for now. Adding support for extensions can be trivially done in a separate step.
Finally, I found some time to rework on this.

I've implemented the behavior discussed previously.

Like in my previous patch, I've created a SetFilePickerFiltersFromAccept in order to replace GetFilterFromAccept.
But since revision fead9a65565f, GetFilterFromAccept is also used in nsFileControlFrame, so I was forced to keep it, even if it is not used anymore in any part of nsHTMLInputElement.cpp.

I hope my code is clear enough. If not, don't hesitate to ask me.
To summarize what it does:
- add a "All Files" filter; init services needed (to retrieve mime types, etc.).
- construct a list of filter (by parsing tokens and using mime type service if needed) and a "All Supported Types" filter.
- actually add each filter to the filepicker:
"All Supported Types" is added iff there are several filters.
If there is only one valid filter which is video/* or audio/* or image/*, it is selected as default

Let me know what you think about this, and if you think some things should be changes/improved.
Attachment #606417 - Attachment is obsolete: true
Comment on attachment 616349 [details] [diff] [review]
Add support for mime type filters (2nd draft)

Review of attachment 616349 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4172,5 @@
> +  // Retrieve all filters
> +  while (tokenizer.hasMoreTokens()) {
> +    const nsDependentSubstring token = tokenizer.nextToken();
> +
> +    nsString mimeTypeName;

Well... Maybe I should have called this nsString "filterName", as image/audio/video filter as not real mime types. This should have be clearer.

@@ +4187,5 @@
> +    } else if (token.EqualsLiteral("video/*")) {
> +      titleBundle->GetStringFromName(NS_LITERAL_STRING("videoTitle").get(), getter_Copies(mimeTypeName));
> +      filterBundle->GetStringFromName(NS_LITERAL_STRING("videoFilter").get(), getter_Copies(extensionListStr));
> +    } else {
> +      //... if no image/audio/video filter if found, check mime types filters

s/if found/is found (typo)
Attachment #616349 - Flags: review?(jonas)
Comment on attachment 616349 [details] [diff] [review]
Add support for mime type filters (2nd draft)

Review of attachment 616349 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below changes. Looks great!

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4170,5 @@
> +  HTMLSplitOnSpacesTokenizer tokenizer(accept, ',');
> +
> +  // Retrieve all filters
> +  while (tokenizer.hasMoreTokens()) {
> +    const nsDependentSubstring token = tokenizer.nextToken();

This should be nsDependentSubstring& to avoid doing an unnecessary copy.

@@ +4174,5 @@
> +    const nsDependentSubstring token = tokenizer.nextToken();
> +
> +    nsString mimeTypeName;
> +    nsString extensionListStr;
> +    bool isMimeTypeFilter = false;

Rename this to isWellKnown or some such. (And reverse when it's true of course). All of what we're dealing with here are some form of mimetype. And we might want to add "application/pdf" as a well-known type for example.

Rename in the struct too.

@@ +4198,5 @@
> +          isMimeTypeFilter = true;
> +          // Get mime type name
> +          nsCString cMimeTypeName;
> +          mimeInfo->GetType(cMimeTypeName);
> +          mimeTypeName = NS_ConvertUTF8toUTF16(cMimeTypeName);

CopyUTF8toUTF16(cMimeTypeName, mimeTypeName);

@@ +4234,5 @@
> +  }
> +
> +  // Add each filter
> +  for (unsigned int i = 0; i < filters.Length(); ++i) {
> +    nsFilePickerFilter filter = filters[i];

use |nsFilePickerFilter& filter| to avoid the extra copy

@@ +4235,5 @@
> +
> +  // Add each filter
> +  for (unsigned int i = 0; i < filters.Length(); ++i) {
> +    nsFilePickerFilter filter = filters[i];
> +    filePicker->AppendFilter(filter.mTitle, filter.mFilter);

Or you can just do |filters[i].mTitle, filters[i].mFilter|.

::: content/html/content/src/nsHTMLInputElement.h
@@ +622,5 @@
>    bool                     mCanShowValidUI      : 1;
>    bool                     mCanShowInvalidUI    : 1;
> +
> +private:
> +  struct nsFilePickerFilter {

Move this struct to the cpp file. I think that if you declare it 'static' it won't be exported as a symbol.
Thanks for reviewing this patch.
I'm glad that it looks great to you :)

Taking into account your remarks, I modified it.

Just one thing I didn't changed: moving the nsFilePickerFilter struct from the .h file to the .cpp file
I didn't get exactly what you meant...
I thought that private members' symbols aren't exported (depending on compiler's options though).
Moving struct declaration to .cpp file, then recompiling, then "nm ./obj-i686-pc-linux-gnu/dist/bin/* | grep nsFilePickerFilter" give similar results (i.e. nsFilePickerFilter is present in both cases).
And defining "static struct ..." in cpp file doesn't work. (I'm not sure it was what you meant).
I firstly added it in the .h because I think it's nicer IMHO to have all definitions in .h and only the logic in .cpp file.
If this is really not OK for you, please let me know what you're excepting more precisely.
Attachment #616349 - Attachment is obsolete: true
Attachment #616349 - Flags: review?(jonas)
Attachment #620210 - Flags: review?(jonas)
Comment on attachment 620210 [details] [diff] [review]
Add support for mime type filters (2nd draft corrected)

Review of attachment 620210 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4176,5 @@
> +    const nsDependentSubstring& token = tokenizer.nextToken();
> +
> +    nsString filterName;
> +    nsString extensionListStr;
> +    bool isTrustedFilter = false;

Just set this to true by default and then to false in the last else-branch where we use the mimeService.

::: content/html/content/src/nsHTMLInputElement.h
@@ +624,5 @@
>    bool                     mCanShowValidUI      : 1;
>    bool                     mCanShowInvalidUI    : 1;
> +
> +private:
> +  struct nsFilePickerFilter {

Sorry, not sure what I was thinking regarding "static". I'd prefer to simply move this to the .cpp file since it's really something internal to just one of the functions. Keeping header files small is always a win IMHO.

But I'm ok with keeping this as it is too.
Attachment #620210 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #41)
> Just set this to true by default and then to false in the last else-branch
> where we use the mimeService.
What I wanted to do is to have this flag set to false by default, i.e. always assuming that a filter is untrusted by default, until it is explicitly marked as trusted.
That was my point of view, but if you think that it should different, let me know and will change this.
Otherwise, if this patch sounds great to you, let me know what are the next steps for integrating it and how I can help.
I don't think it matters much if we "default" to trusted or not. It just seemed like less code to do it the other way around, but I think it's ok either way.

If you want to keep it as-is then this is ready to be checked in.
Keywords: checkin-needed
Actually, it would help if you attached a version of the patch which is created using |hg export|. That way it'll have the patch author and checkin comment ready which makes it easier for someone to just check this in.
Assignee: nobody → arnaud.bienner
Here is the same patch you already reviewed, but generated using "hg export", as you suggested.
Attachment #620210 - Attachment is obsolete: true
Attachment #620494 - Flags: review?(jonas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0fe30e6b1a

Should this have a test?

Also, thanks for the patch, Arnaud! If you're ever looking for more bugs to work on, feel free to stop by #developers on irc.mozilla.org and I'm sure someone there will be able to help.
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla15
(In reply to Ryan VanderMeulen from comment #46)
> Should this have a test?

Yes. Arnaud, are you interested in writing them?
Really sorry Arnaud, this had to be backed out of mozilla-inbound for the moment, since it fails test_bug377624.html:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d7271f499b8b
https://tbpl.mozilla.org/php/getParsedLog.php?id=11421850&tree=Mozilla-Inbound

{
12866 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | Correct filters should have been added - got 1, expected 9
12871 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | Correct filters should have been added - got 1, expected 257
12876 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | Correct filters should have been added - got 1, expected 513
12901 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | Correct filters should have been added - got 1, expected 9
12906 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | Correct filters should have been added - got 1, expected 513
12911 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | Correct filters should have been added - got 1, expected 257
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/94b06a04f17b

sicking, would you be able to help Arnaud resolve this if you have a moment? :-)
Target Milestone: mozilla15 → ---
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #47)
> Yes. Arnaud, are you interested in writing them?
Yes, why not ;)

I knew that some test tests will failed, as the behavior we discussed is slightly different from what we have today:
Before, only one filter was allowed; now several filters are allowed, and moreover, an extra "All Supported Types" filter is added, which is the concatenation of all valid filters.
Previously, this resulted in having no filter set.

But I guess that what makes the test to failed is that previously flags were used to set filters, and know, with my patch, only strings are used.

Should I create a new test_565274.html or can I modify (and rename, eventually) test_bug377624.html directly, and add new test cases for mime types filters?
Or both (i.e. correct test_bug377624.html and add a new test_565274.html only for mime type filter)?

Also, if you have any kind of documentation about writing this kind of tests, I'm very interested in :) (I had quick look on MDN but didn't find anything).

And one last question: how can I run this tests locally? Can I run only one test instead file instead of the test suite?
You can run tests locally following the directions on this page:
https://developer.mozilla.org/en/Mochitest#Running_select_tests

Otherwise, it might be worthwhile for you to request Level 1 commit access so you can push your patches to the Tryserver and test across all supported platforms (especially if you intend to work on other bugs in the future). If that interests you, I'm sure Jonas or myself could vouch for you.
http://www.mozilla.org/hacking/commit-access-policy/
Attached patch Update existing tests + new ones (obsolete) — Splinter Review
Here is a patch to update test_bug377624.html's tests.
After all, I do not create a new file for bug 565274, as it is very close to bug 377624, so I believe it is more consistent to have all these tests in one file.
But maybe we should at least rename it?

What I've done (quickly):
- check extension list directly, instead of filepicker's flags, because, as previously said, we do not use |filepicker::appendFilters(int flags)| anymore but |appendFilter(extensionList, name)| directly.

- update existing test cases as we now allow multiples filters, so expected results have changed in some cases

- check that number of filters is correct, including the "All Supported Types" filter which is added when there is more than one valid filter.
In case of several filters, I've checked only the "All Supported Types" extension list + the number of filters. I think it's enough, and it will be superfluous to also check each filter. 

- and I've added 2 new test cases for mimetypes. I've used common mimetypes which are present in mime type service default entry and which are very unlikely to be overridden by OS's mimetype mapping IMHO.

If it looks good to you, maybe you can resubmit previous patch with this test patch?
Comment on attachment 620494 [details] [diff] [review]
Add support for mime type filters (patch generated using hg export)

Just occurred to me that we should check that this doesn't break the android support for accept="image/*" etc. Requesting feedback from mounir to check that.
Attachment #620494 - Flags: feedback?(mounir)
Comment on attachment 620494 [details] [diff] [review]
Add support for mime type filters (patch generated using hg export)

Review of attachment 620494 [details] [diff] [review]:
-----------------------------------------------------------------

I stopped looking at the patch in the *huge* loop processing all items of the @accept. I believe writing one or two helper methods might be a good idea: it's really hard to read.

Anyway, this will indeed break our work with the Android filepicker. I do not understand why we are passing file extensions instead of |filterImage|, |filterAudio|, etc. to the nsIFilePicker object. Using those values was adding a very important abstraction and was allowing nsIFilePicker's implementation to do whatever they want. Passing file extensions is raw and meaningless data.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4142,5 @@
> +
> +  NS_ASSERTION(HasAttr(kNameSpaceID_None, nsGkAtoms::accept),
> +               "You should not call SetFilePickerFiltersFromAccept if the"
> +               " element has no accept attribute!");
> +  nsAutoString accept;

nit: a blank line before |nsAutoString accept;|

@@ +4150,5 @@
> +  // Services to retrieve image/*, audio/*, video/* filters
> +  nsCOMPtr<nsIStringBundleService> stringService =
> +    mozilla::services::GetStringBundleService();
> +  if (!stringService)
> +    return;

nit:
Coding style is:
if (foo) {
  bar;
}

You are repeating that mistake a few times below. Please fix those too.

@@ +4154,5 @@
> +    return;
> +  nsCOMPtr<nsIStringBundle> titleBundle, filterBundle;
> +  nsresult rv = stringService->CreateBundle("chrome://global/locale/filepicker.properties",
> +                                            getter_AddRefs(titleBundle));
> +  if (NS_FAILED(rv))

nit:
You could do:
if (NS_FAILED(stringService->CreateBundle(...)) {
  return;
}

That would prevent you from getting a nsresult you don't really use. You could change that a few times below too.

@@ +4237,5 @@
> +    filePicker->AppendFilter(title, allExtensionsList);
> +  }
> +
> +  // Add each filter
> +  for (unsigned int i = 0; i < filters.Length(); ++i) {

s/unsigned int/PRUint32/

::: content/html/content/src/nsHTMLInputElement.h
@@ +625,5 @@
>    bool                     mCanShowInvalidUI    : 1;
> +
> +private:
> +  struct nsFilePickerFilter {
> +    nsFilePickerFilter() {}

Why are you defining the default ctor?
Attachment #620494 - Flags: feedback?(mounir) → review-
(In reply to Arnaud from comment #49)
> Should I create a new test_565274.html or can I modify (and rename,
> eventually) test_bug377624.html directly, and add new test cases for mime
> types filters?
> Or both (i.e. correct test_bug377624.html and add a new test_565274.html
> only for mime type filter)?

Could you move the test in content/html/content/test/forms/ and rename it something like test_input_file_picker.html? (feel free to pick a better name)
This test is not testing code from layout but from content AFAICT.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #54)
> Anyway, this will indeed break our work with the Android filepicker. I do
> not understand why we are passing file extensions instead of |filterImage|,
> |filterAudio|, etc. to the nsIFilePicker object. Using those values was
> adding a very important abstraction and was allowing nsIFilePicker's
> implementation to do whatever they want. Passing file extensions is raw and
> meaningless data.
We need to get file extensions in order to create an "All Supported Types" filter, which is the concatenation of all valid filters' extensions.
This is not possible if we use nsIFilePicker's flags.

Also, I wanted to add filters in the following order:
- filterAll
- AllSupportedTypes (if more than one valid filter)
- each filter, in the order they appear in the accept attribute
So I need to first loop over all the elements in the accept, construct "AllSupportedTypes" filter and a filters array, then actually adding each filter to the filePicker.
Having the same kind of filter (i.e. stored in a  new |nsFilePickerFilter struct|) is much more easier than mixing flag and (string name, string extensionList) pairs IMHO.
We can imagine something tricky like supporting flags in |nsFilePickerFilter struct| (using union or two different constructors), but as we already have extension list (for "AllSupportedTypes") I see no advantage of using flags anymore.
Also, it is still generic as the list of extension is still retrieved from properties file, as it is when using flags.

> Why are you defining the default ctor?
I store nsFilePickerFilter in a nsTArray<nsFilePickerFilter>.
And in https://developer.mozilla.org/en/XPCOM_array_guide#nsTArray.3CT.3E, it is specified that "The objects must define a default constructor and a copy constructor."
I'm not sure, but I think that not having a default constructor doesn't work.
I will check if you want.

About all the other minor things, I wasn't aware of everything (I looked to existing code to have an idea about this), so thanks for pointing this out ;)

About the *huge* loop, I know there is quite lot of code, but in fact, there is not so much logic inside finally, and I've tried to add comments to make this clearer. I hoped it would be enough :(
I personally think the loop is fine.

What you'll need to do is to add a mFilterMask member to nsFilePickerFilter and use that to call appendFilters instead where appropriate.
I thought that |AppendFilters(int flags)| was just a helper function, as I looked to |nsBaseFilePicker| which is used "almost" everywhere:
http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseFilePicker.cpp#92

"Almost" because indeed, for Android, the behavior is a little bit different:
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsFilePicker.cpp#54
|AppendFilters(int flags)| stores filters in a |mMimeTypeFilter|, whereas |AppendFilter(string title, string extensions)| stores the extensions directly.
So |AppendFilters(int flags)| and |AppendFilter(string title, string extensions)| haven't the same behavior in Android, but have the same behavior in other filePickers classes.
In case of flags were used, Android use a |ShowFilePickerForMimeType| method, and in the other case a |ShowFilePickerForExtensions| method:
http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.cpp#796
http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.cpp#817

I understood that this is done to allow the use of an external file picker (as an external filePicker can have its own interpretation of the mime type).
Indeed, maybe "image/*" could be more than just a list of file extensions on Android? I guess that's why flags are treated differently for Android.

So, my question is:
- as Android filePicker treats "image/*", "video/*" and "audio/*" mime types differently, does it will make sense to add a generic "AppendMimeTypeFilter" method to all nsFilePicker? So the logic will be deported in the filePicker classes?
- or does the magic mimetypes ("image/*", etc.) are really something specific, so it makes sense to treat them specifically, but it wouldn't makes sense to treat all mime type in the same way?

In the second case (i.e. considering that magic mimetypes are something really apart), my patch can be modified easily by adding an extra flag attribute to the |nsFilePickerFilter|, as suggested by Mounir, so we can treat magic mimetypes differently, using |AppendFilter(int flags)| instead of |AppendFilter(string title, string extensions)| when flag is defined (other fields ignored in this case).

I hope I was clear enough. If not, please let me know.
I think we should keep things simple in this bug and just stick with the API that we have. If it turns out that switching to using mime-types in the future is a good idea we can do that then, but for now I think we should just get this bug in. It's not clear to me that mime-types is better than a bit-field so I'd like to not block on deciding that.
I've modified my patch in the way you suggested: adding an extra |mFilterMask| field  to the |nsFilePickerFilter| struct, and calling |AppendFilters| instead of |AppendFilter| when this mask is set.
So the API remains unchanged and Android's file picker can act differently when using filter mask.

I've included tests in the patch this time (+ move the test file, as suggested by Mounir).
Attachment #620494 - Attachment is obsolete: true
Attachment #620916 - Attachment is obsolete: true
Attachment #622051 - Flags: review?
Attachment #622051 - Flags: feedback?
Attachment #622051 - Flags: review?(jonas)
Attachment #622051 - Flags: review?
Attachment #622051 - Flags: feedback?(mounir)
Attachment #622051 - Flags: feedback?
(In reply to Ryan VanderMeulen from comment #46)
> Also, thanks for the patch, Arnaud! If you're ever looking for more bugs to
> work on, feel free to stop by #developers on irc.mozilla.org and I'm sure
> someone there will be able to help.
Why not :)
I was particularly interested by this particular bug, but, as I love Firefox I would be glad to contribute more often, if I've enough time.
I will definitely have a look to the bug list once this one will be resolved :)
Btw, thanks for the documentation link about test cases ;)
Comment on attachment 622051 [details] [diff] [review]
Add support for mime type filters (3rd draft)

Review of attachment 622051 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming that this passes on try. Path looks great! Still want to hear mounir's feedback to make sure we're not breaking android.
Attachment #622051 - Flags: review?(jonas) → review+
Comment on attachment 622051 [details] [diff] [review]
Add support for mime type filters (3rd draft)

Review of attachment 622051 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I would like to have some answers and have some stuff fixed if they were actually broken.

Also, there is something you could improve. It could be a follow-up bug but that might be nice to do that here:
If you have two filters based on mask like "audio/*, video/*", right now, you default to a long list of extensions. However, the widgets might be interested to know this is actually "audio/*, video/*". So, IMO, we should add another special case: if all filters are masks, we should add a new filter entry which is the additions of all masks and we should default to it instead of default to "All".
Concretely, if we do that, we could have <input type='file' accept='audio/*, video/*'> show a UI for picture *or* video instead of a generic UI.

Very sorry for the late feedback.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4163,5 @@
> +               " element has no accept attribute!");
> +
> +  nsAutoString accept;
> +  nsTArray<nsFilePickerFilter> filters;
> +  nsString allExtensionsList;

Can you move those just before the |GetAttr| below? You don't need them here.

@@ +4189,5 @@
> +  HTMLSplitOnSpacesTokenizer tokenizer(accept, ',');
> +
> +  // Retrieve all filters
> +  while (tokenizer.hasMoreTokens()) {
> +    const nsDependentSubstring& token = tokenizer.nextToken();

Do an early check if token is empty. That would avoid weird behavior. I think right now, your code would add a filter with no name and no extension...

BTW, could you test that?

@@ +4198,5 @@
> +
> +    // First, check for image/audio/video filters...
> +    if (token.EqualsLiteral("image/*")) {
> +      filterMask = nsIFilePicker::filterImages;
> +      filterBundle->GetStringFromName(NS_LITERAL_STRING("imageFilter").get(), getter_Copies(extensionListStr));

80 chars limitation, please.

@@ +4201,5 @@
> +      filterMask = nsIFilePicker::filterImages;
> +      filterBundle->GetStringFromName(NS_LITERAL_STRING("imageFilter").get(), getter_Copies(extensionListStr));
> +    } else if (token.EqualsLiteral("audio/*")) {
> +      filterMask = nsIFilePicker::filterAudio;
> +      filterBundle->GetStringFromName(NS_LITERAL_STRING("audioFilter").get(), getter_Copies(extensionListStr));

ditto

@@ +4204,5 @@
> +      filterMask = nsIFilePicker::filterAudio;
> +      filterBundle->GetStringFromName(NS_LITERAL_STRING("audioFilter").get(), getter_Copies(extensionListStr));
> +    } else if (token.EqualsLiteral("video/*")) {
> +      filterMask = nsIFilePicker::filterVideo;
> +      filterBundle->GetStringFromName(NS_LITERAL_STRING("videoFilter").get(), getter_Copies(extensionListStr));

ditto

@@ +4208,5 @@
> +      filterBundle->GetStringFromName(NS_LITERAL_STRING("videoFilter").get(), getter_Copies(extensionListStr));
> +    } else {
> +      //... if no image/audio/video filter is found, check mime types filters
> +      nsCOMPtr<nsIMIMEInfo> mimeInfo;
> +      if (!token.IsEmpty() &&

You can remove that check with the early check above.

@@ +4210,5 @@
> +      //... if no image/audio/video filter is found, check mime types filters
> +      nsCOMPtr<nsIMIMEInfo> mimeInfo;
> +      if (!token.IsEmpty() &&
> +          NS_SUCCEEDED(mimeService->GetFromTypeAndExtension(NS_ConvertUTF16toUTF8(token),
> +                                                            EmptyCString(), // No extension

80 chars limitation too.

@@ +4212,5 @@
> +      if (!token.IsEmpty() &&
> +          NS_SUCCEEDED(mimeService->GetFromTypeAndExtension(NS_ConvertUTF16toUTF8(token),
> +                                                            EmptyCString(), // No extension
> +                                                            getter_AddRefs(mimeInfo)))) {
> +        if (mimeInfo) {

What would happen if mimeInfo == null? It seems like we would add an empty filter? Shouldn't we call |continue| in that case instead?

@@ +4217,5 @@
> +          // Get mime type name
> +          nsCString mimeTypeName;
> +          mimeInfo->GetType(mimeTypeName);
> +          CopyUTF8toUTF16(mimeTypeName, filterName);
> +          // Get extension list

Please, leave an blank line before that comment. Aeration is good.

@@ +4220,5 @@
> +          CopyUTF8toUTF16(mimeTypeName, filterName);
> +          // Get extension list
> +          nsCOMPtr<nsIUTF8StringEnumerator> extensions;
> +          mimeInfo->GetFileExtensions(getter_AddRefs(extensions));
> +          bool hasMore;

Same before |bool hasMore;|

@@ +4253,5 @@
> +    }
> +  }
> +
> +  // Add "All Supported Types" filter
> +  if (filters.Length() > 1) {

I would prefer to have "All supported types" to be added at the end. That wouldn't change that much but seems more natural.
I wouldn't mind if you do not change this.

::: content/html/content/src/nsHTMLInputElement.h
@@ +53,5 @@
>  #include "nsIFile.h"
>  
>  class nsDOMFileList;
> +class nsIFilePicker;
> +class nsIMIMEInfo;

I'm not sure why you are forward declaring nsIMimeInfo here...
Attachment #622051 - Flags: feedback?(mounir) → review-
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #63) 
> r- because I would like to have some answers and have some stuff fixed if
> they were actually broken.
Just to be sure: are you talking about the answers below about my code? Or some more global stuff?
I mean: I'm fine to rework the patch if needed, but if finally it can't be accepted because this will break things (from a design point of view, no matter how it's implemented) I would prefer not to waste time on this and to have answers answered first ;)

> Also, there is something you could improve. It could be a follow-up bug but
> that might be nice to do that here:
Yep, you're totally right: I thought about this recently, after submitting my patch (I didn't take time to update this bug though :( ) but indeed, as we default to filter if we have only one and if it's a "trusted" filter, we should default to "All Supported Types" if all filters are trusted.

> If you have two filters based on mask like "audio/*, video/*", right now,
> you default to a long list of extensions. However, the widgets might be
> interested to know this is actually "audio/*, video/*". So, IMO, we should
> add another special case: if all filters are masks, we should add a new
> filter entry which is the additions of all masks and we should default to it
> instead of default to "All".
I don't think this can be implemented with the current API.
Maybe we should change it to allow this special case, but I'm not sure about the best way to do it... (moving logic to file picker(s)? special case (i.e. (weird) hack) for Android file picker with filter called "All Supported Type"?)

> Do an early check if token is empty. That would avoid weird behavior. I
> think right now, your code would add a filter with no name and no
> extension...
> 
> BTW, could you test that?
Yep

> > +      filterBundle->GetStringFromName(NS_LITERAL_STRING("imageFilter").get(), getter_Copies(extensionListStr));
> 
> 80 chars limitation, please.
I tried to do that in all my code, but I missed that one, sorry :(
But this code is taken from |nsBaseFilePicker| so in that case, it's not entirely my fault :p
Anyway, I will change this here and elsewhere of course.

> What would happen if mimeInfo == null? It seems like we would add an empty
> filter? Shouldn't we call |continue| in that case instead?
Indeed, |continue| should be better (will also avoid having too much indentation).
However, this is a sanity check, just in case: I don't think mimeService will ever return a NULL mimeInfo. If I'm right, it returns an empty mimeInfo (title and extension list empties) if it can't find the mime type.

> > +  // Add "All Supported Types" filter
> > +  if (filters.Length() > 1) {
> 
> I would prefer to have "All supported types" to be added at the end. That
> wouldn't change that much but seems more natural.
> I wouldn't mind if you do not change this.
IMHO it's more natural to have at first: "All (*.*)", then "All Supported Types" (more restrictive) then each filter (even more restrictive).
I suggest to keep this as is for now, and change it if there are negative feedbacks about this.

> > +class nsIMIMEInfo;
> 
> I'm not sure why you are forward declaring nsIMimeInfo here...
Indeed, it's doesn't seem to be necessary.
(In reply to Arnaud from comment #64)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #63) 
> > r- because I would like to have some answers and have some stuff fixed if
> > they were actually broken.
> Just to be sure: are you talking about the answers below about my code? Or
> some more global stuff?

Mostly the issues regarding the early returns/edge cases.

> I mean: I'm fine to rework the patch if needed, but if finally it can't be
> accepted because this will break things (from a design point of view, no
> matter how it's implemented) I would prefer not to waste time on this and to
> have answers answered first ;)

No worry with that. Your patch in its current state doesn't seem to break anything. I'm just worried about some edge cases that are not handled.
As soon as it will be done, you will be ready to go IMO.

> > Also, there is something you could improve. It could be a follow-up bug but
> > that might be nice to do that here:
> Yep, you're totally right: I thought about this recently, after submitting
> my patch (I didn't take time to update this bug though :( ) but indeed, as
> we default to filter if we have only one and if it's a "trusted" filter, we
> should default to "All Supported Types" if all filters are trusted.
> 
> > If you have two filters based on mask like "audio/*, video/*", right now,
> > you default to a long list of extensions. However, the widgets might be
> > interested to know this is actually "audio/*, video/*". So, IMO, we should
> > add another special case: if all filters are masks, we should add a new
> > filter entry which is the additions of all masks and we should default to it
> > instead of default to "All".
> I don't think this can be implemented with the current API.
> Maybe we should change it to allow this special case, but I'm not sure about
> the best way to do it... (moving logic to file picker(s)? special case (i.e.
> (weird) hack) for Android file picker with filter called "All Supported
> Type"?)

Lets open a follow-up bug for that.

> > Do an early check if token is empty. That would avoid weird behavior. I
> > think right now, your code would add a filter with no name and no
> > extension...
> > 
> > BTW, could you test that?
> Yep

I meant in the test suite, right?
 
> > What would happen if mimeInfo == null? It seems like we would add an empty
> > filter? Shouldn't we call |continue| in that case instead?
> Indeed, |continue| should be better (will also avoid having too much
> indentation).
> However, this is a sanity check, just in case: I don't think mimeService
> will ever return a NULL mimeInfo. If I'm right, it returns an empty mimeInfo
> (title and extension list empties) if it can't find the mime type.

I guess we should check that then. We should silently ignore if someone puts: accept="somestupidthing" instead of having a blank filter in the file picker.
A test might be interesting: check that accept="image/*, someunknownthing" is equivalent to accept="image/*".
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #65)
> No worry with that. Your patch in its current state doesn't seem to break
> anything. I'm just worried about some edge cases that are not handled.
> As soon as it will be done, you will be ready to go IMO.

What are these edges cases you're talking about?
It's not clear to me if that are things about my current implementation, or something else within Mozilla that I'm not aware of.
From what I've understood, it's only the special/tricky cases that may create weird behavior that you pointed out, am I right?

> Lets open a follow-up bug for that.

OK. So it's not a blocking issue for you?
Also, I believe we should open another follow-up bug for handling more filter as "trusted" (all those who are in nsIMimeService's hard-coded set, as we discussed previously).

> > Do an early check if token is empty. That would avoid weird behavior. I
> > think right now, your code would add a filter with no name and no
> > extension...
> > 
> > BTW, could you test that?
> I meant in the test suite, right?

Yes, I thought about this too :)
But in fact, now, my code doesn't add empties filters, as explained below.
But indeed, we should add a test for this, and I should check if token is empty earlier, to avoid unnecessary processing and make code clearer.

>  
> > > What would happen if mimeInfo == null? It seems like we would add an empty
> > > filter? Shouldn't we call |continue| in that case instead?
> > Indeed, |continue| should be better (will also avoid having too much
> > indentation).
> > However, this is a sanity check, just in case: I don't think mimeService
> > will ever return a NULL mimeInfo. If I'm right, it returns an empty mimeInfo
> > (title and extension list empties) if it can't find the mime type.
> 
> I guess we should check that then. We should silently ignore if someone
> puts: accept="somestupidthing" instead of having a blank filter in the file
> picker.

In fact, it's already tested: we check that the extensionList isn't empty before adding it to our list of mime types.
And there is already a test for this:
<input id='z' type='file' accept="i/am,a,pathological,;,,,,test/case">
But it would be better to check that extensionList AND filter aren't empties, instead of only extensionList.

> A test might be interesting: check that accept="image/*, someunknownthing"
> is equivalent to accept="image/*".
<input id='i' type='file' accept="mime/type;parameter,video/*">
should be what you're thinking about, right?
It's equivalent to <input type="file" accept="video/*" />, as expected.

Is there any other things you're worrying about? Or can I change my patch with the modifications discussed?
(In reply to Arnaud from comment #66)
> Is there any other things you're worrying about? Or can I change my patch
> with the modifications discussed?

You had an answer for everything. Do the changes we discussed and it will be good to go.

Indeed, the follow-up bug isn't a blocker because you are not regressing something, it would just be nice to have that new feature. If you are willing to work on that in another bug, that would be awesome :)
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #67)
> You had an answer for everything. Do the changes we discussed and it will be
> good to go.

OK :)
Will rework this when I will have a minute.

> Indeed, the follow-up bug isn't a blocker because you are not regressing
> something, it would just be nice to have that new feature. If you are
> willing to work on that in another bug, that would be awesome :)

OK, thanks for the clarification.

I'm OK to work on the "considering more filters are trusted" thing, because I've got some ideas about how to implement this, but for the "we only have filters represented with mask" special case, I don't really know how this can be implemented for now :/ so it might not be the best guy to do this.
So maybe you can open two follow-up bugs, and assign the first one to me?
For something like accept="image/*,video/*" we in firefox still want the behavior of displaying a filepicker like:

all files: *,*
All Supported Types: <all image extensions>;<all video extensions>;
image files: <all images extensions>
video files: <all video extensions>

So simply calling AppendFilters with the image and video bits set wouldn't give the right behavior.

I'd say let's do that as a followup. I have some ideas for how to handle it, but since the current approach doesn't regress anything I think we should go with that.

But please do file the followup.
So I've changed my patch a little bit, taking into account Mounir's comments.

Only minor changes: coding style changes + few sanity checks added or performed differently + few tests cases added, as discussed.

The only visible thing I changed is to select "All Supported Types" as default filter (instead of All *.*) if there are only "trusted" filters (i.e. when mixing image/*,video/* or audio/*).
Attachment #622051 - Attachment is obsolete: true
Attachment #627380 - Flags: review?(jonas)
Attachment #627380 - Flags: feedback?(mounir)
Comment on attachment 627380 [details] [diff] [review]
Add support for mime type filters (4th draft)

Mounir's review is enough at this point.
Attachment #627380 - Flags: review?(mounir)
Attachment #627380 - Flags: review?(jonas)
Attachment #627380 - Flags: feedback?(mounir)
Comment on attachment 627380 [details] [diff] [review]
Add support for mime type filters (4th draft)

Review of attachment 627380 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with all the comments applied.

Also, for the |mIsTrusted| issue, I would do this:
- remove the optional trusted parameter in the ctor taking extensions in parameter;
- add a comment for |mIsTrusted| explaining that for the moment, it will be true if we are using a filter mask and false when using extensions;
- add a comment in operator= explaining that |mIsTrusted| *should* be the same if the two objects are the some because it depends on |mFilterMask|;
- add a NS_ASSERTION in operator= checking that the previous assertion is actually true.

Attach a new patch with the requested changes, I will push it to try and, if green, to mozilla-inbound.

Thank you for your work on this! :)

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4162,5 @@
> +      nsCOMPtr<nsIMIMEInfo> mimeInfo;
> +      if (NS_SUCCEEDED(mimeService->GetFromTypeAndExtension(
> +              NS_ConvertUTF16toUTF8(token),
> +              EmptyCString(), // No extension
> +              getter_AddRefs(mimeInfo)))) {

nit: I think indentation here should be like this:
if (NS_SUCCEEDED(mimeService->GetFromTypeAndExtension(
                   NS_ConvertUTF16toUTF8(token),
                   EmptyCString(),
                   getter_AddRefs(mimeInfo))) {
(In other words, NS_Convert*, EmptyCString and getter_AddRefs should be aligned two chars after |mimeService|)

@@ +4166,5 @@
> +              getter_AddRefs(mimeInfo)))) {
> +
> +        if (!mimeInfo) {
> +          continue;
> +        }

Why not:
if (NS_FAILED(mimeService->GetFromTypeAndExtension(
               NS_ConvertUTF16toUTF8(token),
               EmptyCString(), // No extension
               getter_AddRefs(mimeInfo))) ||
    !mimeInfo) {
  continue;
}

@@ +4185,5 @@
> +          }
> +          if (!extensionListStr.IsEmpty()) {
> +            extensionListStr.AppendLiteral("; ");
> +          }
> +          extensionListStr += NS_ConvertUTF8toUTF16("*.") +

Maybe you could use NS_LITERAL_STRING here.

@@ +4197,5 @@
> +      continue;
> +    }
> +
> +    // If we arrived here, that means we have a valid filter: let's create it
> +    // and it to our list, if no similar filter is already present

nit: // and *add* it to our list, [...].

@@ +4205,5 @@
> +    } else {
> +      filter = nsFilePickerFilter(filterName, extensionListStr);
> +    }
> +
> +    if (!filters.Contains(filter)) {

So, how do you handle two filters being the same but with mIsTrusted being different?

::: content/html/content/src/nsHTMLInputElement.h
@@ +607,5 @@
> +
> +    nsFilePickerFilter(const nsString& aTitle,
> +                       const nsString& aFilter,
> +                       const bool aIsTrusted = false)
> +      : mFilterMask(0), mTitle(aTitle), mFilter(aFilter), mIsTrusted(aIsTrusted) {}

Seems like this ctor with |aIsTrusted = true| is never used, is it?

@@ +617,5 @@
> +      mIsTrusted = other.mIsTrusted;
> +    }
> +
> +    bool operator== (const nsFilePickerFilter& other) const {
> +      return (mFilter == other.mFilter) && (mFilterMask == other.mFilterMask);

What if mIsTrusted is different?
Attachment #627380 - Flags: review?(mounir) → review+
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #72)
> Also, for the |mIsTrusted| issue, I would do this:
> - remove the optional trusted parameter in the ctor taking extensions in
> parameter;

I was thinking about the possibility to add trusted filters with extensions in the future.
In fact, I think I will try to work on this afterwards, as it may be added easily, so I want this struct to be ready to handle such cases.
So I would prefer not to change this.
But I also wanted filters with extensions to be considered as untrusted by default, that's why it is an optional parameter.
Is this OK for you?

> - add a comment for |mIsTrusted| explaining that for the moment, it will be
> true if we are using a filter mask and false when using extensions;

OK: I think I will add this.
We will need to remove this if we allow more filters to be considered as "trusted" later.

> - add a comment in operator= explaining that |mIsTrusted| *should* be the
> same if the two objects are the some because it depends on |mFilterMask|;
> - add a NS_ASSERTION in operator= checking that the previous assertion is
> actually true.

OK. So I think I should do something like this:
if (mFilter == other.mFilter) && (mFilterMask == other.mFilterMask) {
  NS_ASSERTION(mIsTrusted = other.mIsTrusted);
  return true;
} else {
  return false;
}

> Attach a new patch with the requested changes, I will push it to try and, if
> green, to mozilla-inbound.

Great :)

> 
> Thank you for your work on this! :)

You're welcome ;)

> @@ +4166,5 @@
> > +              getter_AddRefs(mimeInfo)))) {
> > +
> > +        if (!mimeInfo) {
> > +          continue;
> > +        }
> 
> Why not:
> if (NS_FAILED(mimeService->GetFromTypeAndExtension(
>                NS_ConvertUTF16toUTF8(token),
>                EmptyCString(), // No extension
>                getter_AddRefs(mimeInfo))) ||
>     !mimeInfo) {
>   continue;
> }

I was afraid that it might be less readable like this.
But I you don't think so, I can change this.


If you agree with my previous comments, I will change my patch, with your remarks.
However, I will not have time to work on this until next week.
(In reply to Arnaud from comment #73)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #72)
> > Also, for the |mIsTrusted| issue, I would do this:
> > - remove the optional trusted parameter in the ctor taking extensions in
> > parameter;
> 
> I was thinking about the possibility to add trusted filters with extensions
> in the future.
> In fact, I think I will try to work on this afterwards, as it may be added
> easily, so I want this struct to be ready to handle such cases.
> So I would prefer not to change this.
> But I also wanted filters with extensions to be considered as untrusted by
> default, that's why it is an optional parameter.
> Is this OK for you?

I'm not a big fan of this but I guess if you assert when |mIsTrusted| is different, that could be enough...
I've made the changed we discussed about.

Also, I've removed the "appendFilterCalled" test, which wasn't working anymore, because of some changes in your Javascript engine recently I guess, which do not authorize to do: |this.appendFilterCalled = true;|

In fact, I'm surprised that this test has even worked...

Anyway, it doesn't work make sense now, as calling |appendFilter| (i.e. the one that takes extensions in parameter) is a valid use case, and that we already check that the expected filters have been added (which means that |appendFilter| should have been called, or not, depending on the kind of mime types we want to added in our tests).
Attachment #627380 - Attachment is obsolete: true
Attachment #630294 - Flags: review?(mounir)
Comment on attachment 630294 [details] [diff] [review]
Add support for mime type filters (5th draft)

Review of attachment 630294 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to push that to try.
Attachment #630294 - Flags: review?(mounir) → review+
Attached patch test fixSplinter Review
I had to write a fix for the test because platforms don't all agree regarding which files should be associated with "application/xhtml+xml,application/xhtml+xml".
I was looking at this:

In fact what I've tried to test that adding multiple times a mime type filter will result in only one filter (i.e. no duplicate).

This is already tested with test 'j' for filters with masks, but I prefer to test this for filter with "normal" mime types.

The change you propose is great :)
Otherwise, I was thinking about changing the mimetype: I've used "application/xhtml+xml" because I thought it will not changed over platforms.
But as it is not the case, we can:
- change the expected output depending on platforms, as you did
- use another mime type with is more common. For example, "image/gif" or "image/png" should be OK as they are used in test 'k' which is working everywhere.

I let you decide what is the best solution.
(In reply to Arnaud from comment #79)
> Otherwise, I was thinking about changing the mimetype: I've used
> "application/xhtml+xml" because I thought it will not changed over platforms.
> But as it is not the case, we can:
> - change the expected output depending on platforms, as you did
> - use another mime type with is more common. For example, "image/gif" or
> "image/png" should be OK as they are used in test 'k' which is working
> everywhere.

Actually, the later seems better. I've updated the patch to do that.
Pushed to mozilla-inbound. Tests are green.
Status: REOPENED → ASSIGNED
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla16
Attachment #630294 - Flags: checkin+
> Target Milestone: --- → mozilla16
We missed mozilla15 by one or two days, right?
Too bad to have missed this milestone :p I would have been glad to test it "for real" sooner.
Anyway, it's not a big deal :)
Eh, indeed. But it is in the tree and that is pretty awesome! Thank you for the hard work you put in that patch. Your perseverance paid ;)
https://hg.mozilla.org/mozilla-central/rev/5f585e34892b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 773985
No longer depends on: 773985
Thanks for the solutions.

I think that implementetion of the a file filter that accepts the most common mime types. Is possible add some text files mime types like "text/html;text/htm;text/txt..."?.
I changed the compat table here: https://developer.mozilla.org/en-US/docs/HTML/Element/Input and added that change to https://developer.mozilla.org/en-US/docs/Firefox_16_for_developers. Anything else that needs to be done here? The documentation for <input> already mentions mimetypes for accept.
(In reply to Tom Schuster [:evilpie] from comment #87)
> Anything else that needs to be done here? 
Just one thing: you might update the compat table for Opera: it also implements the accept attribute for mime types (I don't since which release though).
Depends on: 826185
Blocks: 826185
No longer depends on: 826185
Blocks: 826176
You need to log in before you can comment on or make changes to this bug.