Closed Bug 780499 Opened 12 years ago Closed 12 years ago

[OS.File] Refactor setPosition constants

Categories

(Toolkit Graveyard :: OS.File, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(2 files, 4 obsolete files)

The current manner in which setPosition/getPosition constants work is quite annoying for bug 777711, as the constants (that need to be known on the controller thread) have different values (known only to the worker threads) depending on the platform.

Let's fix this.
Assignee: nobody → dteller
Attachment #649122 - Flags: review?(nfroyd)
Summary: [OS.File] Refactor setPosition/getPosition constants → [OS.File] Refactor setPosition constants
Wait, so Const.* is not suitable for use on both the controller thread and the worker threads?  I skimmed the patches in bug 777711, but I don't see how this patch is making anything more convenient there.  Could you please elaborate?
Const.* is, but setPosition/getPosition constants do not use Const.*. They define OS.File.POS_START/OS.File.POS_END/OS.File.POS_CUR... with a hack that maps it to different values depending on the platform.

This patch is largely about removing that hack.
I may be dense, but I don't see the hack.  All I see is a definition of machine-independent named constants that happen to use machine-dependent values.  I don't see how this differs from, say, letting big-endian and little-endian subsystems provide different definitions for converting host byte order 32-bit values to network byte order in libc.  Or even what OS.File generally is doing by providing an API that acts sanely on both Unix-likes and Windows.

Tightening up the value of |whence| passed to the OS-specific setPosition function is admirable (though lseek should complain about invalid |whence| values; maybe Windows doesn't?), but permitting all sorts of values to be passed into setPosition strikes me as a bit too loose.
Well, I personally think it is a bit of a hack.

Regardless, the issue is that this is defined in osfile_{unix, win}_front.jsm, which are modules that, by definition, are not safe for inclusion in the main thread (at least not yet). I need a way of making these constants either platform-independent or available to the main thread.

I can think of the following solutions:
1. either define constants myself and map them to platform-specific constants during the application of setPosition; or
2. move the definition to osfile_{unix, win}_allthreads.jsm; or
3. create OS.Constants.whatever with these constants.

I am currently doing option 1. I do not like option 3., as this would involve a C++ patch for something that does not deserve it. If you prefer option 2., though, I have no issue about doing this. Code will be slightly more complex, but nothing intractable.
(In reply to David Rajchenbach Teller from comment #5)
> Regardless, the issue is that this is defined in osfile_{unix,
> win}_front.jsm, which are modules that, by definition, are not safe for
> inclusion in the main thread (at least not yet). I need a way of making
> these constants either platform-independent or available to the main thread.

Thank you for the explanation!

> I can think of the following solutions:
> 1. either define constants myself and map them to platform-specific
> constants during the application of setPosition; or
> 2. move the definition to osfile_{unix, win}_allthreads.jsm; or
> 3. create OS.Constants.whatever with these constants.

I think option 2 is preferable to option 1.

What is the File.prototype.setPosition.foo business for in the current patch?
(In reply to Nathan Froyd (:froydnj) from comment #6)
> Thank you for the explanation!

Thanks for pushing :)

> I think option 2 is preferable to option 1.

Ok, I will do that.

> What is the File.prototype.setPosition.foo business for in the current patch?

Currently, we write:

file.setPosition(..., OS.File.POS_END);

I was toying with the idea of rather writing:

file.setPosition(..., file.setPosition.POS_END);

Not sure it is a good idea, though.
(In reply to David Rajchenbach Teller from comment #7)
> > What is the File.prototype.setPosition.foo business for in the current patch?
> 
> Currently, we write:
> 
> file.setPosition(..., OS.File.POS_END);
> 
> I was toying with the idea of rather writing:
> 
> file.setPosition(..., file.setPosition.POS_END);
> 
> Not sure it is a good idea, though.

Hm, yeah, a little too clever.  More typing, too. :)
Applied feedback, here we go.
Attachment #649122 - Attachment is obsolete: true
Attachment #649122 - Flags: review?(nfroyd)
Attachment #649584 - Flags: review?(nfroyd)
Comment on attachment 649584 [details] [diff] [review]
Refactoring setPosition constants, v2.

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

That seemed even simpler than v1.  Several bits and pieces to clean up, though.  It is also clear that we need setPosition tests somewhere. :)

::: toolkit/components/osfile/osfile_unix_allthreads.jsm
@@ +127,5 @@
> +  // Special constants that need to be defined on all platforms
> +
> +  exports.OS.Shared.POS_START = exports.OS.Constants.libc.SEEK_SET;
> +  exports.OS.Shared.POS_CURRENT = exports.OS.Constants.libc.SEEK_CUR;
> +  exports.OS.Shared.POS_END = exports.OS.Constants.libc.SEEK_END;

Since these are intended to be constants, let's use defineProperty for them.

Did you mean to define these in OS.Shared?  All the documentation and whatnot refers to them in OS.File.  Fix whichever needs fixing, please.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +144,5 @@
> +        * @param {number=} whence The reference position. If omitted
> +        * or |OS.File.POS_START|, |pos| is taken from the start of the
> +        * file.  If |OS.File.POS_CURRENT|, |pos| is taken from the
> +        * current position in the file. If |OS.File.POS_END|, |pos| is
> +        * taken from the end of the file.

I think "|pos| is relative to ..." is clearer throughout the description of |whence|.

@@ +150,5 @@
>          * @return The new position in the file.
>          */
>         setPosition: function setPosition(pos, whence) {
> +         if (whence === undefined) {
> +           whence = OS.File.POS_START;

I think you want OS.Constants.libc.SEEK_SET here.

::: toolkit/components/osfile/osfile_win_allthreads.jsm
@@ +137,5 @@
> +  // Special constants that need to be defined on all platforms
> +
> +  exports.OS.Shared.POS_START = exports.OS.Constants.libc.FILE_BEGIN;
> +  exports.OS.Shared.POS_CURRENT = exports.OS.Constants.libc.FILE_CURRENT;
> +  exports.OS.Shared.POS_END = exports.OS.Constants.libc.FILE_END;

Use defineProperty for these.  And I believe you meant these to be exports.OS.Constants.Win.* or Const.*, as you prefer.

@@ +138,5 @@
> +
> +  exports.OS.Shared.POS_START = exports.OS.Constants.libc.FILE_BEGIN;
> +  exports.OS.Shared.POS_CURRENT = exports.OS.Constants.libc.FILE_CURRENT;
> +  exports.OS.Shared.POS_END = exports.OS.Constants.libc.FILE_END;
> +

Delete this extra newline.

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +171,4 @@
>          * |OS.File.POS_START|, |pos| is taken from the start of the file.
>          * If |OS.File.POS_CURRENT|, |pos| is taken from the current position
>          * in the file. If |OS.File.POS_END|, |pos| is taken from the end of
>          * the file.

Since you will be touching this comment anyway, please use "|pos| is relative to..." here as well.
Attachment #649584 - Flags: review?(nfroyd) → review-
Here we go.
Attachment #649584 - Attachment is obsolete: true
Attachment #650550 - Flags: review?(nfroyd)
And now, with a test.
Attachment #650551 - Flags: review?(nfroyd)
Comment on attachment 650551 [details] [diff] [review]
Companion testsuite

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +293,5 @@
> +  ok("POS_CURRENT" in OS.File, "test_position: POS_CURRENT exists");
> +  ok("POS_END" in OS.File, "test_position: POS_END exists");
> +
> +  let ARBITRARY_POSITION = 321;
> +  let src_file_name = "chrome/toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js";

Nit: please make this the test file itself; that way we know it exists and don't have to worry about platform differences.

@@ +295,5 @@
> +
> +  let ARBITRARY_POSITION = 321;
> +  let src_file_name = "chrome/toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js";
> +
> +

Nit: delete this whitespace, please.
Attachment #650551 - Flags: review?(nfroyd) → review+
Comment on attachment 650550 [details] [diff] [review]
Refactoring setPosition constants, v3.

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

::: toolkit/components/osfile/osfile_unix_allthreads.jsm
@@ +127,5 @@
> +  // Special constants that need to be defined on all platforms
> +
> +   Object.defineProperty(exports.OS.Shared, "POS_START", { value: exports.OS.Constants.libc.SEEK_SET });
> +   Object.defineProperty(exports.OS.Shared, "POS_CURRENT", { value: exports.OS.Constants.libc.SEEK_CUR });
> +   Object.defineProperty(exports.OS.Shared, "POS_END", { value: exports.OS.Constants.libc.SEEK_END });

In comment 5, you mentioned that osfile_*_front.jsm were not (yet) suitable for inclusion on the main thread.  Are we going to make them so at some point, and then these used-only-for-transporting-constants-between-threads definitions can go away?

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +810,4 @@
>       File.Unix = exports.OS.Unix.File;
>       File.Error = exports.OS.Shared.Unix.Error;
>       exports.OS.File = File;
> +     exports.OS.Path = exports.OS.Unix.Path;

This isn't part of this series.  Please remove it.  (I'm hoping this should be inserted in a not-yet checked in one of your patches, given that the Windows version of this is already there...)

::: toolkit/components/osfile/osfile_win_allthreads.jsm
@@ +138,5 @@
> +
> +  Object.defineProperty(exports.OS.Shared, "POS_START", { value: exports.OS.Constants.Win.FILE_BEGIN });
> +  Object.defineProperty(exports.OS.Shared, "POS_CURRENT", { value: exports.OS.Constants.Win.FILE_CURRENT });
> +  Object.defineProperty(exports.OS.Shared, "POS_END", { value: exports.OS.Constants.Win.FILE_END });
> +

Nit: delete extra newline, please.

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +819,5 @@
> +
> +     Object.defineProperty(File, "POS_START", { value: OS.Shared.POS_START });
> +     Object.defineProperty(File, "POS_CURRENT", { value: OS.Shared.POS_CURRENT });
> +     Object.defineProperty(File, "POS_END", { value: OS.Shared.POS_END });
> +

Nit: delete extra newline, please.
Attachment #650550 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #14)
> Comment on attachment 650550 [details] [diff] [review]
> Refactoring setPosition constants, v3.
> 
> Review of attachment 650550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_unix_allthreads.jsm
> @@ +127,5 @@
> > +  // Special constants that need to be defined on all platforms
> > +
> > +   Object.defineProperty(exports.OS.Shared, "POS_START", { value: exports.OS.Constants.libc.SEEK_SET });
> > +   Object.defineProperty(exports.OS.Shared, "POS_CURRENT", { value: exports.OS.Constants.libc.SEEK_CUR });
> > +   Object.defineProperty(exports.OS.Shared, "POS_END", { value: exports.OS.Constants.libc.SEEK_END });
> 
> In comment 5, you mentioned that osfile_*_front.jsm were not (yet) suitable
> for inclusion on the main thread.  Are we going to make them so at some
> point, and then these used-only-for-transporting-constants-between-threads
> definitions can go away?

That's not planned.

> 
> ::: toolkit/components/osfile/osfile_unix_front.jsm
> @@ +810,4 @@
> >       File.Unix = exports.OS.Unix.File;
> >       File.Error = exports.OS.Shared.Unix.Error;
> >       exports.OS.File = File;
> > +     exports.OS.Path = exports.OS.Unix.Path;
> 
> This isn't part of this series.  Please remove it.  (I'm hoping this should
> be inserted in a not-yet checked in one of your patches, given that the
> Windows version of this is already there...)

Oops, sorry.

> 
> ::: toolkit/components/osfile/osfile_win_allthreads.jsm
> @@ +138,5 @@
> > +
> > +  Object.defineProperty(exports.OS.Shared, "POS_START", { value: exports.OS.Constants.Win.FILE_BEGIN });
> > +  Object.defineProperty(exports.OS.Shared, "POS_CURRENT", { value: exports.OS.Constants.Win.FILE_CURRENT });
> > +  Object.defineProperty(exports.OS.Shared, "POS_END", { value: exports.OS.Constants.Win.FILE_END });
> > +
> 
> Nit: delete extra newline, please.

ok

> 
> ::: toolkit/components/osfile/osfile_win_front.jsm
> @@ +819,5 @@
> > +
> > +     Object.defineProperty(File, "POS_START", { value: OS.Shared.POS_START });
> > +     Object.defineProperty(File, "POS_CURRENT", { value: OS.Shared.POS_CURRENT });
> > +     Object.defineProperty(File, "POS_END", { value: OS.Shared.POS_END });
> > +
> 
> Nit: delete extra newline, please.

ok
Applied feedback
Attachment #650550 - Attachment is obsolete: true
Attachment #651337 - Flags: review?(nfroyd)
Comment on attachment 651337 [details] [diff] [review]
Refactoring setPosition constants, v4.

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

::: toolkit/components/osfile/osfile_win_allthreads.jsm
@@ +138,5 @@
> +
> +  Object.defineProperty(exports.OS.Shared, "POS_START", { value: exports.OS.Constants.Win.FILE_BEGIN });
> +  Object.defineProperty(exports.OS.Shared, "POS_CURRENT", { value: exports.OS.Constants.Win.FILE_CURRENT });
> +  Object.defineProperty(exports.OS.Shared, "POS_END", { value: exports.OS.Constants.Win.FILE_END });
> +

Nit: remove newline.
Attachment #651337 - Flags: review?(nfroyd) → review+
Gasp, these pesky newlines.

Thanks :)
https://hg.mozilla.org/mozilla-central/rev/033a6bdf9250
https://hg.mozilla.org/mozilla-central/rev/e025a4eadeca
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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: