Optimize calls to nsLocalFileWin's HasFileAttribute

RESOLVED FIXED in mozilla13

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla13
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.13 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 596670 [details] [diff] [review]
Patch v1.

HasFileAttribute does 2 calls to get the file attributes, one from ResolveAndStat and the other from GetFileAttributesW right after it.
This can be optimized to do only one by using Resolve instead.

Also this function doesn't check for INVALID_FILE_ATTRIBUTES so if ResolveAndStat was not dirty, and the state changed so the file got deleted, this function would return incorrect results before. 

Also the code in IsFile and IsDirectory can be simplified as Neil suggested previously to use HasFileAttribute instead.
Attachment #596670 - Flags: review?(neil)

Comment 1

5 years ago
Comment on attachment 596670 [details] [diff] [review]
Patch v1.

>-  if (INVALID_FILE_ATTRIBUTES == attributes) {
Bah, I hadn't noticed that you like your comparisons the wrong way around ;-)

>+    DWORD attributes = GetFileAttributesW(mResolvedPath.get());
::GetFileAttributesW ?
Attachment #596670 - Flags: review?(neil) → review+
(Assignee)

Comment 2

5 years ago
> Bah, I hadn't noticed that you like your comparisons the wrong way around ;-)

I'm pretty indifferent about it but I've been asked to switch it after some review comments so I just start off that way usually now :)

> ::GetFileAttributesW ?

I think we're moving away from ::, but if you want I can do it.
(Assignee)

Comment 3

5 years ago
and by we I mean me :)
(Assignee)

Comment 4

5 years ago
Created attachment 598221 [details] [diff] [review]
Patch v2.

Returned converted windows error code instead of always returning invalid path since it was causing try failures. Carrying forward r+.
Attachment #596670 - Attachment is obsolete: true
Attachment #598221 - Flags: review+
(Assignee)

Comment 5

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/52917f8cfc41
https://hg.mozilla.org/mozilla-central/rev/52917f8cfc41
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.