Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 18

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bbondy, Assigned: andreshm)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
No longer blocks: 731307
Depends on: 731307
(Reporter)

Updated

5 years ago
Summary: Use filepicker's showAsync() instead of the obsolete show() in /browser/* → Use filepicker's open() instead of the obsolete show() in /browser/*
(Reporter)

Comment 1

5 years ago
showAsync was renamed to open()

Updated

5 years ago
Whiteboard: [Snappy]
(Assignee)

Comment 2

5 years ago
I'll take a look at this.
Assignee: nobody → andres
Hardware: x86_64 → All
(Assignee)

Comment 3

5 years ago
Created attachment 660931 [details] [diff] [review]
Patch v1

Updated all browser/* filePicker.show to async filePicker.open.
Attachment #660931 - Flags: review?(netzen)
(Reporter)

Comment 4

5 years ago
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+
(Reporter)

Comment 5

5 years ago
-...in the dispatch file...
+...in the dispatch function...
(Assignee)

Comment 6

5 years ago
Created attachment 661379 [details] [diff] [review]
Patch v2

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

Comment 9

5 years ago
Created attachment 661826 [details] [diff] [review]
Patch v3 (browser)

Removed the UUID and test pilot changes.
Attachment #661379 - Attachment is obsolete: true
Attachment #661379 - Flags: review?(netzen)
Attachment #661826 - Flags: review?(netzen)
(Assignee)

Comment 10

5 years ago
Created attachment 661827 [details] [diff] [review]
Patch v3 (test pilot)

Test pilot part, not sure if it should be in this bug.
Attachment #661827 - Flags: review?(netzen)
(Assignee)

Updated

5 years ago
Attachment #661826 - Attachment description: Patch v3 → Patch v3 (browser)
(Reporter)

Comment 11

5 years ago
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+
(Reporter)

Comment 12

5 years ago
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).
(Reporter)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 662615 [details] [diff] [review]
Patch v4

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

Comment 15

5 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=503738a4061f
(Assignee)

Updated

5 years ago
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.
(Reporter)

Comment 18

5 years ago
I just thought it would lead to a smaller chance of introducing regressions in untested changed code.
(Assignee)

Comment 19

5 years ago
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.
(Reporter)

Comment 21

5 years ago
k cool
(Assignee)

Comment 22

5 years ago
Created attachment 662738 [details] [diff] [review]
Patch v5

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

Comment 23

5 years ago
Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=9df34553f36b
(Reporter)

Comment 24

5 years ago
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+
(Assignee)

Comment 25

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

Brian, please confirm is ok to land it.
(Reporter)

Comment 26

5 years ago
Are you sure no other tests touch this code?
(Reporter)

Comment 27

5 years ago
I would prefer all tests to be run
(Assignee)

Comment 28

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

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

Let's wait for results.
(Reporter)

Comment 29

5 years ago
It's just possible that there's a missing brace for example or something like that and then who knows what will break.
(Assignee)

Comment 30

5 years ago
All green :) https://tbpl.mozilla.org/?tree=Try&rev=dcd2b2cf14ce
(Reporter)

Comment 31

5 years ago
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!
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 793031
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22c10ecf390
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b22c10ecf390
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18

Updated

5 years ago
Blocks: 112134

Updated

5 years ago
Blocks: 796994
Blocks: 865803
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.