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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Brian R. Bondy [:bbondy]]

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.

There may also be a few other uses in cpp code throughout the tree in various tools, so a find in files should be done to fix those too in the context of this bug.

Comment 1

[Brian R. Bondy [:bbondy]]

showAsync was renamed to open()

Comment 2

[Andres Hernandez [:andreshm]]

I'll take a look at this.

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Updated all browser/* filePicker.show to async filePicker.open.

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 660931 [details] [diff] [review]
Patch v1

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

Thanks for the patch!

Let's update nsIFilePickerShownCallback's uuid and add the function annotation to it so you can simplify all of the calls you fixed.
See here: https://developer.mozilla.org/en-US/docs/XPIDL

An example use of this annotation can be found in: xpcom\threads\nsIRunnable.idl

Then add .bind(this) to everything like we do in the dispatch file to avoid any regressions. 
An example use of this annotation can be found in: toolkit\components\filepicker\nsFilePicker.js

Please request review again with those changes and I'll continue reviewing.

Comment 5

[Brian R. Bondy [:bbondy]]

-...in the dispatch file...
+...in the dispatch function...

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Applied changes.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

You don't need to change an interface's IID to mark it "function", since that doesn't affect binary compatibility.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

The testpilot changes might need to be "upstreamed" somehow, but I'm not sure whether they apply to "test pilot 2" (see bug 783002, and bug 719455).

Comment 9

[Andres Hernandez [:andreshm]]

Attachment: Patch v3 (browser)

Removed the UUID and test pilot changes.

Comment 10

[Andres Hernandez [:andreshm]]

Attachment: Patch v3 (test pilot)

Test pilot part, not sure if it should be in this bug.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 661379 [details] [diff] [review]
Patch v2

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

Looks good, thanks for all the work.  Please r=me with comments addressed.  
Please push to try and manually test as many cases as you can before landing.

::: browser/base/content/sync/utils.js
@@ +171,5 @@
> +
> +      fp.init(window, dialogTitle, Ci.nsIFilePicker.modeSave);
> +      fp.appendFilters(Ci.nsIFilePicker.filterHTML);
> +      fp.defaultString = defaultSaveName;
> +      fp.open(fpCallback);

I think we should be returning false after this to keep the code consistent with how it was before.

::: browser/components/feeds/src/FeedWriter.js
@@ +708,5 @@
>  
>    /**
>     * Displays a prompt from which the user may choose a (client) feed reader.
> +   * @param aCallback the callback method, return true if a feed reader was
> +   *                  selected, false otherwise.

nit: passes in true if a feed reader was selected, false otherwise.

@@ +739,5 @@
> +                            "selectedAppMenuItem.doCommand();"
> +              Cu.evalInSandbox(codeStr, this._contentSandbox);
> +              if (aCallback) {
> +                aCallback(true);
> +                callbackCalled = true;

nit: I think you can just return here and remove the callbackCalled variable completely.

::: browser/devtools/scratchpad/scratchpad.js
@@ +666,5 @@
>          }
>        }.bind(this));
> +    }.bind(this);
> +
> +    if (aIndex && aIndex > -1) {

I think what you're trying to do here is:
if (aIndex !== unefined && aIndex > -1) {

If aIndex is 0 (which I think is a valid value), it will never enter the condition.

But since undefined > X is always false, you can just simplify to:
if (aIndex > -1) {

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +693,5 @@
>     * @param nsIWindow aParentWindow
>     *        Optional parent window. If null the parent window of the file picker
>     *        will be the window of the attached input element.
> +   * @param aCallback
> +   *        The callback method, return the selected file or null if the user

nit: which will be called passing in the selected...

Comment 12

[Brian R. Bondy [:bbondy]]

You can carry forward an r+ on the new patches once the review comments are implemented assuming they are the same as the ones I reviewed with the couple changes you mentioned.

Thanks for clarifying about the uuid Gavin, I knew it should be updated when there are changes but I never knew why (binary compatibility).

Comment 13

[Brian R. Bondy [:bbondy]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> The testpilot changes might need to be "upstreamed" somehow, but I'm not
> sure whether they apply to "test pilot 2" (see bug 783002, and bug 719455).

I think there are some file picker uses in c++ code as well, could you file a followup bug for that?  Also you might as well move the testpilot change into its own bug so it won't hold up landing the rest.

Comment 14

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Updated patch with suggested changes.

Comment 15

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=503738a4061f

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 662615 [details] [diff] [review]
Patch v4

> function BrowserOpenFileWindow()
> {
>   // Get filepicker component.
>   try {
>-    const nsIFilePicker = Components.interfaces.nsIFilePicker;
>-    var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>-    fp.init(window, gNavigatorBundle.getString("openFile"), nsIFilePicker.modeOpen);
>-    fp.appendFilters(nsIFilePicker.filterAll | nsIFilePicker.filterText | nsIFilePicker.filterImages |
>-                     nsIFilePicker.filterXML | nsIFilePicker.filterHTML);
>+    const nsIFilePicker = Ci.nsIFilePicker;
>+    let fp = Cc["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>+    let fpCallback = function fpCallback_done(aResult) {
>+      if (aResult == nsIFilePicker.returnOK) {
>+        try {
>+          if (fp.file) {
>+            gLastOpenDirectory.path =
>+              fp.file.parent.QueryInterface(Ci.nsILocalFile);
>+          }
>+        } catch (ex) {
>+        }
>+        openUILinkIn(fp.fileURL.spec, "current");
>+      }
>+    }.bind(this);

What's the point of binding fpCallback to 'this' (the window object in this case)?

Comment 17

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Then add .bind(this) to everything

Please don't.

Comment 18

[Brian R. Bondy [:bbondy]]

I just thought it would lead to a smaller chance of introducing regressions in untested changed code.

Comment 19

[Andres Hernandez [:andreshm]]

I'll update the patch. 

Btw, Brian I created the follow up bug 792489 and bug 792499.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #18)
> I just thought it would lead to a smaller chance of introducing regressions
> in untested changed code.

I understand, but this really doesn't make sense especially if you wouldn't even use 'this' in the callback's parent scope because there's no useful host object. Also, there are many more ways to screw such changes up, so I hope you test these code paths manually anyway.

Comment 21

[Brian R. Bondy [:bbondy]]

k cool

Comment 22

[Andres Hernandez [:andreshm]]

Attachment: Patch v5

Removed unnecessary .bind(this) and fixed an issue on style editor.

Comment 23

[Andres Hernandez [:andreshm]]

Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=9df34553f36b

Comment 24

[Brian R. Bondy [:bbondy]]

Comment on attachment 662738 [details] [diff] [review]
Patch v5

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

Thanks! Sorry for suggesting bind(this) for everything, and the uuid, but thanks for fixing it.

Comment 25

[Andres Hernandez [:andreshm]]

Try finish with some intermittent oranges. 
See: https://tbpl.mozilla.org/?tree=Try&rev=9df34553f36b

Brian, please confirm is ok to land it.

Comment 26

[Brian R. Bondy [:bbondy]]

Are you sure no other tests touch this code?

Comment 27

[Brian R. Bondy [:bbondy]]

I would prefer all tests to be run

Comment 28

[Andres Hernandez [:andreshm]]

Ok, I run it again on try, with all tests. 

See: https://tbpl.mozilla.org/?tree=Try&rev=f593cbf912a1

Let's wait for results.

Comment 29

[Brian R. Bondy [:bbondy]]

It's just possible that there's a missing brace for example or something like that and then who knows what will break.

Comment 30

[Andres Hernandez [:andreshm]]

All green :) https://tbpl.mozilla.org/?tree=Try&rev=dcd2b2cf14ce

Comment 31

[Brian R. Bondy [:bbondy]]

By the way, you don't need my approval after the patch is r+.  You can push it to mozilla-inbound, or if you can't push yet, just add the keyword: checkin-needed

Thanks again for the patch!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b22c10ecf390

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390

Comment 34

[Rob Campbell [:rc] (:robcee)]

There are a number of places in browser that this change has broken the ability to save a file.

devtools/scratchpad,
devtools/styleeditor,
about:memory (measure and save dialog).

Filed bug 865803 to track this.

Comment 35

[Rob Campbell [:rc] (:robcee)]

ignore the above. Looks like it's a more recent change.