Last Comment Bug 832923 - Implement <input type='file'> on B2G
: Implement <input type='file'> on B2G
Status: RESOLVED FIXED
[triaged:1/22]
: addon-compat, compat, dev-doc-needed, feature
Product: Core
Classification: Components
Component: Widget: Gonk (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Andrea Marchesini [:baku]
: Jason Smith [:jsmith]
:
Mentors:
: 845034 (view as bug list)
Depends on: 1100967 690659 782766 801635 805292 834773 854102 856001 880042
Blocks: twitter.com 852203 852204 709742 804311 824313 884878
  Show dependency treegraph
 
Reported: 2013-01-21 02:55 PST by Mounir Lamouri (:mounir)
Modified: 2014-11-18 06:54 PST (History)
46 users (show)
jsmith: in‑moztrap-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
+
verified
wontfix
wontfix


Attachments
patch (13.31 KB, patch)
2013-03-11 09:34 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch (13.31 KB, patch)
2013-03-11 11:14 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch (16.16 KB, patch)
2013-03-12 06:25 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
b2g18 part 1 - re enable input file (9.43 KB, patch)
2013-03-12 17:35 PDT, Mounir Lamouri (:mounir)
amarchesini: review+
akeybl: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review
patch (19.25 KB, patch)
2013-03-13 06:54 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch b (26.48 KB, patch)
2013-03-13 10:24 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch c (28.68 KB, patch)
2013-03-14 08:43 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch d (28.23 KB, patch)
2013-03-15 01:44 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch e (29.11 KB, patch)
2013-03-15 22:59 PDT, Andrea Marchesini [:baku]
mounir: review+
Details | Diff | Splinter Review
patch (28.34 KB, patch)
2013-03-18 10:43 PDT, Andrea Marchesini [:baku]
fabrice: review+
Details | Diff | Splinter Review
m-c part 2 - input type=file for b2g (30.00 KB, patch)
2013-03-19 04:31 PDT, Andrea Marchesini [:baku]
amarchesini: review+
Details | Diff | Splinter Review
m-c part 1 - re enable input file (4.40 KB, patch)
2013-03-19 05:18 PDT, Andrea Marchesini [:baku]
amarchesini: review+
Details | Diff | Splinter Review
b2g18 part 2 - input type=file for b2g (29.92 KB, patch)
2013-03-19 06:52 PDT, Andrea Marchesini [:baku]
amarchesini: review+
akeybl: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review
b2g18 part 3 - from DOM Blob to DOM File (2.42 KB, patch)
2013-03-19 06:54 PDT, Andrea Marchesini [:baku]
jonas: review+
akeybl: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2013-01-21 02:55:26 PST
+++ 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.
Comment 1 Joe Cheng [:jcheng] 2013-01-21 22:57:59 PST
Triage: leo? shira is to be as close to tef as possible
Comment 2 Dietrich Ayala (:dietrich) 2013-02-04 16:41:29 PST
There's no product or Web standards definition for what this would actually do. Not holding the 1.1 release for this.
Comment 3 testit 2013-02-05 00:25:36 PST
(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.
Comment 4 Mounir Lamouri (:mounir) 2013-02-05 02:14:24 PST
(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?
Comment 5 [:fabrice] Fabrice Desré 2013-02-05 08:01:32 PST
(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.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-02-06 20:27:22 PST
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?
Comment 7 David Baron :dbaron: ⌚️UTC-10 2013-02-07 20:35:55 PST
(In reply to Jonas Sicking (:sicking) from comment #6)
> 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.

It's older than that:
http://tools.ietf.org/html/draft-ietf-html-fileupload-00 (November 18, 1994)
http://www.rfc-editor.org/rfc/rfc1867.txt (November 1995)
http://www.w3.org/TR/REC-html32-19970114#input
http://www.w3.org/TR/REC-html40-971218/interact/forms.html#input-control-types
http://www.w3.org/TR/2012/CR-html5-20121217/forms.html#file-upload-state-%28type=file%29
http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#file-upload-state-%28type=file%29
Comment 8 Mounir Lamouri (:mounir) 2013-02-26 06:24:48 PST
*** Bug 845034 has been marked as a duplicate of this bug. ***
Comment 9 Brion Vibber 2013-02-27 11:17:56 PST
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.
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-02-27 11:36:16 PST
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)
Comment 11 Brion Vibber 2013-02-27 13:31:08 PST
(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.
Comment 12 Brion Vibber 2013-02-27 14:28:53 PST
(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.
Comment 13 Mounir Lamouri (:mounir) 2013-02-28 15:07:08 PST
I will work on this next week.
Comment 14 Mounir Lamouri (:mounir) 2013-03-08 04:23:56 PST
We are getting compat' issues with this. Shouldn't we block?
Comment 15 [:fabrice] Fabrice Desré 2013-03-08 07:45:56 PST
(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!
Comment 16 Jason Smith [:jsmith] 2013-03-08 08:03:41 PST
(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+.
Comment 17 Andrea Marchesini [:baku] 2013-03-11 09:34:53 PDT
Created attachment 723513 [details] [diff] [review]
patch

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.
Comment 18 [:fabrice] Fabrice Desré 2013-03-11 10:00:49 PDT
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?
Comment 19 Brion Vibber 2013-03-11 10:17:36 PDT
(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?
Comment 20 David Teller [:Yoric] (please use "needinfo") 2013-03-11 11:00:17 PDT
(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?
Comment 21 Andrea Marchesini [:baku] 2013-03-11 11:14:32 PDT
Created attachment 723559 [details] [diff] [review]
patch
Comment 22 Andrea Marchesini [:baku] 2013-03-11 11:19:18 PDT
> 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?
Comment 23 Andrea Marchesini [:baku] 2013-03-11 11:20:51 PDT
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.
Comment 24 Andrea Marchesini [:baku] 2013-03-11 11:26:11 PDT
> 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.
Comment 25 David Teller [:Yoric] (please use "needinfo") 2013-03-11 12:53:29 PDT
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.
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-03-12 02:03:47 PDT
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
Comment 27 Andrea Marchesini [:baku] 2013-03-12 06:25:28 PDT
Created attachment 723910 [details] [diff] [review]
patch

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.
Comment 28 Mounir Lamouri (:mounir) 2013-03-12 17:35:31 PDT
Created attachment 724222 [details] [diff] [review]
b2g18 part 1 - re enable input file

I think the first part of this bug should be about doing the backout of the bug disabling <input type='file'>.
Comment 29 Mounir Lamouri (:mounir) 2013-03-12 18:25:06 PDT
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.
Comment 30 [:fabrice] Fabrice Desré 2013-03-12 21:55:26 PDT
(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.
Comment 31 Andrea Marchesini [:baku] 2013-03-13 03:46:55 PDT
> 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?
Comment 32 Andrea Marchesini [:baku] 2013-03-13 04:28:26 PDT
> 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.
Comment 33 Andrea Marchesini [:baku] 2013-03-13 04:39:19 PDT
> 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?
Comment 34 Andrea Marchesini [:baku] 2013-03-13 06:54:45 PDT
Created attachment 724398 [details] [diff] [review]
patch

This patch fixes all the comments except the domfile() in nsIFilePicker.
I would like to see a Jonas' comment before proceeding in that way.
Comment 35 Andrea Marchesini [:baku] 2013-03-13 10:24:26 PDT
Created attachment 724487 [details] [diff] [review]
patch b

This patch adds domfile() and domfiles() to nsIFilePicker. I'm waiting for some result on try.
Comment 36 Andrea Marchesini [:baku] 2013-03-13 11:00:35 PDT
https://bugzilla.mozilla.org/show_bug.cgi?id=832923
Comment 37 Mounir Lamouri (:mounir) 2013-03-13 17:24:50 PDT
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.
Comment 38 Mounir Lamouri (:mounir) 2013-03-13 17:26:58 PDT
(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.
Comment 39 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-03-14 01:44:51 PDT
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.
Comment 40 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-03-14 01:46:54 PDT
I'll defer to Robert O'Callahan regarding if we should add an nsIFilePicker2 or not. He's the owner of widget/
Comment 41 Andrea Marchesini [:baku] 2013-03-14 08:42:39 PDT
> > +  /* 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 :)
Comment 42 Andrea Marchesini [:baku] 2013-03-14 08:43:14 PDT
Created attachment 724930 [details] [diff] [review]
patch c
Comment 43 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-03-14 17:25:15 PDT
(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 44 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-03-14 17:25:47 PDT
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.
Comment 45 Brion Vibber 2013-03-14 17:45:04 PDT
(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."
Comment 46 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-03-14 18:21:36 PDT
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".
Comment 47 Andrea Marchesini [:baku] 2013-03-15 01:44:58 PDT
Created attachment 725312 [details] [diff] [review]
patch d

1. Blob + extension - using mime service
2. specialpowers/content/MockFilePicker.jsm updated
Comment 48 Mounir Lamouri (:mounir) 2013-03-15 11:30:09 PDT
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.
Comment 49 Andrea Marchesini [:baku] 2013-03-15 22:40:28 PDT
> > +    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.
Comment 50 Andrea Marchesini [:baku] 2013-03-15 22:59:04 PDT
Created attachment 725722 [details] [diff] [review]
patch e

No file properties but we move back to arrays in FilePicker.js
Comment 51 Mounir Lamouri (:mounir) 2013-03-18 10:15:31 PDT
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).
Comment 52 Andrea Marchesini [:baku] 2013-03-18 10:43:42 PDT
Created attachment 726229 [details] [diff] [review]
patch
Comment 53 [:fabrice] Fabrice Desré 2013-03-18 11:53:16 PDT
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
Comment 54 Andrea Marchesini [:baku] 2013-03-19 04:31:54 PDT
Created attachment 726579 [details] [diff] [review]
m-c part 2 - input type=file for b2g
Comment 55 Andrea Marchesini [:baku] 2013-03-19 05:18:47 PDT
Created attachment 726594 [details] [diff] [review]
m-c part 1 - re enable input file

I just rebased your patch. How can review it?
Comment 56 Andrea Marchesini [:baku] 2013-03-19 06:44:52 PDT
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?
Comment 57 Andrea Marchesini [:baku] 2013-03-19 06:52:07 PDT
Created attachment 726640 [details] [diff] [review]
b2g18 part 2 - input type=file for b2g
Comment 58 Andrea Marchesini [:baku] 2013-03-19 06:54:42 PDT
Created attachment 726644 [details] [diff] [review]
b2g18 part 3 - from DOM Blob to DOM File

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.
Comment 59 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-03-19 10:35:09 PDT
Just make sure to give credit to the person who wrote the original patch in the checkin comment.
Comment 61 Andrea Marchesini [:baku] 2013-03-19 11:03:02 PDT
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.
Comment 62 Andrea Marchesini [:baku] 2013-03-19 11:03:18 PDT
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.
Comment 63 Andrea Marchesini [:baku] 2013-03-19 11:03:26 PDT
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.
Comment 65 Alex Keybl [:akeybl] 2013-03-20 12:37:15 PDT
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
Comment 66 Andrew Overholt [:overholt] 2013-03-21 11:10:20 PDT
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.
Comment 67 Jason Smith [:jsmith] 2013-03-21 11:18:13 PDT
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.
Comment 68 Chris Lee [:clee] 2013-03-21 14:43:44 PDT
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.
Comment 69 Lawrence Mandel [:lmandel] (use needinfo) 2013-03-21 14:46:51 PDT
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.
Comment 70 Jason Smith [:jsmith] 2013-03-22 07:56:26 PDT
Per the email thread on this, the decision is to uplift this to v1.1 per product.
Comment 71 Alex Keybl [:akeybl] 2013-03-22 15:28:21 PDT
(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 ;).
Comment 73 Brion Vibber 2013-04-01 11:36:59 PDT
I've confirmed this fixes mobile photo uploads on Wikipedia on my unagi phone as of build 20130328070204. Thanks everybody!
Comment 74 Jason Smith [:jsmith] 2013-06-18 22:50:37 PDT
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?
Comment 75 Mounir Lamouri (:mounir) 2013-06-19 03:27:04 PDT
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.
Comment 76 Jason Smith [:jsmith] 2013-06-19 09:11:24 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.