Last Comment Bug 726576 - Optimize calls to nsLocalFileWin's HasFileAttribute
: Optimize calls to nsLocalFileWin's HasFileAttribute
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Brian R. Bondy [:bbondy]
: Nathan Froyd [:froydnj]
Depends on:
  Show dependency treegraph
Reported: 2012-02-13 06:27 PST by Brian R. Bondy [:bbondy]
Modified: 2012-02-17 17:59 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1. (2.13 KB, patch)
2012-02-13 06:27 PST, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review
Patch v2. (2.13 KB, patch)
2012-02-17 06:20 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2012-02-13 06:27:17 PST
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.
Comment 1 2012-02-13 16:30:00 PST
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 ?
Comment 2 Brian R. Bondy [:bbondy] 2012-02-13 16:32:31 PST
> 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.
Comment 3 Brian R. Bondy [:bbondy] 2012-02-13 16:33:17 PST
and by we I mean me :)
Comment 4 Brian R. Bondy [:bbondy] 2012-02-17 06:20:12 PST
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+.
Comment 5 Brian R. Bondy [:bbondy] 2012-02-17 06:25:32 PST
Comment 6 Ed Morley [:emorley] 2012-02-17 17:59:25 PST

Note You need to log in before you can comment on or make changes to this bug.