837989 - Use IOUtils for gLastOpenDirectory

Status: NEW

(Firefox :: General, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

Instead of checking for file existence on the main thread, we can use OS.File's stat.

The attached patch is just a draft, I'm still learning how promises work.
And I think there are some tests that I need to fix (because they expect an immediate result).

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 710007 [details] [diff] [review]
Patch

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

::: browser/base/content/browser.js
@@ +2171,5 @@
>    _lastDir: null,
>    get path() {
> +    return this._lastDir;
> +  },
> +  set path(val) {

Shouldn't |path| accept a path instead of a |nsIFile|?

@@ +2187,5 @@
> +        }
> +      },
> +      function onFailure(reason) {
> +        if (!(reason instanceof OS.File.Error && reason.becauseNoSuchFile)) {
> +          throw reason;

This won't report the error, ever.
You should add some form of error reporting here.

@@ +2193,4 @@
>        }
> +    );
> +  },
> +  loadPath: function() {

For improved readability, you may want to use Task.jsm. Your call.

@@ +2217,5 @@
> +  },
> +  checkExists: function() {
> +    let promise = OS.File.stat(this._lastDir.path);
> +    return promise;
> +  },

Method |checkExists| doesn't look very useful. I would just inline it.

@@ +2237,2 @@
>          }
> +

Nit: What's that empty line?

@@ +2250,5 @@
> +    promise.then(
> +      function onSuccess() {
> +        fp.displayDirectory = gLastOpenDirectory.path;
> +        fp.open(fpCallback);
> +      }

Could you add a comment to explain that you are ignoring errors deliberately?

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch v2

https://tbpl.mozilla.org/?tree=Try&rev=49599e3d30af

Comment 3

[Marco Castelluccio [:marco]]

Looks like my fix for the test doesn't work. The test is timing out.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 5

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

Comment on attachment 784458 [details] [diff] [review]
Patch v3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> var gLastOpenDirectory = {

>+  setPath: function(val) {

Can we just get rid of the isDirectory check, rather than forcing setPath to be synchronous? Having it not be synchronous seems like a footgun and probably overly complicates tests and some consumers.

Comment 6

[Marco Castelluccio [:marco]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> Comment on attachment 784458 [details] [diff] [review]
> Patch v3
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> > var gLastOpenDirectory = {
> 
> >+  setPath: function(val) {
> 
> Can we just get rid of the isDirectory check, rather than forcing setPath to
> be synchronous? Having it not be synchronous seems like a footgun and
> probably overly complicates tests and some consumers.

I didn't want to modify the behavior of the code, but I guess we could safely move the isDirectory check in getPath (so we don't even need to call exists). What do you say?

Comment 7

[Marco Castelluccio [:marco]]

The only problem is that if the directory doesn't exist, we don't want to set the pref.

Comment 8

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

I'm suggesting a change in behavior, yes - I was hoping you could tell me whether that would break anything :) What's the harm in setting the pref to a bogus value, if someone passes one in? What's the likelihood that anyone would do that - do we really need to defend against it?

Comment 9

[Marco Castelluccio [:marco]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> I'm suggesting a change in behavior, yes - I was hoping you could tell me
> whether that would break anything :) What's the harm in setting the pref to
> a bogus value, if someone passes one in? What's the likelihood that anyone
> would do that - do we really need to defend against it?

I think it's highly unlikely.
The only consumer is using a file picker and I think it's highly unlikely that the directory just chosen by the user would be removed in such a short time. Moreover between the setPath and getPath everything could happen, so there's already no guarantee that the file is a directory when getPath is called.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 784458 [details] [diff] [review]
Patch v3

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

::: browser/base/content/browser.js
@@ +1854,5 @@
>                        postData: aPostData,
>                        inBackground: false,
>                        allowThirdPartyFixup: aAllowThirdPartyFixup});
>  }
>  

Since you're improving that code, could you please the opportunity to add some documentation?

@@ +1862,5 @@
>    get path() {
> +    return this._lastDir;
> +  },
> +
> +  setPath: function(val) {

For my understanding: why did you replace |set path| with |setPath|?
Also, while you're here, some name more meaningful than |val| might be nice.
Also, since |val| is a |nsIFile|, |setDir| might be a better name.

@@ +1884,5 @@
> +    }.bind(this)).then(null, function onError(reason) {
> +      if (!(reason instanceof OS.File.Error && reason.becauseNoSuchFile)) {
> +        Cu.reportError("Checking for directory existence failed: " + reason);
> +      }
> +    });

Since you're already using Task.spawn, you should replace this onError with a use of try/catch, this will be easier to read.

@@ +1887,5 @@
> +      }
> +    });
> +  },
> +
> +  loadPath: function() {

It's not clear to me what |loadPath| intends to do, who should call it and when.

@@ +1893,5 @@
> +      if (!this._lastDir) {
> +        try {
> +          this._lastDir = gPrefService.getComplexValue("browser.open.lastDir",
> +                                                       Ci.nsIFile);
> +        } catch(e) {}

Please document this catch-all.

@@ +1897,5 @@
> +        } catch(e) {}
> +      }
> +
> +      if (this._lastDir) {
> +        let fileExists = yield OS.File.exists(this._lastDir.path);

So, if we are calling |stat| in |setPath|, why not here?

@@ +1923,4 @@
>            if (fp.file) {
> +            yield gLastOpenDirectory.setPath(fp.file
> +                                               .parent
> +                                               .QueryInterface(Ci.nsIFile));

This |yield| won't do anything interesting. What was your objective here?

@@ +1937,5 @@
> +
> +      yield gLastOpenDirectory.loadPath();
> +      fp.displayDirectory = gLastOpenDirectory.path;
> +      fp.open(fpCallback);
> +    } catch (ex) {}

Here, too, could you please document the catch-all?

Comment 11

[Marco Castelluccio [:marco]]

Attachment: gLastOpenDirectory_OS.File

I've changed the behavior of the object. Now all the checks are made in loadDir.

> 
> @@ +1923,4 @@
> >            if (fp.file) {
> > +            yield gLastOpenDirectory.setPath(fp.file
> > +                                               .parent
> > +                                               .QueryInterface(Ci.nsIFile));
> 
> This |yield| won't do anything interesting. What was your objective here?

I've removed it, but it was here to wait for the async operations executed by setPath (setPath returned a promise).

> @@ +1937,5 @@
> > +
> > +      yield gLastOpenDirectory.loadPath();
> > +      fp.displayDirectory = gLastOpenDirectory.path;
> > +      fp.open(fpCallback);
> > +    } catch (ex) {}
> 
> Here, too, could you please document the catch-all?

Actually I don't know why here we're ignoring errors.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 785451 [details] [diff] [review]
gLastOpenDirectory_OS.File

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

::: browser/base/content/browser.js
@@ +1874,5 @@
> +      }
> +    }
> +  },
> +
> +  // This function reads the last open directory from the browser.open.lastDir

Ok, but what is it for?
Also, nit: Remove "this function."

@@ +1884,5 @@
> +        // lastDir value.
> +        try {
> +          this._lastDir = gPrefService.getComplexValue("browser.open.lastDir",
> +                                                       Ci.nsIFile);
> +        } catch(e) {}

Please move your comment "if for some reason [...]" inside the {}.

@@ +1895,2 @@
>        try {
> +        let stat = yield OS.File.stat(this._lastDir.path);

I still don't quite understand the role of this function. Looks like you're doing this call to |stat| at each call to |loadDir|. Shouldn't you do it only if |this._lastDir| has been modified?

Also, would it be better/worse if |loadDir| were replaced by a preference observer and/or were part of |set dir| or |get dir|?

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_opendir.js
@@ +123,5 @@
>  
> +        // cleanup
> +        file.remove(false);
> +        finish();
> +      });

Why did you remove Test 5?

Comment 13

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> @@ +1895,2 @@
> >        try {
> > +        let stat = yield OS.File.stat(this._lastDir.path);
> 
> I still don't quite understand the role of this function. Looks like you're
> doing this call to |stat| at each call to |loadDir|. Shouldn't you do it
> only if |this._lastDir| has been modified?

We need to check if the directory is still there since the last time we've used it. The directory could also be removed while Firefox isn't running.

> :::
> browser/components/privatebrowsing/test/browser/
> browser_privatebrowsing_opendir.js
> @@ +123,5 @@
> >  
> > +        // cleanup
> > +        file.remove(false);
> > +        finish();
> > +      });
> 
> Why did you remove Test 5?

Because I changed the behavior of the object. Now we check if the directory exists and is a directory only when we load the pref, not when we save it (because it's highly unlikely that it's set to an non existent file, as it's set through a file picker).

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Any progress here?

Comment 15

[David Teller [:Yoric] - still alive but not very active]

In the absence of news, de-assigning.

Comment 16

[:Gijs (he/him)]

This code is only used when using the 'open file' dialog directly from the File menu, the 'open file' button, or via an app command (sent via the OS), so doesn't seem worth prioritizing higher than p3.

I'm unassigning Marco because given that it's been 5 years I assume he's not working on this at the moment. :-)