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

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

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.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Use filepicker's open() instead of show(). (v1)

Comment 2

[Ian Neal]

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.

Comment 3

[Edmund Wong (:ewong)]

Attachment: Use filepicker's open() instead of show(). (v2)

Comment 4

[Ian Neal]

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

Comment 5

[Edmund Wong (:ewong)]

Attachment: Use filepicker's open() instead of show(). (v3)

Comment 6

[Ian Neal]

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.

Comment 7

[Edmund Wong (:ewong)]

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.

Comment 8

[Shubham Jindal [:shubham]]

Attachment: bug796994(v4).diff

Comment 9

[Philip Chee]

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.

Comment 10

[Philip Chee]

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().

> })

Comment 11

[Shubham Jindal [:shubham]]

Attachment: bug796994(v5).diff

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));
>        }
>      ));
>    }

Comment 13

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-new.patch

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.

Comment 14

[Frank-Rainer Grahl (:frg)]

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

Comment 15

[Frank-Rainer Grahl (:frg)]

No response. Taking the bug

Comment 16

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-open.patch

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.

Comment 17

[Hartmut Figge]

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.

Comment 18

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-open.patch

Fixing two minor let/var/const NITs to make everything consistent.

Comment 19

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-open-257.patch

Rebased patch after latest updates.

Comment 20

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-open-257.patch

Removed a leftover wrong // TODO to avoid confusion. Sorry for the bugspam.

Comment 21

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-open-257.patch

Rebased after Bug 1436605 landed.

Comment 22

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-open-257.patch

Another rebase

Comment 23

[Ian Neal]

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

Comment 24

[Frank-Rainer Grahl (:frg)]

Attachment: 796994-filepicker-open-257-V2.patch

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.

Comment 25

[Ian Neal]

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

[Triage Comment]
LGTM and very clever r/a=me

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/08ceb6d5c898
Replace obsolete filepicker.show with filepicker.open in suite. r=IanN

Comment 27

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/919d80f33c8fc63b98b08035cca6ef36b1dc11d3