Closed Bug 781973 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: bbondy, Assigned: andreshm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Snappy])

Attachments

(1 file, 5 obsolete files)

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.
No longer blocks: 731307
Depends on: 731307
Summary: Use filepicker's showAsync() instead of the obsolete show() in /browser/* → Use filepicker's open() instead of the obsolete show() in /browser/*
showAsync was renamed to open()
Whiteboard: [Snappy]
I'll take a look at this.
Assignee: nobody → andres
Hardware: x86_64 → All
Attached patch Patch v1 (obsolete) — Splinter Review
Updated all browser/* filePicker.show to async filePicker.open.
Attachment #660931 - Flags: review?(netzen)
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.
Attachment #660931 - Flags: review?(netzen) → feedback+
-...in the dispatch file...
+...in the dispatch function...
Attached patch Patch v2 (obsolete) — Splinter Review
Applied changes.
Attachment #660931 - Attachment is obsolete: true
Attachment #661379 - Flags: review?(netzen)
You don't need to change an interface's IID to mark it "function", since that doesn't affect binary compatibility.
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).
Attached patch Patch v3 (browser) (obsolete) — Splinter Review
Removed the UUID and test pilot changes.
Attachment #661379 - Attachment is obsolete: true
Attachment #661379 - Flags: review?(netzen)
Attachment #661826 - Flags: review?(netzen)
Attached patch Patch v3 (test pilot) (obsolete) — Splinter Review
Test pilot part, not sure if it should be in this bug.
Attachment #661827 - Flags: review?(netzen)
Attachment #661826 - Attachment description: Patch v3 → Patch v3 (browser)
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...
Attachment #661379 - Attachment is obsolete: false
Attachment #661379 - Flags: feedback+
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).
(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.
Attached patch Patch v4 (obsolete) — Splinter Review
Updated patch with suggested changes.
Attachment #661379 - Attachment is obsolete: true
Attachment #661826 - Attachment is obsolete: true
Attachment #661827 - Attachment is obsolete: true
Attachment #661826 - Flags: review?(netzen)
Attachment #661827 - Flags: review?(netzen)
Attachment #662615 - Flags: review?(netzen)
Blocks: 792489
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)?
(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Then add .bind(this) to everything

Please don't.
I just thought it would lead to a smaller chance of introducing regressions in untested changed code.
I'll update the patch. 

Btw, Brian I created the follow up bug 792489 and bug 792499.
(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.
k cool
Attached patch Patch v5Splinter Review
Removed unnecessary .bind(this) and fixed an issue on style editor.
Attachment #662615 - Attachment is obsolete: true
Attachment #662615 - Flags: review?(netzen)
Attachment #662738 - Flags: review?(netzen)
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.
Attachment #662738 - Flags: review?(netzen) → review+
Try finish with some intermittent oranges. 
See: https://tbpl.mozilla.org/?tree=Try&rev=9df34553f36b

Brian, please confirm is ok to land it.
Are you sure no other tests touch this code?
I would prefer all tests to be run
Ok, I run it again on try, with all tests. 

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

Let's wait for results.
It's just possible that there's a missing brace for example or something like that and then who knows what will break.
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!
Keywords: checkin-needed
Blocks: 793031
https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Blocks: 112134
Blocks: 796994
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.
ignore the above. Looks like it's a more recent change.
No longer blocks: 865803
You need to log in before you can comment on or make changes to this bug.