Closed Bug 799226 Opened 12 years ago Closed 12 years ago

Optimize |OS.File.exists|

Categories

(Toolkit Graveyard :: OS.File, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla19

People

(Reporter: andreshm, Assigned: andreshm)

References

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 file, 2 obsolete files)

Optimize |OS.File.exists| using e.g. |access()| and create tests.
Whiteboard: [mentor=Yoric][lang=js]
Attached patch Patch v1 (obsolete) — Splinter Review
I'm not sure of the windows solution, I can't test it locally. 

Unix solutions works fine.
Attachment #670076 - Flags: review?(dteller)
Comment on attachment 670076 [details] [diff] [review]
Patch v1

Review of attachment 670076 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Could you please apply the following changes?

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +267,5 @@
> +     File.exists = function Unix_exists(path) {
> +       try {
> +         throw_on_negative("exists",
> +           UnixFile.access(path, OS.Constants.libc.F_OK)
> +         );

I believe it would be simpler and faster to just check if |UnixFile.access(...)| returns -1, rather than introducing this exception block.

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +323,5 @@
> +      * @param {string} path The path to the file.
> +      *
> +      * @return {bool} true if the file exists, false otherwise.
> +      */
> +     File.exists = function Unix_exists(path) {

Win_exists?

@@ +325,5 @@
> +      * @return {bool} true if the file exists, false otherwise.
> +      */
> +     File.exists = function Unix_exists(path) {
> +       try {
> +         throw_on_null("exists", WinFile.FindFirstFile(path, gFindDataPtr));

I believe that, for Windows, you should just use File.prototype.open, as in the non-optimized version.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +977,5 @@
> +      test.ok(aExists, "file exists");
> +      return OS.File.exists(EXISTING_FILE + ".tmp");
> +    });
> +
> +    promise = promise.then(function exists_not_worked(aExists) {

I would rather call this |exists_on_absent_file_worked|.
Attachment #670076 - Flags: review?(dteller) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Applied changes.
Attachment #670076 - Attachment is obsolete: true
Attachment #670446 - Flags: review?(dteller)
Comment on attachment 670446 [details] [diff] [review]
Patch v2

Review of attachment 670446 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following trivial change

Thanks again for one more nice patch :)

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +778,5 @@
> + * Test the file |exists| method.
> + */
> +function test_exists_file()
> +{
> +  let file_name = "chrome/toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js";

I would prefer:

OS.Path.join("chrome", "toolkit", "components" ,"osfile", "tests", "mochi", "test_osfile_front.xul");
Attachment #670446 - Flags: review?(dteller) → review+
Attachment #670446 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a25b864146d1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: