Get rid of nsIFilePicker.show() use in gecko

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

49 Branch
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Posted patch mock.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8831609 - Flags: review?(bugs)
Comment on attachment 8831609 [details] [diff] [review]
mock.patch

This is really all in firefox UI code.

Perhaps Gijs could help here?

But to ease reviewing, -w patch would be nice.
Attachment #8831609 - Flags: review?(bugs) → review?(gijskruitbosch+bugs)
Posted patch mock.patch (obsolete) — Splinter Review
Attachment #8831609 - Attachment is obsolete: true
Attachment #8831609 - Flags: review?(gijskruitbosch+bugs)
Attachment #8831664 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8831664 [details] [diff] [review]
mock.patch

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

Please could you attach a patch -w or use mozreview? The latter would be helpful anyway because it allows for more context. Given that we're changing sync code to async code, more context is really valuable for understanding if there'll now be unintended side-effects given things will run to completion while the dialog was up where they didn't before.

::: toolkit/components/printing/content/printdialog.js
@@ +406,5 @@
>      fp.init(window, dialog.fpDialog.getAttribute("label"), nsIFilePicker.modeSave);
>      fp.appendFilters(nsIFilePicker.filterAll);
> +    fp.open(rv => {
> +      if (rv != Components.interfaces.nsIFilePicker.returnCancel &&
> +          fp.file && fp.file.path) {

This looks like the promise + callbacks etc. will just stay in memory forever if the user clicks cancel, potentially leaking things and so on. Can we actually handle cancel or not having a file instead?
Attachment #8831664 - Flags: review?(gijskruitbosch+bugs)
Attachment #8831664 - Attachment is obsolete: true
Attachment #8831673 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8831673 [details]
Bug 1334975 - Get rid of nsIFilePicker.show() use in gecko,

https://reviewboard.mozilla.org/r/108218/#review109276

I'm confused by the inline settings changes. Those might not be add-on compatible... Can you explain more about why those are necessary?

The devtools changes should ideally be reviewed by :ochameau or another devtools peer.

::: devtools/client/netmonitor/har/har-exporter.js:83
(Diff revision 1)
>      options.compress = Services.prefs.getBoolPref(
>        "devtools.netmonitor.har.compress");
>  
>      // Get target file for exported data. Bail out, if the user
>      // presses cancel.
> -    let file = HarUtils.getTargetFile(options.defaultFileName,
> +    HarUtils.getTargetFile(options.defaultFileName, options.jsonp,

This used to return a promise and now it doesn't. I bet that breaks tests, and maybe other things.

::: devtools/client/webide/content/simulator.js:292
(Diff revision 1)
> -            let file = utils.getCustomBinary(window);
> +            utils.getCustomBinary(window).then(file => {
> -            if (file) {
> +              if (file) {
> -              this.version = file.path;
> +                this.version = file.path;
> -            }
> +              }
> -            // Whatever happens, don't stay on the "pick" option.
> +              // Whatever happens, don't stay on the "pick" option.
> -            this.updateVersionSelector();
> +              this.updateVersionSelector();
> +            });

The "whatever happens" thing looks like there might be things that expect this to be sync. :-(

Getting :ochameau to review this bit (and maybe other devtools parts) would be a good idea. Though I'm not sure why we still ship this.

::: layout/tools/layout-debug/ui/content/layoutdebug.js:271
(Diff revision 1)
>        fp.init(window, "New Regression Test List", nsIFilePicker.modeOpen);
>        fp.appendFilters(nsIFilePicker.filterAll);
>        fp.defaultString = "rtest.lst";
> -      if (fp.show() != nsIFilePicker.returnOK)
> +      fp.open(rv => {
> +        if (rv != nsIFilePicker.returnOK)
>          return;

Nit: indentation.

::: security/manager/pki/resources/content/certManager.js:335
(Diff revision 1)
>            bundle.getString("chooseP12BackupFileDialog"),
>            nsIFilePicker.modeSave);
>    fp.appendFilter(bundle.getString("file_browse_PKCS12_spec"),
>                    "*.p12");
>    fp.appendFilters(nsIFilePicker.filterAll);
> -  var rv = fp.show();
> +  fp.open(rv => {

Here and elsewhere in this file... this picker can be  opened from a frame in about:preferences, and it seems that closing the tab doesn't automatically kill the picker. We should make sure that the behaviour is sane even if the user tries to use the picker after the tab is closed.

::: toolkit/mozapps/extensions/content/setting.xml:63
(Diff revision 1)
> +            this.dispatchEvent(event);
> +
> +            if (!body) {
> +              return;
> +            }

Why this change? It looks unrelated.
Attachment #8831673 - Flags: review?(gijskruitbosch+bugs)
> > +            if (!body) {
> > +              return;
> > +            }
> 
> Why this change? It looks unrelated.

I need to propagate an event. If the body is not defined, the event is not dispatched. Actually this method is called FireEvent, but it doesn't fire any event: it runs a callback code.
(In reply to Andrea Marchesini [:baku] from comment #7)
> > > +            if (!body) {
> > > +              return;
> > > +            }
> > 
> > Why this change? It looks unrelated.
> 
> I need to propagate an event. If the body is not defined, the event is not
> dispatched. Actually this method is called FireEvent, but it doesn't fire
> any event: it runs a callback code.

But why do we suddenly need the event? You added the handlers in the test code for this, but this never used to fire an event, so I'm confused why it needs to do so now.
Flags: needinfo?(amarchesini)
> But why do we suddenly need the event? You added the handlers in the test
> code for this, but this never used to fire an event, so I'm confused why it
> needs to do so now.

I need the event in order to know when the FilePicker has been processed. Before everything was sync, so the code was:

-      EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
       is(input.value, testFile.path, "Label value should match file chosen");

now this is not possible. We need to wait until MockFilePicker (or the real picker) completes the open().
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #10)
> > But why do we suddenly need the event? You added the handlers in the test
> > code for this, but this never used to fire an event, so I'm confused why it
> > needs to do so now.
> 
> I need the event in order to know when the FilePicker has been processed.
> Before everything was sync, so the code was:
> 
> -      EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 },
> gManagerWindow);
>        is(input.value, testFile.path, "Label value should match file
> chosen");
> 
> now this is not possible. We need to wait until MockFilePicker (or the real
> picker) completes the open().

This problem is test-only, right? So then, can we add a test-only MockFilePicker API that works for this usecase, rather than modifying this other code?
Flags: needinfo?(amarchesini)
> This problem is test-only, right? So then, can we add a test-only
> MockFilePicker API that works for this usecase, rather than modifying this
> other code?

This bug is needed for bug 1332003. We need to make the nsIFilePicker async. I don't want to write here too many details, but the Blobs must be created correctly and with a sync method this is impossible.
Blocks: 1332003
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #12)
> > This problem is test-only, right? So then, can we add a test-only
> > MockFilePicker API that works for this usecase, rather than modifying this
> > other code?
> 
> This bug is needed for bug 1332003. We need to make the nsIFilePicker async.
> I don't want to write here too many details, but the Blobs must be created
> correctly and with a sync method this is impossible.

Yes, I'm aware of that.

Is there an actual problem with the behaviour of the inline settings after making the API here async, or is the only problem with the automated test? That's the main issue here - if, as I believe, the problem is only with the test, I'd prefer to e.g. add a callback/eventing system to the mock file picker than adding real event dispatch to the dependent component, just to fix an automated test. If there is actual problem with the behaviour of the inline settings after making the API async, then I don't follow how the events are fixing them - you seem to only listen for them in the automated test, so there will be no difference to their behaviour 'in the wild'.
Flags: needinfo?(amarchesini)
> Yes, I'm aware of that.

Cool. Thanks for the advice. I changed my patch.
Flags: needinfo?(amarchesini)
Comment on attachment 8831673 [details]
Bug 1334975 - Get rid of nsIFilePicker.show() use in gecko,

Gijs, do you mind to take a look, if you have time?
Attachment #8831673 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8831673 [details]
Bug 1334975 - Get rid of nsIFilePicker.show() use in gecko,

https://reviewboard.mozilla.org/r/108218/#review113088

The non-devtools changes here look OK to me. I assume :ochameau can review the devtools bits.
Attachment #8831673 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8831673 [details]
Bug 1334975 - Get rid of nsIFilePicker.show() use in gecko,

https://reviewboard.mozilla.org/r/108218/#review115364

Sorry for the delay.

Should be good for /devtools/ with fixes made to HarExporter (you will have to rebase, your changes doesn't apply on latest m-c)

::: devtools/client/netmonitor/har/har-exporter.js
(Diff revision 6)
>  
>      // Get target file for exported data. Bail out, if the user
>      // presses cancel.
> -    let file = HarUtils.getTargetFile(options.defaultFileName,
> -      options.jsonp, options.compress);
> +    return HarUtils.getTargetFile(options.defaultFileName, options.jsonp,
> +      options.compress).then(file => {
> -

Looks like this code isn't covered by tests...
Here you expect HarUtils.getTargetFile returns a promise whereas it doesn't, it returns null and expect a callback as 4th argument... You should make it return a promise that looks like the best option.

::: devtools/client/netmonitor/har/har-exporter.js:86
(Diff revision 6)
>      // presses cancel.
> -    let file = HarUtils.getTargetFile(options.defaultFileName,
> -      options.jsonp, options.compress);
> +    return HarUtils.getTargetFile(options.defaultFileName, options.jsonp,
> +      options.compress).then(file => {
> -
> -    if (!file) {
> +        if (!file) {
> -      return resolve();
> +          return resolve();

Then, here, you can just do `return;` as you are in a promise callback, there is no need to create a promise object.
Attachment #8831673 - Flags: review?(poirot.alex) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/597004bec637
Get rid of nsIFilePicker.show() use in gecko, r=Gijs, r=ochameau
If I read
https://hg.mozilla.org/integration/mozilla-inbound/rev/597004bec637#l24.12
correctly, you're not removing the show() function just yet, right? It's used all over the place in C-C so we really need some advance notice. We'll stop using, see bug 1341211.

When do you plan to remove the function entirely?
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a0da23ac05
Get rid of nsIFilePicker.show() use in gecko, r=Gijs, r=ochameau
Flags: needinfo?(amarchesini)
Backed out for frequent failures of test_chrome_ext_downloads_saveAs.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/10bf2b81234bcd8c6174f904bb99483a0fb523e6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c32c74847eb6564ec7d912b5dcef64264ba421e5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=79095030&repo=mozilla-inbound

08:22:52     INFO - TEST-START | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
08:22:52     INFO - TEST-INFO | started process screenshot
08:22:52     INFO - TEST-INFO | screenshot: exit 0
08:22:52     INFO - Buffered messages logged at 08:22:52
08:22:52     INFO - SpawnTask.js | Entering test test_downloads_saveAs
08:22:52     INFO - Extension loaded
08:22:52     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | downloads.download() works with saveAs 
08:22:52     INFO - Buffered messages finished
08:22:52     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | downloaded file is the correct size - got +0, expected 12
08:22:52     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:310:5
08:22:52     INFO - test_downloads_saveAs@chrome://mochitests/content/chrome/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:49:3
08:22:52     INFO - onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:69:15
08:22:52     INFO - promise callback*next@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:104:45
08:22:52     INFO - onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:73:7
08:22:52     INFO - promise callback*next@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:104:45
08:22:52     INFO - onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:73:7
08:22:52     INFO - promise callback*next@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:104:45
08:22:52     INFO - onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:73:7
08:22:52     INFO - co/<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:58:5
08:22:52     INFO - co@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:54:10
08:22:52     INFO - toPromise@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:122:60
08:22:52     INFO - next@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:103:19
08:22:52     INFO - onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:73:7
08:22:52     INFO - co/<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:58:5
08:22:52     INFO - co@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:54:10
08:22:52     INFO - add_task/<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:270:9
08:22:52     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:672:12
08:22:52     INFO - add_task@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:269:7
08:22:52     INFO - @chrome://mochitests/content/chrome/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:15:1
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea959a93ce4
Get rid of nsIFilePicker.show() use in gecko, r=ochameau
I had to back this out because it caused merge conflicts for me merging inbound to mozilla-central. Can you rebase your patch and reland it?
https://hg.mozilla.org/mozilla-central/rev/63c4e77cfe90e90d35d85e1a4821015823c4ecee
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed300efb2fa
Get rid of nsIFilePicker.show() use in gecko, r=ochameau
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/5ed300efb2fa
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1387800
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.