Last Comment Bug 781973 - Use filepicker's open() instead of the obsolete show() in /browser/*
: Use filepicker's open() instead of the obsolete show() in /browser/*
Status: RESOLVED FIXED
[Snappy]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Andres Hernandez [:andreshm]
:
:
Mentors:
Depends on: 731307
Blocks: 112134 796994 792489 793031
  Show dependency treegraph
 
Reported: 2012-08-10 16:07 PDT by Brian R. Bondy [:bbondy]
Modified: 2013-04-26 10:13 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (67.20 KB, patch)
2012-09-13 12:33 PDT, Andres Hernandez [:andreshm]
netzen: feedback+
Details | Diff | Splinter Review
Patch v2 (67.26 KB, patch)
2012-09-14 15:07 PDT, Andres Hernandez [:andreshm]
netzen: feedback+
Details | Diff | Splinter Review
Patch v3 (browser) (58.74 KB, patch)
2012-09-17 09:45 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v3 (test pilot) (8.81 KB, patch)
2012-09-17 09:49 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v4 (58.60 KB, patch)
2012-09-19 11:08 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v5 (58.45 KB, patch)
2012-09-19 16:24 PDT, Andres Hernandez [:andreshm]
netzen: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2012-08-10 16:07:42 PDT
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] 2012-08-14 12:48:29 PDT
showAsync was renamed to open()
Comment 2 Andres Hernandez [:andreshm] 2012-09-11 09:36:52 PDT
I'll take a look at this.
Comment 3 Andres Hernandez [:andreshm] 2012-09-13 12:33:15 PDT
Created attachment 660931 [details] [diff] [review]
Patch v1

Updated all browser/* filePicker.show to async filePicker.open.
Comment 4 Brian R. Bondy [:bbondy] 2012-09-13 12:56:38 PDT
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] 2012-09-13 12:57:30 PDT
-...in the dispatch file...
+...in the dispatch function...
Comment 6 Andres Hernandez [:andreshm] 2012-09-14 15:07:48 PDT
Created attachment 661379 [details] [diff] [review]
Patch v2

Applied changes.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-14 23:33:10 PDT
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: gavin@gavinsharp.com] 2012-09-14 23:35:24 PDT
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] 2012-09-17 09:45:56 PDT
Created attachment 661826 [details] [diff] [review]
Patch v3 (browser)

Removed the UUID and test pilot changes.
Comment 10 Andres Hernandez [:andreshm] 2012-09-17 09:49:04 PDT
Created attachment 661827 [details] [diff] [review]
Patch v3 (test pilot)

Test pilot part, not sure if it should be in this bug.
Comment 11 Brian R. Bondy [:bbondy] 2012-09-17 10:19:16 PDT
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] 2012-09-17 10:21:55 PDT
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] 2012-09-17 10:29:11 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] 2012-09-19 11:08:39 PDT
Created attachment 662615 [details] [diff] [review]
Patch v4

Updated patch with suggested changes.
Comment 15 Andres Hernandez [:andreshm] 2012-09-19 11:09:30 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=503738a4061f
Comment 16 Dão Gottwald [:dao] 2012-09-19 11:14:14 PDT
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] 2012-09-19 11:15:21 PDT
(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] 2012-09-19 11:32:17 PDT
I just thought it would lead to a smaller chance of introducing regressions in untested changed code.
Comment 19 Andres Hernandez [:andreshm] 2012-09-19 11:39:07 PDT
I'll update the patch. 

Btw, Brian I created the follow up bug 792489 and bug 792499.
Comment 20 Dão Gottwald [:dao] 2012-09-19 11:48:16 PDT
(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] 2012-09-19 11:49:35 PDT
k cool
Comment 22 Andres Hernandez [:andreshm] 2012-09-19 16:24:24 PDT
Created attachment 662738 [details] [diff] [review]
Patch v5

Removed unnecessary .bind(this) and fixed an issue on style editor.
Comment 23 Andres Hernandez [:andreshm] 2012-09-19 16:24:59 PDT
Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=9df34553f36b
Comment 24 Brian R. Bondy [:bbondy] 2012-09-19 18:40:17 PDT
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] 2012-09-20 09:31:04 PDT
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] 2012-09-20 09:46:58 PDT
Are you sure no other tests touch this code?
Comment 27 Brian R. Bondy [:bbondy] 2012-09-20 09:47:49 PDT
I would prefer all tests to be run
Comment 28 Andres Hernandez [:andreshm] 2012-09-20 09:54:13 PDT
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] 2012-09-20 10:07:44 PDT
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] 2012-09-21 09:31:32 PDT
All green :) https://tbpl.mozilla.org/?tree=Try&rev=dcd2b2cf14ce
Comment 31 Brian R. Bondy [:bbondy] 2012-09-21 12:18:18 PDT
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] 2012-09-21 20:38:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22c10ecf390
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-09-22 05:41:50 PDT
https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390
Comment 34 Rob Campbell [:rc] (:robcee) 2013-04-26 09:47:03 PDT
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) 2013-04-26 10:13:16 PDT
ignore the above. Looks like it's a more recent change.

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