Closed
Bug 780499
Opened 12 years ago
Closed 12 years ago
[OS.File] Refactor setPosition constants
Categories
(Toolkit Graveyard :: OS.File, enhancement)
Toolkit Graveyard
OS.File
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla17
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
Attachments
(2 files, 4 obsolete files)
2.45 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•12 years ago
|
||
Assignee: nobody → dteller
Attachment #649122 -
Flags: review?(nfroyd)
Assignee | ||
Updated•12 years ago
|
Summary: [OS.File] Refactor setPosition/getPosition constants → [OS.File] Refactor setPosition constants
Comment 2•12 years ago
|
||
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?
Assignee | ||
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
(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?
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Comment 8•12 years ago
|
||
(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. :)
Assignee | ||
Comment 9•12 years ago
|
||
Applied feedback, here we go.
Attachment #649122 -
Attachment is obsolete: true
Attachment #649122 -
Flags: review?(nfroyd)
Attachment #649584 -
Flags: review?(nfroyd)
Comment 10•12 years ago
|
||
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-
Assignee | ||
Comment 11•12 years ago
|
||
Here we go.
Attachment #649584 -
Attachment is obsolete: true
Attachment #650550 -
Flags: review?(nfroyd)
Assignee | ||
Comment 12•12 years ago
|
||
And now, with a test.
Attachment #650551 -
Flags: review?(nfroyd)
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
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+
Assignee | ||
Comment 15•12 years ago
|
||
(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
Assignee | ||
Comment 16•12 years ago
|
||
Applied feedback
Attachment #650550 -
Attachment is obsolete: true
Attachment #651337 -
Flags: review?(nfroyd)
Comment 17•12 years ago
|
||
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+
Assignee | ||
Comment 18•12 years ago
|
||
Gasp, these pesky newlines. Thanks :)
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #651337 -
Attachment is obsolete: true
Attachment #651418 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•12 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=87960bb79cd4
Comment 21•12 years ago
|
||
(In reply to David Rajchenbach Teller from comment #20) > Try run: https://tbpl.mozilla.org/?tree=Try&rev=87960bb79cd4 Green on Try. https://hg.mozilla.org/integration/mozilla-inbound/rev/033a6bdf9250 https://hg.mozilla.org/integration/mozilla-inbound/rev/e025a4eadeca
Flags: in-testsuite+
Keywords: checkin-needed
Comment 22•12 years ago
|
||
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
Updated•11 months ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•