Closed Bug 832923 Opened 11 years ago Closed 11 years ago

Implement <input type='file'> on B2G

Categories

(Core Graveyard :: Widget: Gonk, defect)

defect
Not set
normal

Tracking

(firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18+ verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 + verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: mounir, Assigned: baku)

References

Details

(4 keywords, Whiteboard: [triaged:1/22])

Attachments

(5 files, 9 obsolete files)

9.43 KB, patch
baku
: review+
Details | Diff | Splinter Review
30.00 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.40 KB, patch
baku
: review+
Details | Diff | Splinter Review
29.92 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.42 KB, patch
sicking
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #801635 +++


Given that bug 801635 has been transformed to disable <input type='file'> (in a way that helps web developers finding it), this bug will handle the implementation of the functionality.
Triage: leo? shira is to be as close to tef as possible
blocking-b2g: shira? → leo?
Whiteboard: [triaged:1/22]
There's no product or Web standards definition for what this would actually do. Not holding the 1.1 release for this.
blocking-b2g: leo? → -
(In reply to Dietrich Ayala (:dietrich) from comment #2)
> There's no product or Web standards definition for what this would actually
> do. Not holding the 1.1 release for this.

Does this mean you want to postpone this feature further? If so, this would be pretty disappointing for me. I need this feature urgently to make my app working and I'm sure, I'm not the only one. In Firefox for Android this works without problems, in Firefox for Firefox OS it does not. Pretty hard to argue.
(In reply to Dietrich Ayala (:dietrich) from comment #2)
> There's no product or Web standards definition for what this would actually
> do. Not holding the 1.1 release for this.

No Web Standards definition of <input type='file'>? What do you mean by that?
Flags: needinfo?(dietrich)
(In reply to testit from comment #3)
> (In reply to Dietrich Ayala (:dietrich) from comment #2)
> > There's no product or Web standards definition for what this would actually
> > do. Not holding the 1.1 release for this.
> 
> Does this mean you want to postpone this feature further? If so, this would
> be pretty disappointing for me. I need this feature urgently to make my app
> working and I'm sure, I'm not the only one. In Firefox for Android this
> works without problems, in Firefox for Firefox OS it does not. Pretty hard
> to argue.

No one says we don't want that. If someone has time to provide an implementation, great! But it's not a top priority at the moment.
I'm honestly not sure if this is a blocker or not. There definitely are web standards describing how <input type=file> should work. This has been described since the HTML4/DOM2 days.

This is a feature used often enough on the web that I think the level of web breakage might rise to this being a blocker.

But rather than argue if this is a blocker or not, I think we should assign it to someone.

Mounir: Would you do the honors?
Downstream bug for Wikipedia's mobile interface photo upload feature:
https://bugzilla.wikimedia.org/show_bug.cgi?id=45500

I'm trying to work around lack of <input type="file"> by using a 'pick' MozActivity; it's sort of working but not production-ready yet (crashes my tabs a lot). Would be a lot easier on us and others if it were handled by the browser though.
Can you please file a separate bug, with steps-to-reproduce, for the crash you are seeing. WebActivities are intended to be production ready at this point. (Even though I do agree that they are no substitute for having <input type=file> implemented)
(In reply to Jonas Sicking (:sicking) from comment #10)
> Can you please file a separate bug, with steps-to-reproduce, for the crash
> you are seeing. WebActivities are intended to be production ready at this
> point. (Even though I do agree that they are no substitute for having <input
> type=file> implemented)

Filed as bug 845989. It might just be that the tab is a memory hog when dealing with full-size camera images, in which case I'll see about reducing the RAM usage.
(In reply to Brion Vibber from comment #11)
> Filed as bug 845989. It might just be that the tab is a memory hog when
> dealing with full-size camera images, in which case I'll see about reducing
> the RAM usage.

Closed that out; on further investigation we were using a *lot* of extra memory due to bug 839630 (Gallery returned a giant PNG instead of a JPEG). I've also filed bug 846015 for the stripping of EXIF data and recompression that Gallery seems to perform when picking.
I will work on this next week.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Flags: needinfo?(dietrich)
Blocks: 709742
No longer blocks: 759986
Keywords: compat
We are getting compat' issues with this. Shouldn't we block?
Assignee: mounir → amarchesini
blocking-b2g: - → leo?
(In reply to Mounir Lamouri (:mounir) from comment #14)
> We are getting compat' issues with this. Shouldn't we block?

That's feature work, and leo is due for March 15. If you can make it, great!
(In reply to Fabrice Desré [:fabrice] from comment #15)
> (In reply to Mounir Lamouri (:mounir) from comment #14)
> > We are getting compat' issues with this. Shouldn't we block?
> 
> That's feature work, and leo is due for March 15. If you can make it, great!

Right. Feature work that greatly helps with compatibility. That's why it's got a tracking+.
blocking-b2g: leo? → ---
Attached patch patch (obsolete) — — Splinter Review
This works but it creates a temporary file and then it doesn't remove it...
In general, give me a feedback about what I wrote so far.
Attachment #723513 - Flags: feedback?(mounir)
Comment on attachment 723513 [details] [diff] [review]
patch

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

::: b2g/components/FilePicker.js
@@ +50,5 @@
> +  /* methods */
> +
> +  init: function(parent, title, mode) {
> +    if (mode != Ci.nsIFilePicker.modeOpen) {
> +      throw 'Only mode \'modeOpen\' is supported on B2G';

nit: there's a mix of single and double quotes in this file.

@@ +56,5 @@
> +  },
> +
> +  /* readonly attribute nsILocalFile file; */
> +  get file() { return this.mFilesEnumerator ?
> +                 this.mFilesEnumerator.mFiles[0] : null; },

nit: indentation looks odd, even if it's correct ;)

@@ +70,5 @@
> +    if (!this.mFilesEnumerator)
> +      return null;
> +
> +    var ioService = Cc['@mozilla.org/network/io-service;1']
> +                  .getService(Ci.nsIIOService);

this is Services.io

@@ +191,5 @@
> +      if (!tmp.exists()) {
> +        tmpFile = tmp;
> +        break;
> +      }
> +    }

Why not use https://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#271 ?

@@ +198,5 @@
> +      cbError();
> +      return;
> +    }
> +
> +    // TODO: how to delete this tmp file?!?

https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsIExternalHelperAppService.idl#64 will delete the file for you when the process shuts down. Since all our web content is OOP, we'll keep the files only as long as the app is running, which is ok I think.

But can't we just pass blobs instead of real files?
(In reply to Fabrice Desré [:fabrice] from comment #18)
> But can't we just pass blobs instead of real files?

Speaking from the consumer end... many web apps expect a file name along with the file. If the blob has a filename on it which has the expected extension, that's probably good enough.

Question: does this work for both pulling File objects out of input.files array, and for direct form submissions too?
(In reply to Fabrice Desré [:fabrice] from comment #18) 
> @@ +191,5 @@
> > +      if (!tmp.exists()) {
> > +        tmpFile = tmp;
> > +        break;
> > +      }
> > +    }
> 
> Why not use
> https://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#271 ?

Actually, why not use OS.File to keep things out of the ui thread?
Attached patch patch (obsolete) — — Splinter Review
Attachment #723513 - Attachment is obsolete: true
Attachment #723513 - Flags: feedback?(mounir)
Attachment #723559 - Flags: review?(mounir)
> Speaking from the consumer end... many web apps expect a file name along
> with the file. If the blob has a filename on it which has the expected
> extension, that's probably good enough.

Good point. The new version of this patch does this. Just for a small set of mimetype.

> Question: does this work for both pulling File objects out of input.files
> array, and for direct form submissions too?

I tested it just for input.files, but this is the implementation of the nsIFilePicker, so any piece of code that uses it should work. Do you have in mind some particular test?
Thanks for the review, fabrice.

> But can't we just pass blobs instead of real files?

 /**
  * Get the nsIFile for the file or directory.
  *
  * @return Returns the file currently selected
  */
  readonly attribute nsIFile file;

Unfortunately it seems that nsIFilePicker.idl wants nsIFile.
> Actually, why not use OS.File to keep things out of the ui thread?

Oh.. OS.File is nice :) But can we have nsIFile from it?
Btw, this code is asynchronous.
Well, if the code is asynchronous, you should be able to port it to OS.File without heavy refactoring. But if you absolutely need a nsIFile, OS.File won't help you get it. You'll have to open the nsIFile from the path.
Keywords: feature
We should always expose File objects to the calling webpage. But we shouldn't need to write them to disk.

If a WebActivity returns a Blob object, we should be able to wrap a File object around that Blob and set the .name to something like "blob". Wrapping a File around a Blob can be done in JS using "new window.File(blob, { name: 'blob', type: blob.type })" and in C++ by calling [1].

I do realize that the current nsIFilePicker API is making it tricky to not write the file to disk though. Could we change the <input type=file> code to use a better designed, asynchronous, interface and have the implementation of that interface use nsIFilePicker on existing platforms, and use something else on B2G?

We really should change the current <input type=file> code such that it doesn't require a synchronous call to the system filepicker API from the main thread. And I bet that it'd be nice to avoid going through nsIFilePicker on Android too.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMBlobBuilder.h#20
Attached patch patch (obsolete) — — Splinter Review
In this patch I followed the Jonas' comment.
I have implemented a new nsIFilePicker2 that inherits nsIFilePicker: it has this attribute:

readonly attribute nsIDOMFile domfile;

Then, only for B2G nsHTMLInputElement uses this new interface.
Attachment #723559 - Attachment is obsolete: true
Attachment #723559 - Flags: review?(mounir)
Attachment #723910 - Flags: review?(mounir)
I think the first part of this bug should be about doing the backout of the bug disabling <input type='file'>.
Attachment #724222 - Flags: review?(jonas)
Comment on attachment 723910 [details] [diff] [review]
patch

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

If I understand correctly, the reason why you are creating a nsIFilePicker2 is because you want a nsIDOMFile instead of a nsIFile as an output of the nsIFilePicker? Given that we likely have code depending on nsIFile, you could probably add a .domFile as an attribute for nsIFilePicker. This could easily be added to nsIFilePicker (no need for a new interface) given that .domFile would always return something if .file was returning something. The other way isn't true because nsIDOMFile can live on top of a Blob.

Also, one of my concerns with nsIFilePicker on B2G is what should we do when we have a file picker running with application=b2g and widget=gtk? Should we show the GTK file picker or the Activity-based one? I guess it's more a decision that has to be taken by Gonk/B2G people.

I think someone will have to double-check shell.js and FilePicker.js changes.

::: b2g/app/b2g.js
@@ +651,5 @@
>  // this fifo to trigger about:memory dumps, among other things.
>  pref("memory_info_dumper.watch_fifo.enabled", true);
>  pref("memory_info_dumper.watch_fifo.directory", "/data/local");
>  
> +pref("dom.disable_input_file", false);

I just attached a patch that simply removes that pref. No need to worry about that.

::: b2g/components/FilePicker.js
@@ +43,5 @@
> +
> +  init: function(parent, title, mode) {
> +    this.mParent = parent;
> +
> +    if (mode != Ci.nsIFilePicker.modeOpen) {

We should definitely handle .modeOpenMultiple.

@@ +62,5 @@
> +    // Ci.nsIFilePicker.filterXML
> +    // Ci.nsIFilePicker.filterXUL
> +    // Ci.nsIFilePicker.filterAllowURLs
> +    // Ci.nsIFilePicker.filterApps
> +    // Ci.nsIFilePicker.filterAudio

I would prefer if we could support all of those.

@@ +67,5 @@
> +
> +    if (filterMask & Ci.nsIFilePicker.filterImages) {
> +      this.mFilters.push('image/jpeg');
> +      this.mFilters.push('image/png');
> +      this.mFilters.push('image/gif');

Can't you get the pre-defined string and split it to add it to the array here?

@@ +73,5 @@
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterVideo) {
> +      this.mFilters.push('video/webm');
> +      this.mFilters.push('video/mp4');

ditto

@@ +84,5 @@
> +  },
> +
> +  appendFilter: function(title, extensions) {
> +    // Title is ignored
> +    this.mFilters.push(extensions);

Did you take into account that the current behaviour of nsIFilePicker when you have multiple filter{,s} is to have an array of filters that will all have a name and some extensions associated. Those filters will also have an id and you will be able to switch from one to another. You can just keep adding stuff.

Our Android implementation uses appendFilter as an extension filter and appendFilters as a mime type filter. Extension filters can be combined but you can only use one mime type filter which mean .appendFilters('image/*'); .appendFilters('audio/*'); will actually filter on 'audio/*'. Did you consider that solution?

@@ +130,5 @@
> +      return;
> +    }
> +
> +    let file = new this.mParent.File(data.result.blob,
> +                                     { name: 'filepicked' + this.getExtension(data.result.blob),

I am not a big fan of the file name. It's probably not a big deal though. Actually, I think that we shouldn't show file names on mobile.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1900,5 @@
>    aValue.Truncate();
>    for (int32_t i = 0; i < mFiles.Count(); ++i) {
>      nsString str;
> +#if MOZ_B2G
> +    mFiles[i]->GetName(str);

You don't need that.
Attachment #723910 - Flags: review?(mounir) → review-
(In reply to Mounir Lamouri (:mounir) from comment #29)
> Comment on attachment 723910 [details] [diff] [review]
> patch

> Also, one of my concerns with nsIFilePicker on B2G is what should we do when
> we have a file picker running with application=b2g and widget=gtk? Should we
> show the GTK file picker or the Activity-based one? I guess it's more a
> decision that has to be taken by Gonk/B2G people.

I think we should always use the activity-based one when running with MOZ_B2G. I agree that we currently lack a real filepicker, but that could be implemented as a filemanager app using the device storage API.
> I think the first part of this bug should be about doing the backout of the
> bug disabling <input type='file'>.

This patch doesn't apply on m-c. Is it for b2g18 or what?
> I am not a big fan of the file name. It's probably not a big deal though.
> Actually, I think that we shouldn't show file names on mobile.
> 
> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +1900,5 @@
> >    aValue.Truncate();
> >    for (int32_t i = 0; i < mFiles.Count(); ++i) {
> >      nsString str;
> > +#if MOZ_B2G
> > +    mFiles[i]->GetName(str);
> 
> You don't need that.

If we don't do this, the input will be empty and it breaks the user experience:
you select a file, but the input field remains empty... this is wrong.

With this line you will see the file name, so the user sees that the input as been populated.
If we don't like the name, we can change it, but I think it's important to have something written into the input field.
> If I understand correctly, the reason why you are creating a nsIFilePicker2
> is because you want a nsIDOMFile instead of a nsIFile as an output of the
> nsIFilePicker? Given that we likely have code depending on nsIFile, you
> could probably add a .domFile as an attribute for nsIFilePicker. This could
> easily be added to nsIFilePicker (no need for a new interface) given that
> .domFile would always return something if .file was returning something. The
> other way isn't true because nsIDOMFile can live on top of a Blob.

This is right. But adding a new method to nsIFilePicker means that we have to implement this method in the cocoa, qt, gtk2, winrt, window, metro, xpwidgets and os2. This is probably too much.

I like the Jonas' comment - comment 26:

... interface and have the implementation of that interface use nsIFilePicker on existing platforms, and use something else on B2G?
Attached patch patch (obsolete) — — Splinter Review
This patch fixes all the comments except the domfile() in nsIFilePicker.
I would like to see a Jonas' comment before proceeding in that way.
Attachment #723910 - Attachment is obsolete: true
Attachment #724398 - Flags: review?(mounir)
Attached patch patch b (obsolete) — — Splinter Review
This patch adds domfile() and domfiles() to nsIFilePicker. I'm waiting for some result on try.
Attachment #724398 - Attachment is obsolete: true
Attachment #724398 - Flags: review?(mounir)
Attachment #724487 - Flags: review?(mounir)
Comment on attachment 724487 [details] [diff] [review]
patch b

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

Sorry, I wasn't that much available today to chat about the patch. Hopefully, I should be more available tomorrow. So feel free to ping me.

::: b2g/components/FilePicker.js
@@ +43,5 @@
> +                     'application/rdf+xml', 'application/rss+xml',
> +                     'application/soap+xml', 'application/font-woff',
> +                     'application/xhtml+xml', 'application/xml',
> +                     'application/xml-dtd', 'application/xop+xml',
> +                     'application/zip', 'application/gzip'];

Can't you do like the toolkit's filepicker.js and use the bundles?

@@ +72,5 @@
> +    this.mParent = parent;
> +
> +    if (mode != Ci.nsIFilePicker.modeOpen &&
> +        mode != Ci.nsIFilePicker.modeOpenMultiple) {
> +      throw 'Only mode \'modeOpen\' is supported on B2G';

Couldn't you throw a NS_ERROR_NOT_IMPLEMENTED?

@@ +78,5 @@
> +  },
> +
> +  /* readonly attribute nsILocalFile file - not implemented; */
> +  /* readonly attribute nsISimpleEnumerator files - not implemented; */
> +  /* readonly attribute nsIURI fileURL - not implemented; */

ditto for those (or is that working for free?)

@@ +188,5 @@
> +      this.fireError();
> +      return;
> +    }
> +
> +    let name = 'filepicked' + this.getExtension(data.result.blob);

The name of the file should be 'blob'. No need to have a fake name.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +277,4 @@
>      while (NS_SUCCEEDED(iter->HasMoreElements(&loop)) && loop) {
>        iter->GetNext(getter_AddRefs(tmp));
> +      nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(tmp);
> +      if (!domFile) {

This should simply not happen. You should MOZ_ASSERT in that case.

@@ +1893,5 @@
>    for (int32_t i = 0; i < mFiles.Count(); ++i) {
>      nsString str;
>      mFiles[i]->GetMozFullPathInternal(str);
> +    if (str.IsEmpty()) {
> +      mFiles[i]->GetName(str);

Argh, I have a patch for <input type='file'> that could make that change useless but the patch is being blocked by another patch :(

::: widget/nsIFilePicker.idl
@@ +144,5 @@
>  
>   /**
> +  * Get the nsIDOMFile for the file.
> +  *
> +  * @return Returns the file currently selected

... as a DOMFile.

@@ +152,5 @@
> + /**
> +  * Get the enumerator for the selected files
> +  * only works in the modeOpenMultiple mode
> +  *
> +  * @return Returns the files currently selected

... as DOMFiles.

::: widget/xpwidgets/nsBaseFilePicker.cpp
@@ +253,5 @@
> +  nsCOMPtr<nsIFile> localFile;
> +  nsresult rv = GetFile(getter_AddRefs(localFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsRefPtr<nsDOMFileFile> domFile = new nsDOMFileFile(localFile);

Should you check that localFile is null before and simply return nullptr in that case?

@@ +279,5 @@
> +NS_IMETHODIMP
> +nsBaseFilePicker::GetDomfiles(nsISimpleEnumerator** aDomfiles)
> +{
> +  nsCOMPtr<nsISimpleEnumerator> iter;
> +  nsresult rv = GetDomfiles(getter_AddRefs(iter));

Isn't that going to do an infinite recursive call?

Shouldn't you be calling GetFiles(), convert those to nsIDOMFile and use NS_NewArrayEnumerator to create the iterator?

::: widget/xpwidgets/nsBaseFilePicker.h
@@ +38,2 @@
>  #endif
> +  NS_IMETHOD GetAddToRecentDocs(bool* aFlag);

For some reasons your editor changes a lot of lines here. Please, revert that.
Attachment #724487 - Flags: review?(mounir) → review-
(In reply to Andrea Marchesini (:baku) from comment #31)
> > I think the first part of this bug should be about doing the backout of the
> > bug disabling <input type='file'>.
> 
> This patch doesn't apply on m-c. Is it for b2g18 or what?

It was applying on b2g18 last time I checked. Given that I wanted to get that on B2G ASAP I wrote the patch on b2g18. Getting it apply in m-c should be a detail.
If the activity returns a Blob and you have to wrap a File around it, simply set the .name of the file to the string "blob". This matches what the FormData interface does when handed a Blob rather than a File.
I'll defer to Robert O'Callahan regarding if we should add an nsIFilePicker2 or not. He's the owner of widget/
> > +  /* readonly attribute nsILocalFile file - not implemented; */
> > +  /* readonly attribute nsISimpleEnumerator files - not implemented; */
> > +  /* readonly attribute nsIURI fileURL - not implemented; */
> 
> ditto for those (or is that working for free?)

It works for free.

> The name of the file should be 'blob'. No need to have a fake name.

What about comment 19 ?

> >    for (int32_t i = 0; i < mFiles.Count(); ++i) {
> >      nsString str;
> >      mFiles[i]->GetMozFullPathInternal(str);
> > +    if (str.IsEmpty()) {
> > +      mFiles[i]->GetName(str);
> 
> Argh, I have a patch for <input type='file'> that could make that change
> useless but the patch is being blocked by another patch :(

... ok. How do we want to proceed?

> Shouldn't you be calling GetFiles(), convert those to nsIDOMFile and use
> NS_NewArrayEnumerator to create the iterator?

Doing this I do it only when needed... but yes, it was an infinite loop. Hard to test the same patch for 2 builds (b2g and desktop).

> For some reasons your editor changes a lot of lines here. Please, revert
> that.

emm... no. I converted any |foo *bar| to |foo* bar|. But I can revert it :)
Attached patch patch c (obsolete) — — Splinter Review
Attachment #724487 - Attachment is obsolete: true
Attachment #724930 - Flags: review?(mounir)
(In reply to Andrea Marchesini (:baku) from comment #41)
> > The name of the file should be 'blob'. No need to have a fake name.
> 
> What about comment 19 ?

I still think we should simply use the name "blob". We can do something more complicated if we actually see websites having trouble with that.
Comment on attachment 724222 [details] [diff] [review]
b2g18 part 1 - re enable input file

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

I'll let Mounir do the reviewing here.
Attachment #724222 - Flags: review?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #43)
> (In reply to Andrea Marchesini (:baku) from comment #41)
> > > The name of the file should be 'blob'. No need to have a fake name.
> > 
> > What about comment 19 ?
> 
> I still think we should simply use the name "blob". We can do something more
> complicated if we actually see websites having trouble with that.

Just "blob" with no extension will work for some upload workflows in MediaWiki, but will *not* work with the primary upload mechanism we have on https://commons.wikimedia.org/ (Wikipedia's central media repo) -- an extensionless JPEG file is rejected with "This wiki requires that files have an extension — like ".JPG" at the end of the filename. The allowed extensions are: <big list>".

It will also not work with WordPress, which rejects an extensionless JPEG file with "Sorry, this file type is not permitted for security reasons."
Ugh, ok, I guess we should use the mime service to append an approximate mimetype to "blob" then, so that we get something like "blob.jpg".
Attached patch patch d (obsolete) — — Splinter Review
1. Blob + extension - using mime service
2. specialpowers/content/MockFilePicker.jsm updated
Attachment #724930 - Attachment is obsolete: true
Attachment #724930 - Flags: review?(mounir)
Attachment #725312 - Flags: review?(mounir)
Comment on attachment 725312 [details] [diff] [review]
patch d

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

::: b2g/components/FilePicker.js
@@ +41,5 @@
> +
> +  /* members */
> +
> +  mParent: undefined,
> +  mFilterType: [],

nit: mFilterTypes

@@ +69,5 @@
> +    return this.mFilesEnumerator ? this.mFilesEnumerator.mFiles[0] : null;
> +  },
> +
> +  appendFilters: function(filterMask) {
> +    this.mFilterType = null;

So, you change this method to no longer use file extensions but mime types? In that case, there is no reason to use the properties file.

@@ +72,5 @@
> +  appendFilters: function(filterMask) {
> +    this.mFilterType = null;
> +
> +    if (filterMask & Ci.nsIFilePicker.filterHTML) {
> +      this.mFilterType = filterBundle.GetStringFromName("htmlMime").split(';');

Just set text/html.

@@ +76,5 @@
> +      this.mFilterType = filterBundle.GetStringFromName("htmlMime").split(';');
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterText) {
> +      this.mFilterType = filterBundle.GetStringFromName("textMime").split(';');

text/plain?

@@ +80,5 @@
> +      this.mFilterType = filterBundle.GetStringFromName("textMime").split(';');
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterImages) {
> +      this.mFilterType = filterBundle.GetStringFromName("imageMime").split(';');

You should use "image/*".

@@ +84,5 @@
> +      this.mFilterType = filterBundle.GetStringFromName("imageMime").split(';');
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterXML) {
> +      this.mFilterType = filterBundle.GetStringFromName("xmlMime").split(';');

Just set the correct mime type.

@@ +88,5 @@
> +      this.mFilterType = filterBundle.GetStringFromName("xmlMime").split(';');
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterXUL) {
> +      this.mFilterType = filterBundle.GetStringFromName("xulMime").split(';');

Use the mime type.

@@ +92,5 @@
> +      this.mFilterType = filterBundle.GetStringFromName("xulMime").split(';');
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterApps) {
> +      this.mFilterType = filterBundle.GetStringFromName("appMime").split(';');

I doubt B2G really supports that...

@@ +98,5 @@
> +
> +    // FIXME: Ci.nsIFilePicker.filterAllowURLs
> +
> +    if (filterMask & Ci.nsIFilePicker.filterVideo) {
> +      this.mFilterType = filterBundle.GetStringFromName("videoMime").split(';');

video/*

@@ +102,5 @@
> +      this.mFilterType = filterBundle.GetStringFromName("videoMime").split(';');
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterAudio) {
> +      this.mFilterType = filterBundle.GetStringFromName("audioMime").split(';');

audio/*

@@ +128,5 @@
> +
> +  fireSuccess: function(file) {
> +    this.mFilesEnumerator = {
> +      mFiles: [file],
> +      mIndex: 0,

Shouldn't you include the QI information?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +290,5 @@
> +
> +        nsCOMPtr<nsIFile> localFile;
> +        rv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(path), true,
> +                                   getter_AddRefs(localFile));
> +        NS_ENSURE_SUCCESS(rv, rv);

Could you pass the domFile to StoreLastUsedDirectory? and do the rest inside that method? This method is called in two places and both are doing a lot of work before calling it because they don't have the correct object. I think it would be better to simply do the work in the method and change the parameter.

@@ +312,5 @@
> +      if (NS_SUCCEEDED(rv) && !path.IsEmpty()) {
> +        nsCOMPtr<nsIFile> localFile;
> +        rv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(path), true,
> +                                   getter_AddRefs(localFile));
> +        NS_ENSURE_SUCCESS(rv, rv);

ditto

::: testing/specialpowers/content/MockFilePicker.jsm
@@ +143,5 @@
>      };
>    },
> +  get domfiles()  {
> +    let utils = this.parent.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDOMWindowUtils);

nit: indentation

::: toolkit/components/filepicker/nsFilePicker.js
@@ +111,5 @@
> +
> +    if (!this.mDOMFilesEnumerator) {
> +      this.mDOMFilesEnumerator {
> +        mFiles: [],
> +        mIndex: 0,

You should add the QI information.

@@ +114,5 @@
> +        mFiles: [],
> +        mIndex: 0,
> +
> +        hasMoreElements: function() {
> +          return (this.mIndex < this.mFiles.length);

nit: parenthesis are not needed.

::: toolkit/content/filepicker.properties
@@ +18,5 @@
> +htmlMime=text/html
> +textMime=text/cmd;text/css;text/csv;text/html;text/plain;text/vcard;text/xml
> +xmlMime=text/xml
> +xulMime=application/vnd.mozilla.xul+xml
> +appMime=application/atom+xml;application/ecmascript;application/EDI-X12;application/EDIFACT;application/json;application/javascript;application/octet-stream;application/ogg;application/pdf;application/postscript;application/rdf+xml;application/rss+xml;application/soap+xml;application/font-woff;application/xhtml+xml;application/xml;application/xml-dtd;application/xop+xml;application/zip;application/gzip

No need to populate this properties file with mime types.

::: widget/xpwidgets/nsBaseFilePicker.cpp
@@ +263,5 @@
> +  domFile.forget(aDomfile);
> +  return NS_OK;
> +}
> +
> +class nsBaseFilePickerEnumerator : public nsISimpleEnumerator

Could you move that on top of the file, after AsyncShowFilePicker and have the definition of the methods inlined.
Attachment #725312 - Flags: review?(mounir) → review-
> > +    this.mFilterType = null;
> 
> So, you change this method to no longer use file extensions but mime types?
> In that case, there is no reason to use the properties file.

I never used extensions for this method. I have always used mime types.
And you suggested to use the properties file... and now you... changed your mind? :)


> > +    if (filterMask & Ci.nsIFilePicker.filterImages) {
> > +      this.mFilterType = filterBundle.GetStringFromName("imageMime").split(';');
> 
> You should use "image/*".

No. Unfortunately for the pick activity needs the exact mime type. Regexp or image/* are not supported.
This is the reason why I used the properties file. Here we have to use the full list of acceptable mime types.

> 
> @@ +128,5 @@
> > +
> > +  fireSuccess: function(file) {
> > +    this.mFilesEnumerator = {
> > +      mFiles: [file],
> > +      mIndex: 0,
> 
> Shouldn't you include the QI information?

No. It's not needed. It works without QI information.

> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +290,5 @@
> > +
> > +        nsCOMPtr<nsIFile> localFile;
> > +        rv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(path), true,
> > +                                   getter_AddRefs(localFile));
> > +        NS_ENSURE_SUCCESS(rv, rv);
> 
> Could you pass the domFile to StoreLastUsedDirectory? and do the rest inside
> that method? This method is called in two places and both are doing a lot of
> work before calling it because they don't have the correct object. I think
> it would be better to simply do the work in the method and change the
> parameter.
> 
> @@ +312,5 @@
> > +      if (NS_SUCCEEDED(rv) && !path.IsEmpty()) {
> > +        nsCOMPtr<nsIFile> localFile;
> > +        rv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(path), true,
> > +                                   getter_AddRefs(localFile));
> > +        NS_ENSURE_SUCCESS(rv, rv);
> 
> ditto

> ::: toolkit/components/filepicker/nsFilePicker.js
> @@ +111,5 @@
> > +
> > +    if (!this.mDOMFilesEnumerator) {
> > +      this.mDOMFilesEnumerator {
> > +        mFiles: [],
> > +        mIndex: 0,
> 
> You should add the QI information.

Not needed. This is has been tested.

I'm going to submit a new patch with these small nit, but the mime type will be kept as it is.
Attached patch patch e (obsolete) — — Splinter Review
No file properties but we move back to arrays in FilePicker.js
Attachment #725312 - Attachment is obsolete: true
Attachment #725722 - Flags: review?(mounir)
Comment on attachment 725722 [details] [diff] [review]
patch e

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

r=me with the comments applied but I would like to get a B2G person (like Fabrice) to review the B2G specific bits.

Also, we should open two follow-ups that we should work on to have this feature really done:
- <input type='file' multiple> should work for real. For the moment, the pick activity doesn't handle that but we should make this possible;
- <input type='file' accept='mime/type'> should work on Firefox OS like it is working on Firefox Desktop/Mobile.

Also, another follow-up would be to allow stuff like "image/*", "audio/*" and "video/*" to work with the pick activity. This is essential to have a sane "API".

::: b2g/components/FilePicker.js
@@ +18,5 @@
> + */
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +// FIXME: improve this list of filters.

The good practice here is to have something like:
// TODO: improve this list of filters, see bug 1234567.

@@ +43,5 @@
> +                     'application/rdf+xml', 'application/rss+xml',
> +                     'application/soap+xml', 'application/font-woff',
> +                     'application/xhtml+xml', 'application/xml',
> +                     'application/xml-dtd', 'application/xop+xml',
> +                     'application/zip', 'application/gzip'];

I think we should only support IMAGE_FILTERS, VIDEO_FILTERS and AUDIO_FILTERS for the moment because those things are used by content. All other filters are chrome specifics (aka used by Firefox). Firefox OS will never use those for the moment.

@@ +115,5 @@
> +    if (filterMask & Ci.nsIFilePicker.filterApps) {
> +      this.mFilterTypes = APP_FILTERS;
> +    }
> +
> +    // FIXME: Ci.nsIFilePicker.filterAllowURLs

Don't do a FIXME, add a TODO with a bug number if we expect to fix this someday. Otherwise, just say that this is not supported.

@@ +122,5 @@
> +      this.mFilterTypes = VIDEO_FILTERS;
> +    }
> +
> +    if (filterMask & Ci.nsIFilePicker.filterAudio) {
> +      this.mFilterTypes = AUDIO_FILTERS;

You should be fine with onle VIDEO, AUDIO and IMAGE.

@@ +149,5 @@
> +  fireSuccess: function(file) {
> +    this.mFilesEnumerator = {
> +      mFiles: [file],
> +      mIndex: 0,
> +

XPConnect might make this work but you should still add:
QueryInterface: XPCOMUtils.generateQI([Ci.nsISimpleEnumerator]),

@@ +189,5 @@
> +      return;
> +    }
> +
> +    var mimeSvc = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService);
> +    var mimeInfo = mimeSvc.getFromTypeAndExtension(data.result.blob.type, null);

Don't use null but "".

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +490,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +UploadLastDir::StoreLastUsedDirectory(nsIDocument* aDoc, nsIDOMFile* aDomFile)

Thanks for doing that change :)

@@ +1883,5 @@
>      nsString str;
>      mFiles[i]->GetMozFullPathInternal(str);
> +    if (str.IsEmpty()) {
> +      mFiles[i]->GetName(str);
> +    }

I guess I will have to rebase my patch queue :'(

::: testing/specialpowers/content/MockFilePicker.jsm
@@ +143,5 @@
>      };
>    },
> +  get domfiles()  {
> +    let utils = this.parent.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDOMWindowUtils);

nit: indentation

::: toolkit/components/filepicker/nsFilePicker.js
@@ +112,5 @@
> +    if (!this.mDOMFilesEnumerator) {
> +      this.mDOMFilesEnumerator {
> +        mFiles: [],
> +        mIndex: 0,
> +

XPConnect might do some magic to make this work without QueryInterface being set (because object looks like a nsISimpleEnumerator) but you should still add:
QueryInterface: XPCOMUtils.generateQI([Ci.nsISimpleEnumerator]),

::: widget/xpwidgets/nsBaseFilePicker.cpp
@@ +263,5 @@
> +  domFile.forget(aDomfile);
> +  return NS_OK;
> +}
> +
> +class nsBaseFilePickerEnumerator : public nsISimpleEnumerator

Could you get that class defined at the top of the file with the method being inlined so it is easier to read (because this class and nsBaseFilePicker have very similar names, it can be confusing).
Attachment #725722 - Flags: review?(mounir)
Attachment #725722 - Flags: review?(fabrice)
Attachment #725722 - Flags: review+
Blocks: 852203
Blocks: 852204
Attached patch patch (obsolete) — — Splinter Review
Attachment #725722 - Attachment is obsolete: true
Attachment #725722 - Flags: review?(fabrice)
Attachment #726229 - Flags: review?(fabrice)
Comment on attachment 726229 [details] [diff] [review]
patch

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

r=me with nits addressed. I did not review the c++ parts.

::: b2g/chrome/content/shell.js
@@ +532,5 @@
>      });
>    },
>  
>    receiveMessage: function shell_receiveMessage(message) {
> +    var activities = { 'content-handler': { activity: 'view', response: null },

nit: if you use "name" instead of "avitviy" there, we won't have the activity.activity strangeness later.

::: b2g/components/Makefile.in
@@ +22,5 @@
>          B2GComponents.manifest \
>          ContentHandler.js \
>          ContentPermissionPrompt.js \
>          DirectoryProvider.js \
> +        FilePicker.js \

We could do that cleanup in a followup, but there's no reason for this component to be pre-processed (that's also true for a bunch of others).

::: testing/specialpowers/content/MockFilePicker.jsm
@@ +143,5 @@
>      };
>    },
> +  get domfiles()  {
> +    let utils = this.parent.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDOMWindowUtils);

nit: indentation

::: toolkit/components/filepicker/nsFilePicker.js
@@ +95,5 @@
>    set files(a) { throw "readonly property"; },
>    get files()  { return this.mFilesEnumerator; },
>  
> +  /* readonly attribute nsIDOMFile domfile; */
> +  set domfile(a) { throw "readonly property"; },

I think xpconnect gives you that for free.

@@ +102,5 @@
> +    return enumerator ? enumerator.mFiles[0] : null;
> +  },
> +
> +  /* readonly attribute nsISimpleEnumerator domfiles; */
> +  set domfiles(a) { throw "readonly property"; },

ditto

::: widget/xpwidgets/nsBaseFilePicker.h
@@ +34,2 @@
>    NS_IMETHOD GetFiles(nsISimpleEnumerator **aFiles);
>  #ifdef BASEFILEPICKER_HAS_DISPLAYDIRECTORY 

nit: time to kill this trailing ws
Attachment #726229 - Flags: review?(fabrice) → review+
Attachment #726229 - Attachment is obsolete: true
Attachment #726579 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
I just rebased your patch. How can review it?
Attachment #724222 - Attachment is obsolete: true
Attachment #726594 - Flags: review?(mounir)
Depends on: 690659
We need part of the patch for 690659 in b2g18.

This commit: https://hg.mozilla.org/mozilla-central/rev/9f4e7e70cd69

But we cannot import all the patch for that bug easily... can we just import this?
Flags: needinfo?(jonas)
Attachment #726594 - Attachment description: part 1 - re enable input file → m-c part 1 - re enable input file
Attachment #726579 - Attachment description: patch → m-c part 2 - input type=file for b2g
Attachment #724222 - Attachment description: Re-enable <input type='file'> on B2G → b2g18 part 1 - re enable input file
Attachment #724222 - Attachment is obsolete: false
Attachment #726640 - Flags: review+
This is the part we need in b2g18 in order to allow the creation of DOM File from a DOM Blob. This patch is taken from https://hg.mozilla.org/mozilla-central/rev/9f4e7e70cd69 and it has been already reviewed by sicking.
Attachment #726644 - Flags: review?(mounir)
Just make sure to give credit to the person who wrote the original patch in the checkin comment.
Flags: needinfo?(jonas)
Attachment #726594 - Flags: review?(mounir) → review+
Attachment #724222 - Flags: review+
Comment on attachment 724222 [details] [diff] [review]
b2g18 part 1 - re enable input file

[Approval Request Comment]
User impact if declined: No file picker is available on B2G
Testing completed: Yes. We have mochitests on gecko. Nothing on b2g
Risk to taking this patch (and alternatives if risky): I don't see big risks.
Attachment #724222 - Flags: approval-mozilla-b2g18?
Comment on attachment 726640 [details] [diff] [review]
b2g18 part 2 - input type=file for b2g

[Approval Request Comment]
User impact if declined: No file picker is available on B2G
Testing completed: Yes. We have mochitests on gecko. Nothing on b2g
Risk to taking this patch (and alternatives if risky): I don't see big risks.
Attachment #726640 - Flags: approval-mozilla-b2g18?
Comment on attachment 726644 [details] [diff] [review]
b2g18 part 3 - from DOM Blob to DOM File

[Approval Request Comment]
User impact if declined: No file picker is available on B2G
Testing completed: Yes. We have mochitests on gecko. Nothing on b2g
Risk to taking this patch (and alternatives if risky): I don't see big risks.
Attachment #726644 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/e19e707704ce
https://hg.mozilla.org/mozilla-central/rev/18ccc35f8880
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Now that we're past our FC, let's hold this functionality till the next release rather than take risk of new regressions in 1.1
Whiteboard: [triaged:1/22] → [triaged:1/22][holding for v1.2]
Mounir made a good point when he and I were discussing this earlier:  without these patches, websites that use input type=file will just plain not work.  If we take this, it may have bugs, but at least these websites won't be missing the functionality they expect.

I don't know if that sways any inclusion decisions but I thought it was worth thinking about.
From a compatibility perspective, this is a high value patch. Facebook and Twitter are taking hits without this working on Firefox OS. So I'm not sure I totally agree out right "we're past the date so we don't take this" is a valid reason to not take this. We have technically moved that date, anyways.

Bumping to product for a judgment call. But I think this is important enough that we should take this into v1.1.
Flags: needinfo?(clee)
Whiteboard: [triaged:1/22][holding for v1.2] → [triaged:1/22]
Thanks for looping me in.

Jason, what are the hits Facebook and Twitter are taking by not taking this patch?  I'd like understand specific examples of what we gain by taking this.
Flags: needinfo?(clee)
Without this fix you cannot upload pictures to the Facebook or Twitter apps. Note that this functionality is currently disabled in the Facebook app. We will need to work with Facebook to enable it.
Per the email thread on this, the decision is to uplift this to v1.1 per product.
(In reply to Jason Smith [:jsmith] from comment #70)
> Per the email thread on this, the decision is to uplift this to v1.1 per
> product.

We should of course note the risk evaluation provided by engineering that we don't expect there to be significant regressions from this (famous last words ;).
Attachment #724222 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Attachment #726640 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Attachment #726644 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
QA Contact: jsmith
Flags: in-moztrap?(jsmith)
Depends on: 854102
Depends on: 856001
Depends on: 834773
I've confirmed this fixes mobile photo uploads on Wikipedia on my unagi phone as of build 20130328070204. Thanks everybody!
Keywords: verifyme
Depends on: 880042
Finally got the chance to test this and verify this. Looks okay overall noting the following issues:

- Lack of support for multiple - bug 852203
- Lack of support for mime type - bug 852203
- Broken experience on the Gaia side for Communications using the pick activity - bug 858851 & bug 883341
- Although parity is met here with desktop, I noticed that my video & audio filter and image & video filter test cases didn't apply the filters I specified - need input if this is a bug in my test case or an actual DOM bug

Test case to test this was http://mozilla.github.io/qa-testcase-data/webapi/filemanagement/fileUploadTest.html.

Mounir - Why doesn't the video & audio filter and image & video filter test cases not work on all platforms? Am I specifying something incorrectly here or is this a bug?
Flags: needinfo?(mounir)
Flags: in-moztrap?(jsmith)
Flags: in-moztrap-
Keywords: verifyme
Filters should be separated with a comma, not a pipe.
Also, we only support {image,audio,video}/* consistently cross platform. Other filters are kind of flaky.
Flags: needinfo?(mounir)
(In reply to Mounir Lamouri (:mounir) from comment #75)
> Filters should be separated with a comma, not a pipe.
> Also, we only support {image,audio,video}/* consistently cross platform.
> Other filters are kind of flaky.

Ah okay. Updated the test case.

Looks like I found a bug then - if I specify multiple filters, the last filter is only applied, not multiple. I'll file a followup.
Blocks: 884878
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.