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

RESOLVED FIXED in Firefox 21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

(Blocks 1 bug)

20 Branch
Firefox 21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 4 obsolete attachments)

No description provided.
Posted patch Patch (obsolete) — Splinter Review
Attachment #710951 - Flags: review?(dietrich)
Attachment #710951 - Flags: review?(dietrich) → review?(mak77)
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.
Attachment #710951 - Flags: review?(mak77) → review-
Posted patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #710951 - Attachment is obsolete: true
Attachment #710964 - Flags: review?(mak77)
Comment on attachment 710964 [details] [diff] [review]
Patch v2

since Gavin started looking at this, let's fix requests properly!
Attachment #710964 - Flags: review?(mak77) → review?(gavin.sharp)
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.
Attachment #710964 - Flags: review?(gavin.sharp) → review-
Posted patch Patch v3 (obsolete) — Splinter Review
Attachment #710964 - Attachment is obsolete: true
Attachment #710971 - Flags: review?(gavin.sharp)
Comment on attachment 710971 [details] [diff] [review]
Patch v3

Seems like Cr isn't defined in this file? Did you test this patch?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Posted patch Patch v4 (obsolete) — Splinter Review
Attachment #710971 - Attachment is obsolete: true
Attachment #710971 - Flags: review?(gavin.sharp)
Attachment #711561 - Flags: review?(gavin.sharp)
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").
Attachment #711561 - Flags: review?(gavin.sharp) → review+
Posted patch Patch v5Splinter Review
Attachment #711561 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/272789a88835
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.