838821 - don't use file.exists() when not necessary (browser/components)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

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

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

Drive-by yoink!

For nsBrowserContentHandler, we're going to remove that code anyways in bug 703377, and it essentially never runs anymore, so I would just leave it.

For applications.js, it seems like we should convert all of that code to OS.File, rather than fixing only this very specific piece of it.

The _SessionFile.jsm change looks fine.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Thank you Marco.

> For applications.js, it seems like we should convert all of that code to
> OS.File, rather than fixing only this very specific piece of it.

Can we apply this fix in this patch, and open a new bug to port everything to OS.File?

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 710964 [details] [diff] [review]
Patch v2

since Gavin started looking at this, let's fix requests properly!

Comment 5

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

Comment on attachment 710964 [details] [diff] [review]
Patch v2

I don't think it's worth touching the code only to change the exists() call, since the benefit is negligible (the isExecutable call right after it will do a stat as well).

It makes sense to remove uses of exists() in places where it's the only use of nsIFile (or where we can't yet get rid of the synchronous I/O entirely, as in the _SessionFile.jsm change here), but otherwise we should focus on porting to OS.File.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 7

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

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

Seems like Cr isn't defined in this file? Did you test this patch?

Comment 8

[Marco Castelluccio [:marco]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Comment on attachment 710971 [details] [diff] [review]
> Patch v3
> 
> Seems like Cr isn't defined in this file? Did you test this patch?

I've sent it to try, waiting for the results.

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Patch v4

Comment 10

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

Comment on attachment 711561 [details] [diff] [review]
Patch v4

Can you change the |return null| in the catch block to |return "";|? It doesn't make a difference in practice at the moment because the only callers just end up null checking, but it makes more sense semantically (there's no need to distinguish "nonexistent file" from "failed to read file" or "empty file").

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Patch v5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/272789a88835

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/272789a88835