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

RESOLVED FIXED in Firefox 21

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

(Blocks: 1 bug)

20 Branch
Firefox 21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 710951 [details] [diff] [review]
Patch
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-
(Assignee)

Comment 3

5 years ago
Created attachment 710964 [details] [diff] [review]
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?
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-
(Assignee)

Comment 6

5 years ago
Created attachment 710971 [details] [diff] [review]
Patch v3
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?
(Assignee)

Comment 8

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

Comment 9

5 years ago
Created attachment 711561 [details] [diff] [review]
Patch v4
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+
(Assignee)

Comment 11

5 years ago
Created attachment 711575 [details] [diff] [review]
Patch v5
Attachment #711561 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/272789a88835
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/272789a88835
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.