Use filepicker's open() instead of the obsolete show() in /suite/*

RESOLVED FIXED in seamonkey2.58

Status

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: philip.chee, Assigned: frg)

Tracking

(Blocks 1 bug)

Trunk
seamonkey2.58
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.57esr fixed, seamonkey2.58 fixed)

Details

Attachments

(1 attachment, 12 obsolete attachments)

58.38 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
Reporter

Description

7 years ago
From Bug 781973 Comment 0:

> Bug 731307 adds code to make the filepicker show method obsolete. It will
> still be supported for a very long time, but we should update our internal
> uses to use the new showAsync method instead.
Reporter

Updated

7 years ago
Blocks: 808722

Updated

7 years ago
No longer blocks: 808722

Updated

7 years ago
Depends on: 781973
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #735559 - Flags: review?(iann_bugzilla)

Comment 2

6 years ago
Comment on attachment 735559 [details] [diff] [review]
Use filepicker's open() instead of show(). (v1)

This is on code inspection only, no testing as yet.

>+++ b/suite/browser/navigator.js
>@@ -1364,37 +1364,40 @@ function selectFileToOpen(label, prefRoo
>   var fileURL = null;
> 
>   // Get filepicker component.
>   const nsIFilePicker = Components.interfaces.nsIFilePicker;
>   var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>   fp.init(window, gNavigatorBundle.getString(label), nsIFilePicker.modeOpen);
>   fp.appendFilters(nsIFilePicker.filterAll | nsIFilePicker.filterText | nsIFilePicker.filterImages |
>                    nsIFilePicker.filterXML | nsIFilePicker.filterHTML);
>+  var fpCallback = function fpCallback_done(aResult) {
>+    if (aResult == nsIFilePicker.returnOK) {
>+      Services.prefs.setIntPref(filterIndexPref, fp.filterIndex);
>+      Services.prefs.setComplexValue(lastDirPref,
>+                                     Components.interfaces.nsILocalFile,
>+                                     fp.file.parent);
>+      fileURL = fp.fileURL;
>+    }
>+  };
> 
>   const filterIndexPref = prefRoot + "filterIndex";
>   const lastDirPref = prefRoot + "dir";
> 
>   // use a pref to remember the filterIndex selected by the user.
>   fp.filterIndex = GetIntPref(filterIndexPref, 0);
> 
>   // use a pref to remember the displayDirectory selected by the user.
>   try {
>     fp.displayDirectory = Services.prefs.getComplexValue(lastDirPref,
>                               Components.interfaces.nsILocalFile);
>   } catch (ex) {
>   }
> 
>-  if (fp.show() == nsIFilePicker.returnOK) {
>-    Services.prefs.setIntPref(filterIndexPref, fp.filterIndex);
>-    Services.prefs.setComplexValue(lastDirPref,
>-                                   Components.interfaces.nsILocalFile,
>-                                   fp.file.parent);
>-    fileURL = fp.fileURL;
>-  }
>+  fp.open(fpCallback);
> 
>   return fileURL;
This return should probably be moved into the callback.

>+++ b/suite/browser/pageinfo/pageInfo.js
>@@ -865,20 +865,24 @@ function selectSaveFolder()
>   if (!initialDir) {
>     let dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>                             .getService(Components.interfaces.nsIDownloadManager);
>     initialDir = dnldMgr.userDownloadsDirectory;
>   }
>   fp.displayDirectory = initialDir;
> 
>   fp.appendFilters(nsIFilePicker.filterAll);
>-  var ret = fp.show();
>+  var fpCallback = function fpCallback_done(aResult) {
>+    if (aResult == nsIFilePicker.returnOK) {
>+      return fp.file.QueryInterface(nsILocalFile);
>+    }
>+  };
> 
>-  if (ret == nsIFilePicker.returnOK)
>-    return fp.file.QueryInterface(nsILocalFile);
>+  fp.open(fpCallback);
>+
>   return null;
This return should probably be moved into the callback.

>+++ b/suite/common/pref/pref-applications.js

>+    var fpCallback = function fpCallback_done(aResult) {
>+      if (aResult == nsIFilePicker.returnOK && fp.file &&
>+          this._isValidHanlderExecutable(fp.file)) {
>+        handlerApp = Components.classes["@mozilla.org/uriloader/local-handler-app;1"]
>+                               .createInstance(nsILocalHandlerApp);
>+        handlerApp.name = getFileDisplayName(fp.file);
>+        handlerApp.executable = fp.file;
> 
>+        // Add the app to the type's list of possible handlers.
>+        let handlerInfo = this._handledTypes[this._list.selectedItem.type];
>+        handlerInfo.addPossibleApplicationHandler(handlerApp);
>+      }
>+    };
>+    
> #endif
> 
>     // Rebuild the actions menu whether the user picked an app or canceled.
>     // If they picked an app, we want to add the app to the menu and select it.
>     // If they canceled, we want to go back to their previous selection.
>     this.rebuildActionsMenu();
> 
>     // If the user picked a new app from the menu, select it.
This is wrong, look at how it is done in bug 781973.

>+++ b/suite/mailnews/mailWindowOverlay.js
>@@ -1525,31 +1525,32 @@ function MsgSaveAsTemplate()
> {
>   SaveAsTemplate(gFolderDisplay.selectedMessageUri);
> }
> 
> const nsIFilePicker = Components.interfaces.nsIFilePicker;
> 
> function MsgOpenFromFile()
> {
>+  var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>+  var fpCallback = function fpCallback_done(aResult) {
>+    if (aResult == nsIFilePicker.returnCancel)
>+      return;
>+  };
>   var filterLabel = gMessengerBundle.getString("EMLFiles");
>   var windowTitle = gMessengerBundle.getString("OpenEMLFiles");
> 
>    fp.init(window, windowTitle, nsIFilePicker.modeOpen);
>    fp.appendFilter(filterLabel, "*.eml; *.msg");
> 
>    // Default or last filter is "All Files"
>    fp.appendFilters(nsIFilePicker.filterAll);
> 
>   try {
>+    fp.open(fpCallback);
>    }
>    catch (ex) {
>      dump("filePicker.chooseInputFile threw an exception\n");
>      return;
>    }
> 
>   var uri = fp.fileURL.QueryInterface(Components.interfaces.nsIURL);
>   uri.query = "type=application/x-message-display";
>   window.openDialog( "chrome://messenger/content/messageWindow.xul", "_blank", "all,chrome,dialog=no,status,toolbar", uri, null, null );
These lines need to be included within the callback.

r- as I want to see, and test, the new patch.
Attachment #735559 - Flags: review?(iann_bugzilla) → review-
Attachment #735559 - Attachment is obsolete: true
Attachment #737321 - Flags: review?(iann_bugzilla)

Comment 4

6 years ago
Comment on attachment 737321 [details] [diff] [review]
Use filepicker's open() instead of show(). (v2)

>+++ b/suite/browser/navigator.js
>@@ -1364,39 +1364,42 @@ function selectFileToOpen(label, prefRoo
>   var fileURL = null;
This probably needs moving into the callback function.
> 
>   // Get filepicker component.
>   const nsIFilePicker = Components.interfaces.nsIFilePicker;
>   var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>   fp.init(window, gNavigatorBundle.getString(label), nsIFilePicker.modeOpen);
>   fp.appendFilters(nsIFilePicker.filterAll | nsIFilePicker.filterText | nsIFilePicker.filterImages |
>                    nsIFilePicker.filterXML | nsIFilePicker.filterHTML);
>+  var fpCallback = function fpCallback_done(aResult) {
>+    if (aResult == nsIFilePicker.returnOK) {
>+      Services.prefs.setIntPref(filterIndexPref, fp.filterIndex);
>+      Services.prefs.setComplexValue(lastDirPref,
>+                                     Components.interfaces.nsILocalFile,
>+                                     fp.file.parent);
>+      fileURL = fp.fileURL;
>+
>+      return fileURL;
This probably needs moving out of the if statement.
>+    }
>+  };

>+++ b/suite/common/pref/pref-applications.js

> #ifdef XP_WIN
>     var params = {};
>     var handlerInfo = this._handledTypes[this._list.selectedItem.type];
> 
>     if (isFeedType(handlerInfo.type)) {
>       // MIME info will be null, create a temp object.
>       params.mimeInfo = mimeSvc.getFromTypeAndExtension(handlerInfo.type, 
>                                                  handlerInfo.primaryExtension);
>@@ -1597,60 +1620,43 @@ var gApplicationsPane = {
>                       params);
> 
>     if (this.isValidHandlerApp(params.handlerApp)) {
>       handlerApp = params.handlerApp;
> 
>       // Add the app to the type's list of possible handlers.
>       handlerInfo.addPossibleApplicationHandler(handlerApp);
>     }
>+
>+    chooseAppCallback(handlerApp);
> #else
>     var fp = Components.classes["@mozilla.org/filepicker;1"]
>                        .createInstance(nsIFilePicker);
>     var winTitle = this._prefsBundle.getString("fpTitleChooseApp");
>+    var fpCallback = function fpCallback_done(aResult) {
>+      if (aResult == nsIFilePicker.returnOK && fp.file &&
>+          this._isValidHanlderExecutable(fp.file)) {
>+        handlerApp = Components.classes["@mozilla.org/uriloader/local-handler-app;1"]
>+                               .createInstance(nsILocalHandlerApp);
>+        handlerApp.name = getFileDisplayName(fp.file);
>+        handlerApp.executable = fp.file;
>+
>+        // Add the app to the type's list of possible handlers.
>+        let handlerInfo = this._handledTypes[this._list.selectedItem.type];
>+        handlerInfo.addPossibleApplicationHandler(handlerApp);
You probably need to call chooseAppCallback here now.
>+      }
>+    }.bind(this);

>+++ b/suite/mailnews/mailWindowOverlay.js
> function MsgOpenFromFile()
> {
>+  var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>+  var fpCallback = function fpCallback_done(aResult) {
>+    if (aResult == nsIFilePicker.returnCancel)
>+      return;
>+
>+    var uri = fp.fileURL.QueryInterface(Components.interfaces.nsIURL);
>+    uri.query = "type=application/x-message-display";
>+
>+    window.openDialog("chrome://messenger/content/messageWindow.xul",
>+                      "_blank", "all,chrome,dialog=no,status,toolbar", uri, null, null );
Perhaps change the if statement to use returnOK and move these three lines into the if statement.
>+  };
r- for the moment
Attachment #737321 - Flags: review?(iann_bugzilla) → review-
Attachment #737321 - Attachment is obsolete: true
Attachment #738842 - Flags: review?(iann_bugzilla)

Comment 6

6 years ago
Comment on attachment 738842 [details] [diff] [review]
Use filepicker's open() instead of show(). (v3)

The navigator.js changes won't work, you need to move the code from the callers of selectFileToOpen into the callback.
Check what Firefox did.
You need to also check the other files for similar issues.
Please make sure you test the changes before submitting the next patch.
Attachment #738842 - Flags: review?(iann_bugzilla) → review-
Gone back to square one, so this bug will have to have someone else to give it love.  I might retake this bug when I feel I have the brain cells to match the required skill.  Until then, this is available for anyone to take.
Assignee: ewong → nobody
Status: ASSIGNED → NEW
Posted patch bug796994(v4).diff (obsolete) — Splinter Review
Attachment #8536552 - Flags: review?(philip.chee)
Reporter

Comment 9

5 years ago
Comment on attachment 8536552 [details] [diff] [review]
bug796994(v4).diff

There are two bugs we can use as references:

Firefox Bug 781973 - Use filepicker's open() instead of the obsolete show() in /browser
https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390

SeaMonkey Bug 896947 Use asynchronous version of getCharsetForURI
https://hg.mozilla.org/comm-central/rev/dc23a5a6112e

We aren't going to slavishly follow Firefox, not because of NIH but it's now 2+ years later and we now have better and more efficient solutions.

> @@ -1404,39 +1404,42 @@ function selectFileToOpen(label, prefRoo
> 
> +  var fpCallback = function fpCallback_done(aResult) {
> +    if (aResult == nsIFilePicker.returnOK) {
> +      Services.prefs.setIntPref(filterIndexPref, fp.filterIndex);
> +      Services.prefs.setComplexValue(lastDirPref,
> +                                    Components.interfaces.nsILocalFile,
> +                                    fp.file.parent);
> +      fileURL = fp.fileURL;  
> +    }
> +    return fileURL;
> +  };
> +
> +  fp.open(fpCallback);

This is wrong. selectFileToOpen() has two callers and both expect a fileURL string back. Traditionally async code has been written using callbacks so there is a temptation here to make the callers pass a callback. This leads to callback hell, nested callbacks, and a mass of unreadable code.

https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390#l3.22
At this point in the Firefox patch we have nested callbacks with aCallback() inside fpCallback().

But we can do better. We can use Promises instead. If we change selectFileToOpen to return a promise the caller should be able to do:

function BrowserOpenFileWindow()
{
  try {
    openTopWin(selectFileToOpen("openFile", "browser.open.").spec);
  } catch (e) {}
}

becomes:

function BrowserOpenFileWindow()
{
  selectFileToOpen("openFile", "browser.open.").then(fileURL => {
    openTopWin(fileURL.spec);
  }).catch(error => {});
}

The above example demonstrates using a "then-able", fat arrow => and the catch() method in a Promise object.

That takes care of the callers, now change selectFileToOpen() to return a promise instead.

> --- a/suite/browser/pageinfo/pageInfo.js
> +++ b/suite/browser/pageinfo/pageInfo.js
> @@ -869,21 +869,24 @@ function selectSaveFolder()
Same for pageInfo

> --- a/suite/common/bookmarks/bookmarksManager.js
> +++ b/suite/common/bookmarks/bookmarksManager.js

> -    if (fp.show() != Components.interfaces.nsIFilePicker.returnCancel) {
> -      if (fp.fileURL) {
> -        Components.utils.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
> -        BookmarkHTMLUtils.importFromURL(fp.fileURL.spec, false)
> -                         .then(null, Components.utils.reportError);
> -      }
> -    }
> +    
> +    var fpCallback = function fpCallback_done(aResult) {
These days we don't decorate the function expression so:
var fpCallback = function (aResult) {

> +      if (aResult != Components.interfaces.nsIFilePicker.returnCancel) {
> +        if (fp.fileURL) {
You can collapse the nested if's
if (aResult != fp..returnCancel && fp.fileURL) {
(see: https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390#l6.15)

> +          Components.utils.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
> +          BookmarkHTMLUtils.importFromURL(fp.fileURL.spec, false)
> +                           .then(null, Components.utils.reportError);
> +        }
> +      }  
> +    };
> +
> +    fp.open(fpCallback);

> +        BookmarkHTMLUtils.exportToFile(fp.file)
> +                         .then(null, Components.utils.reportError);
While you're here please change:
.exportToFile(fp.file)
to
.exportToFile(fp.file.path)
(we should really port Bug 824433 from Firefox)
    
> +    var fpCallback = function fpCallback_done(aResult) {
> +      if (aResult != Components.interfaces.nsIFilePicker.returnCancel)
if (aResult != fp.returnCancel)

> +        this.restoreBookmarksFromFile(fp.file);  
> +    };
You can't use "this" here because it's the wrong this.
You can bind the right this to the callback. e.g.
https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390#l6.86
Or you can use a fat arrow function since fat arrows us a lexical "this"

    var fpCallback = aResult => {
      if (aResult != fp.returnCancel) {
        this.restoreBookmarksFromFile(fp.file.path);
      }
    }

+      if (aResult != Components.interfaces.nsIFilePicker.returnCancel)
+        PlacesBackups.saveBookmarksToJSONFile(fp.file); 
PlacesBackups.saveBookmarksToJSONFile(fp.file.path);

> -    if (fp.show() == nsIFilePicker.returnOK && fp.fileURL.spec && fp.fileURL.spec.length > 0)
> -      gInput.value = fp.fileURL.spec;
> +    var fpCallback = function fpCallback_done(aResult) {
> +      if (aResult == nsIFilePicker.returnOK && fp.fileURL.spec && fp.fileURL.spec.length > 0)
Slightly better:
if (aResult == fp.returnOK && fp.fileURL.spec && fp.fileURL.spec.length)

> +        gInput.value = fp.fileURL.spec;
> +    };
> +
> +    fp.open(fpCallback);
>    }
>    catch(ex) {
>    }
>    doEnabling();
Move doEnabling() into the callback.

Even better - use Promises instead:

var deferComplete = Promise.defer();
fp.open(function(aResult) {
  deferComplete.resolve(aResult);
});
deferComplete.then(aResult => {
  if (aResult == fp.returnOK && fp.fileURL.spec && fp.fileURL.spec.length)
    gInput.value = fp.fileURL.spec;
  doEnabling();
})

> --- a/suite/common/pref/pref-applications.js
> +++ b/suite/common/pref/pref-applications.js
This is wrong. See the Firefox version at:
https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390#l7.1
That isn't much better, but at least it works.
* I need to think about this a bit more, but in the meantime if you can come up with something better go ahead.

> @@ -71,18 +71,23 @@ function CacheSelectFolder()
> 
> -  if (fp.show() == nsIFilePicker.returnOK)
> -    pref.value = fp.file;
> +  
> +  var fpCallback = function fpCallback_done(aResult) {
> +    if (aResult == nsIFilePicker.returnOK)
> +      pref.value = fp.file;  
> +  };
> +
> +  fp.open(fpCallback);
Either inline the callback or use a Promise.

> +  var fpCallback = function fpCallback_done(aResult) {
> +    if (aResult == nsIFilePicker.returnOK) {
> +      var currentDirPref = document.getElementById("browser.download.dir");
> +      currentDirPref.value = fp.file;
> +      folderListPref.value = FolderToIndex(fp.file);
> +      // Note, the real prefs will not be updated yet, so dnld manager's
> +      // userDownloadsDirectory may not return the right folder after
> +      // this code executes. displayDownloadDirPref will be called on
> +      // the assignment above to update the UI.
> +    }
> +  };
> +
> +  fp.open(fpCallback);
Use a promise here.

> -  if (fp.show() == nsIFilePicker.returnOK)
> -    aSoundUrlPref.value = fp.fileURL.spec;
> +  var fpCallback = function fpCallback_done(aResult) {
> +    if (aResult == nsIFilePicker.returnOK)
> +      aSoundUrlPref.value = fp.fileURL.spec;
> +  };
> +
> +  fp.open(fpCallback);
Either inline the callback or use a Promise.

> +      let fpCallback = function fpCallback_done(aResult) {
> +        if (aResult == Ci.nsIFilePicker.returnOK
> +          || aResult == Ci.nsIFilePicker.returnReplace) {
> +          let stream = Cc["@mozilla.org/network/file-output-stream;1"]
> +                         .createInstance(Ci.nsIFileOutputStream);
> +          stream.init(filepicker.file, -1, parseInt("0600", 8), 0);
> +
> +          let serializer = new XMLSerializer();
> +          let output = serializer.serializeToString(iframe.contentDocument);
> +          output = output.replace(/<!DOCTYPE (.|\n)*?]>/,
> +            '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" ' +
> +            '"DTD/xhtml1-strict.dtd">');
> +          output = Weave.Utils.encodeUTF8(output);
> +          stream.write(output, output.length);
> +        }
> +        return false;
This return doesn't do anything. Please remove the above line.

> +      };
> +
> +      filepicker.open(fpCallback);
>      });
Use a promise here.

> --- a/suite/feeds/src/FeedWriter.js
> +++ b/suite/feeds/src/FeedWriter.js

> +      var fpCallback = function fpCallback_done(aResult) {
> +        if (aResult == Components.interfaces.nsIFilePicker.returnOK) {
> +          this._selectedApp = fp.file;
> +          if (this._selectedApp) {
> +            var file = Services.dirsvc.get("XREExeF", Components.interfaces.nsILocalFile);
> +            if (fp.file.leafName != file.leafName) {
> +              this._initMenuItemWithFile(this._selectedAppMenuItem,
> +                                         this._selectedApp);
>  
> -            // Show and select the selected application menuitem
> -            this._selectedAppMenuItem.hidden = false;
> -            this._selectedAppMenuItem.doCommand();
> -            return true;
> +              // Show and select the selected application menuitem
> +              this._selectedAppMenuItem.hidden = false;
> +              this._selectedAppMenuItem.doCommand();
> +              return true;
> +            }
>            }
>          }
> -      }
> +      };
> +
> +      fp.open(fpCallback);
This doesn't do what you think it does.

_chooseClientApp() has two callers. You should modify it to return a promise and then fix the callers to take a promise.

> +  var fpCallback = function fpCallback_done(aResult) {
> +    if (aResult == nsIFilePicker.returnOK) {
> +      document.getElementById("PhotoFile").file = fp.file;
> +      onSwitchPhotoType(document.getElementById("FilePhotoType").value);
> +      return true;
> +    }
> +    return false;
It's useless to return anything in the Callback because the return value is ignored.


>   //Get file using nsIFilePicker and convert to URL
>   try {
....
> +      var fpCallback = function fpCallback_done(aResult) {
> +        if (aResult == nsIFilePicker.returnOK) {
> +          var firstAttachedFile = AttachFiles(fp.files);
> +          if (firstAttachedFile)
> +            SetLastAttachDirectory(firstAttachedFile);
> +        }
> +      };
> +
> +      fp.open(fpCallback);
I don't think anything can throw here but you can use a promise here and a final .catch() at the end of the Promise chain.
(try/catch doesn't work very well with async code)

>    try {
> -     var ret = fp.show();
> -     if (ret == nsIFilePicker.returnCancel)
> +    var fpCallback = function fpCallback_done(aResult) {
> +      if (aResult == nsIFilePicker.returnCancel)
>         return;
> -   }
> -   catch (ex) {
> -     dump("filePicker.chooseInputFile threw an exception\n");
> -     return;
> -   }
> +    };
> +
> +    fp.open(fpCallback);
> +  }
> +  catch (ex) {
> +    dump("filePicker.chooseInputFile threw an exception\n");
> +    return;
> +  }
Noooo. You need to move the following lines into the callback.

>    var uri = fp.fileURL.QueryInterface(Components.interfaces.nsIURL);
>    uri.query = "type=application/x-message-display";
>  
>    window.openDialog( "chrome://messenger/content/messageWindow.xul", "_blank", "all,chrome,dialog=no,status,toolbar", uri, null, null );

If you use promise.defer/promise.resolve here you can use the .catch() method on the promise.
Attachment #8536552 - Flags: review?(philip.chee) → review-
Reporter

Comment 10

5 years ago
Discussion over IRC:

> var promise = Promise.defer();
I notice that Mozilla style is something like:
var deferred = Promise.defer();

> fp.open(function(aResult) {
> 	promise.resolve(aResult);
> });
> promise.then(aResult => {
> 	if (aResult == nsIFilePicker.returnOK) {
Two space indentation and no tabs please.
You can use fp.returnOK instead.

> 	    Services.prefs.setIntPref(filterIndexPref, fp.filterIndex);
> 	    Services.prefs.setComplexValue(lastDirPref,
> 	                                  Components.interfaces.nsILocalFile,
Please change this to Components.interfaces.nsIFile. In current Gecko interfaces nsILocalFile has been folded into nsIFile and nsILocalFile just forwards to nsIFile.

> 	                                  fp.file.parent);
Vertically align Components... and fp.file... with lastDirPref
> 	    fileURL = fp.fileURL;  
>   }
>   return Promise.resolve(fileURL);
This should be moved out of the previous then().

> })
Assignee: nobody → shubhamjindal18
Posted patch bug796994(v5).diff (obsolete) — Splinter Review
I have done the required changes. Some concerns.

> --- a/suite/common/pref/pref-applications.js
> +++ b/suite/common/pref/pref-applications.js

Firefox Version: https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390#l7.1

Do I need to add the entire function of chooseAppCallback as done in the Firefox implementation and then call it in the fpCallback?

> --- a/suite/browser/pageinfo/pageInfo.js
> +++ b/suite/browser/pageinfo/pageInfo.js

> function saveMedia()
> {
...
> selectSaveFolder().then(result => {
>   odir = result;
> });
> ...
> for (var i = 0; i < rowArray.length; i++) {
>      var v = rowArray[i];
>      var dir = odir.clone();
>      var item = gImageView.data[v][COL_IMAGE_NODE];
>      var uriString = gImageView.data[v][COL_IMAGE_ADDRESS];
>      var uri = makeURI(uriString);
>
>      try {
>        uri.QueryInterface(Components.interfaces.nsIURL);
>        dir.append(decodeURIComponent(uri.fileName));
>      }
>      catch(ex) { /* data: uris */ }
>
>      if (i == 0)
>        saveAnImage(uriString, new AutoChosen(dir, uri), makeURI(item.baseURI));
>      else {
>        // This delay is a hack which prevents the download manager
>        // from opening many times. See bug 377339.
>        setTimeout(saveAnImage, 200, uriString, new AutoChosen(dir, uri),
>                   makeURI(item.baseURI));
>      }
>    }

I have a strong feeling that this is incorrect. My feeling says it should be something like

>    for (var i = 0; i < rowArray.length; i++) {
>      var v = rowArray[i];
>      selectSaveFolder().then(result => {
>        var odir = result;
>        var dir = odir.clone();
>        var item = gImageView.data[v][COL_IMAGE_NODE];
>        var uriString = gImageView.data[v][COL_IMAGE_ADDRESS];
>        var uri = makeURI(uriString);
>
>        try {
>          uri.QueryInterface(Components.interfaces.nsIURL);
>          dir.append(decodeURIComponent(uri.fileName));
>        }
>        catch(ex) { /* data: uris */ }
>
>        if (i == 0)
>          saveAnImage(uriString, new AutoChosen(dir, uri), makeURI(item.baseURI));
>        else {
>          // This delay is a hack which prevents the download manager
>          // from opening many times. See bug 377339.
>          setTimeout(saveAnImage, 200, uriString, new AutoChosen(dir, uri),
>                     makeURI(item.baseURI));
>        }
>      ));
>    }
Attachment #738842 - Attachment is obsolete: true
Attachment #8536552 - Attachment is obsolete: true
Attachment #8542078 - Flags: feedback?(philip.chee)
Assignee

Updated

2 years ago
See Also: → 1341211
Assignee

Updated

2 years ago
Duplicate of this bug: 1390213
Assignee

Comment 13

2 years ago
Posted patch 796994-filepicker-new.patch (obsolete) — Splinter Review
Untested and incomplete. I just picked the easy targets from browser and mail. Need to be merged with the old patch and fixed up for todays tree.
Assignee

Comment 14

2 years ago
Shubham, I just saw that you are still active in Bugzilla. filepicker.show has been removed and we need to fix this bug now. Do you still want to work on it? Otherwisse I will just reassign it.

Thanks
Flags: needinfo?(shubhamjindal18)
Assignee

Comment 15

2 years ago
No response. Taking the bug
Assignee: shubhamjindal18 → frgrahl
Status: NEW → ASSIGNED
Flags: needinfo?(shubhamjindal18)

Updated

2 years ago
See Also: → 1417819
Assignee

Updated

2 years ago
See Also: 1417819
Assignee

Comment 16

Last year
Posted patch 796994-filepicker-open.patch (obsolete) — Splinter Review
I tested most of it in 2.55 because mail/news and some other parts are currently broken in 2.57. Would be glad if someone counld test the mail parts in 2.55 and/or osx and Linux.
Attachment #8542078 - Attachment is obsolete: true
Attachment #8901585 - Attachment is obsolete: true
Attachment #8542078 - Flags: feedback?(philip.chee)
Attachment #8946119 - Flags: review?(iann_bugzilla)

Comment 17

Last year
Tested with the last usable SM 2.56a1 Linux X86_64. 2018-01-03 15:43:00 PST   c-c:454eb3e4fa5e m-c:c9bec96cc789. File->Open File works even under Modern with ui.allow_platform_file_picker set to false. Thanks, Frank-Rainer.
Assignee

Comment 18

Last year
Posted patch 796994-filepicker-open.patch (obsolete) — Splinter Review
Fixing two minor let/var/const NITs to make everything consistent.
Attachment #8946119 - Attachment is obsolete: true
Attachment #8946119 - Flags: review?(iann_bugzilla)
Attachment #8946158 - Flags: review?(iann_bugzilla)
Assignee

Comment 19

Last year
Rebased patch after latest updates.
Attachment #8946158 - Attachment is obsolete: true
Attachment #8946158 - Flags: review?(iann_bugzilla)
Attachment #8950056 - Flags: review?(iann_bugzilla)
Assignee

Comment 20

Last year
Removed a leftover wrong // TODO to avoid confusion. Sorry for the bugspam.
Attachment #8950056 - Attachment is obsolete: true
Attachment #8950056 - Flags: review?(iann_bugzilla)
Attachment #8950101 - Flags: review?(iann_bugzilla)
Assignee

Updated

Last year
Blocks: 1378089
Assignee

Comment 21

Last year
Rebased after Bug 1436605 landed.
Attachment #8950101 - Attachment is obsolete: true
Attachment #8950101 - Flags: review?(iann_bugzilla)
Attachment #8955772 - Flags: review?(iann_bugzilla)
Assignee

Comment 22

Last year
Another rebase
Attachment #8955772 - Attachment is obsolete: true
Attachment #8955772 - Flags: review?(iann_bugzilla)
Attachment #8955894 - Flags: review?(iann_bugzilla)

Comment 23

Last year
Comment on attachment 8955894 [details] [diff] [review]
796994-filepicker-open-257.patch

Currently for BrowserOpenFileWindow we remember the filterIndex but that doesn't happen under this patch.
Part of the reason for the helper selectFileToOpen was to remove some code duplication. I know gLastOpenDirectory, in its current form, matches what Firefox does but that seems to prevent the de-duplication (Firefox doesn't seem to have an equivalent upload function).
f=me for the moment as I want to review the next version
Attachment #8955894 - Flags: review?(iann_bugzilla) → feedback+
Assignee

Comment 24

Last year
Only navigator.js changed compared to V1 as discussed via irc some time ago. Restored the filterIndex and made saving it and the last dir using a small ES6 class. Some overkill but I like it and could be used for other cases if needed :) Only instantiated when needed and only once during browser lifetime.

I could probably optimize it by only putting the two callbacks in separate functions but I can live with the current duplication.

There was an error in the first patch. Everything worked but gPrivate was spelled gprivate and the prefs where not saved.

Tested under 2.55 only including uploading to an ftp site.

If ok please approve for comm-beta 2.57.
Attachment #8955894 - Attachment is obsolete: true
Attachment #8966714 - Flags: review?(iann_bugzilla)

Comment 25

Last year
Comment on attachment 8966714 [details] [diff] [review]
796994-filepicker-open-257-V2.patch

[Triage Comment]
LGTM and very clever r/a=me
Attachment #8966714 - Flags: review?(iann_bugzilla)
Attachment #8966714 - Flags: review+
Attachment #8966714 - Flags: approval-comm-beta+

Comment 26

Last year
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/08ceb6d5c898
Replace obsolete filepicker.show with filepicker.open in suite. r=IanN
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Assignee

Updated

Last year
Blocks: 1468063

Updated

3 months ago
Regressions: 1543711
You need to log in before you can comment on or make changes to this bug.