Closed Bug 412822 Opened 13 years ago Closed 13 years ago

nsIFilePicker makes it hard to drop in other url systems

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch change the interface (obsolete) — Splinter Review
Scenarios:
1. user runs firefox on windows and enters an http: url into the windows filepicker.

The result is a temporary file containing the contents of the url (except when windows doesn't like the url structure)

2. user runs firefox on gnome and browses to an obex: url

The result is nothing, because the file picker (which knows nothing about gnomevfs) fails to convert it to an nsILocalFile. And the nsIFileURL scheme would not be any more flexible

3. user wants to do something more interesting with the xul file picker

The xul file picker itself is constrained by the contract.

Proposal: change the contract so that for fileURLs, it returns an nsIURI (very flexible).

This would mean that a filepicker (e.g. the xul filepicker, or the gnome file picker) could return a url (http:, ftp:, obex:, data:) to any consumer that asks for a url. If the consumer QIs to nsIFileURL, it either succeeds or fails (at present our tree doesn't seem to have *anyone* using this feature, and given that you can't get multiple nsIFileURLs whereas you can get multiple nsILocalFiles, I can't see why most consumers would consider asking for nsIFileURLs today).

This is sort of step one in a 3 or 4 step process, the goal is to be able to upload from random protocols. Each of these changes should be relatively reasonable. I hope...
Attachment #297590 - Flags: superreview?(neil)
Attachment #297590 - Flags: review?(neil)
I'm using this patch (plus a couple of others) to upload this patch from an http url in linux :).
Attachment #297632 - Flags: superreview?(neil)
Attachment #297632 - Flags: review?(neil)
Blocks: 412877
Comment on attachment 297590 [details] [diff] [review]
change the interface

IMHO you can't call it a file picker any more.
Attachment #297590 - Flags: superreview?(neil)
Attachment #297590 - Flags: superreview-
Attachment #297590 - Flags: review?(neil)
Attachment #297632 - Flags: superreview?(neil)
Attachment #297632 - Flags: superreview-
Attachment #297632 - Flags: review?(neil)
I spoke with neil and I think we came to an agreement that this would be OK. creating a new interface doesn't really help anyone, and this interface is basically flexible enough.
Attachment #297590 - Attachment is obsolete: true
Attachment #297632 - Attachment is obsolete: true
Attachment #298480 - Flags: review?(neil)
No longer blocks: 412877
Blocks: 121059
Comment on attachment 298480 [details] [diff] [review]
use nsIFilePicker.appendFilters(nsIFilePicker.filterAllowURLs)

>+  return NS_NewURI(aFileURL, mFilename);
You can't make a URI out of a filename.

>+    allowURLs = o.allowURLs;
Turn on autocomplete from history perhaps? ;-)

>-  readonly attribute nsIFileURL fileURL;
>+  readonly attribute nsIURI fileURL;
(How?) do you know that nobody's expecting a fileURL?

>-  file->InitWithPath(mUnicodeFile);
>+  *aFileURL = nsnull;
>+  if (mFile.IsEmpty())
We don't use mFile here...

>+    try {
>+      if (this.mFileURL)
>+        return this.mFileURL;
How is this going to throw?
Attachment #298480 - Flags: review?(neil) → review-
http://timeless.justdave.net/mxr-test/os2008/search?string=upload_dialog&find=microb-eal

you can't see how this is implemented because it's hidden behind another trampoline, however it really uses the url flavor of the GTK APIs instead of the file flavor. note that I'm not changing that code, merely moving it.

upload_dialog_cb (GtkMozEmbed *embed, const gchar *path, const gchar *filter, gchar **file_name_with_path, GMozillaEngine *self)
...
    g_signal_emit_by_name (G_OBJECT(self), G_WEBWIDGET_SIGNAL_UPLOAD_DIALOG, getenv("HOME"), "", file_name_with_path, &response, NULL);
Attachment #298480 - Attachment is obsolete: true
Attachment #301537 - Flags: review?(neil)
Comment on attachment 301537 [details] [diff] [review]
Fix windows instance, clarify embedding case

>+  try {
>+    var ios = Components.classes[NS_IOSERVICE_CONTRACTID].getService(Components.interfaces.nsIIOService);
>+    retvals.fileURL = ios.newURI(textInput.value, '', null);
>+    fileList = [];
I don't think you should try to do this if allowURLs is false.

>+    retvals.fileURL = null;
(which means that you should do this earlier)

>+  if (!allowURLs || !retvals.fileURL) {
(which means that you won't have to check allowURLs here)

>+  return NS_NewURI(aFileURL, mFile.get());
Comment #4 was supposed to apply in multiple places, also why the .get()?

>+    if (!this.mFileURL && !this.mFilesEnumerator)
>+      return null;
>+
>+    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>                     .getService(Components.interfaces.nsIIOService);
>+    if (this.mFileURL)
>+      return this.mFileURL;
You're getting the ioService before you need to (might allow simplification).

>+    var url = ioService.newFileURI(this.file);
>+    return url;
Doesn't need a temporary.
Attachment #301537 - Flags: review?(neil) → review-
Attachment #301537 - Attachment is obsolete: true
Attachment #301865 - Flags: review?(neil)
Comment on attachment 301865 [details] [diff] [review]
Simplify JS case, redo others

This was fine on Windows but unfortunately didn't compile for me on Linux.

>+      retvals.fileURL = ios.newURI(textInput.value, '', null);
null, null is allowed.

>+      if (retvals.fileURL instanceof Components.interfaces.nsIFileURL)
>+        fileList.push(retvals.fileURL.nsIFileURL.file);
No need to QI twice.
Attachment #301865 - Flags: review?(neil) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #301865 - Flags: approval1.9?
Comment on attachment 301865 [details] [diff] [review]
Simplify JS case, redo others

a1.9+=damons
Attachment #301865 - Flags: approval1.9? → approval1.9+
Comment on attachment 301865 [details] [diff] [review]
Simplify JS case, redo others

mozilla/embedding/browser/gtk/src/EmbedFilePicker.cpp 	1.6
mozilla/embedding/browser/gtk/src/EmbedFilePicker.h 	1.6
mozilla/toolkit/components/filepicker/content/filepicker.js 	1.19
mozilla/toolkit/components/filepicker/content/filepicker.js 	1.20
mozilla/widget/public/nsIFilePicker.idl 	3.23
mozilla/widget/src/cocoa/nsFilePicker.h 	1.12
mozilla/widget/src/cocoa/nsFilePicker.mm 	1.21
mozilla/widget/src/beos/nsFilePicker.cpp 	1.30
mozilla/widget/src/beos/nsFilePicker.h 	1.13
mozilla/widget/src/gtk2/nsFilePicker.cpp 	1.17
mozilla/widget/src/gtk2/nsFilePicker.cpp 	1.18
mozilla/widget/src/gtk2/nsFilePicker.cpp 	1.19
mozilla/widget/src/gtk2/nsFilePicker.h 	1.6
mozilla/widget/src/os2/nsFilePicker.cpp 	1.55
mozilla/widget/src/os2/nsFilePicker.h 	1.16
mozilla/widget/src/photon/nsFilePicker.cpp 	1.14
mozilla/widget/src/photon/nsFilePicker.h 	1.8
mozilla/widget/src/windows/nsFilePicker.cpp 	1.96
mozilla/widget/src/windows/nsFilePicker.h 	1.23
mozilla/xpfe/components/filepicker/src/nsFilePicker.js.in 	1.49
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 420770
Depends on: 428666
Depends on: 495291
You need to log in before you can comment on or make changes to this bug.