Closed
Bug 793682
Opened 12 years ago
Closed 12 years ago
[OS.File] getCurrentDirectory/setCurrentDirectory for worker threads
Categories
(Toolkit Graveyard :: OS.File, defect)
Toolkit Graveyard
OS.File
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla19
People
(Reporter: Yoric, Assigned: w1990ww1990w)
Details
(Whiteboard: [mentor=Yoric][lang=js])
Attachments
(1 file, 4 obsolete files)
8.16 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
Currently, in OS.File, on the main thread (osfile_{win, unix}_front.jsm), we use the |OS.File.curDir| getter/setter pair to get/set the current directory. On the worker thread (osfile_async_front.jsm), we use |OS.File.{getCurrentDirectory/setCurrentDirectory}|.
For harmonization purposes, we should deprecate |OS.File.curDir| and implement get/setCurrentDirectory on the main thread.
Reporter | ||
Comment 2•12 years ago
|
||
Sure, but I would like you to finish bug 793435 first.
(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Sure, but I would like you to finish bug 793435 first.
Is bug 793435 finished?
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → w1990ww1990w
testing is needed
Attachment #671415 -
Flags: review?(dteller)
Reporter | ||
Comment 5•12 years ago
|
||
Comment on attachment 671415 [details] [diff] [review]
testing needed
Review of attachment 671415 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, with minor fixes. Can you fix these? I will get this through the TryServer.
::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +857,5 @@
> set: function(path) {
> throw_on_negative("curDir",
> UnixFile.chdir(path)
> );
> },
You should avoid duplicating code. Now that we have File.{get, set}CurrentDir, curDir getter/setter should be reimplemented to call these functions - both under Unix and Windows.
::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +694,5 @@
> && change.getTime() <= stopMs,
> "test_info: file 2 has changed between the start of the test and now - " + start + ", " + stop + ", " + change);
>
> // Test OS.File.stat on directory
> + info = OS.File.stat(OS.File.getCurrentDirectory(););
Typo.
Attachment #671415 -
Flags: review?(dteller) → review+
Attachment #671415 -
Attachment is obsolete: true
Attachment #671442 -
Flags: review?(dteller)
Reporter | ||
Comment 7•12 years ago
|
||
Comment on attachment 671442 [details] [diff] [review]
new patch
Review of attachment 671442 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, thanks.
Attachment #671442 -
Flags: review?(dteller) → review+
Attachment #671442 -
Attachment is obsolete: true
Attachment #671454 -
Flags: review+
Reporter | ||
Comment 9•12 years ago
|
||
Keywords: checkin-needed
Comment 10•12 years ago
|
||
This doesn't apply cleanly to inbound. Please rebase.
Keywords: checkin-needed
Reporter | ||
Comment 11•12 years ago
|
||
Here we go.
Attachment #671454 -
Attachment is obsolete: true
Attachment #672200 -
Flags: review+
Reporter | ||
Updated•12 years ago
|
Keywords: checkin-needed
Summary: [OS.File] getCurrentDirectory/setCurrentDirectory for the main thread → [OS.File] getCurrentDirectory/setCurrentDirectory for worker threads
Comment 12•12 years ago
|
||
This still doesn't apply cleanly. Is your copy of inbound up to date?
applying 793682
patching file toolkit/components/osfile/osfile_unix_front.jsm
Hunk #1 FAILED at 826
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/osfile/osfile_unix_front.jsm.rej
patching file toolkit/components/osfile/osfile_win_front.jsm
Hunk #1 FAILED at 820
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/osfile/osfile_win_front.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 793682
Keywords: checkin-needed
Comment 13•12 years ago
|
||
Why do we support setting the current directory at all? I cannot think of a use case in product code where we'd want to allow that.
Reporter | ||
Comment 14•12 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #13)
> Why do we support setting the current directory at all? I cannot think of a
> use case in product code where we'd want to allow that.
I am sure that some people can think of use cases for B2G. Also, besides product code, I can also think of use cases for testing purposes.
Reporter | ||
Comment 15•12 years ago
|
||
Attachment #672200 -
Attachment is obsolete: true
Attachment #672768 -
Flags: review+
Reporter | ||
Updated•12 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 16•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #12)
> This still doesn't apply cleanly. Is your copy of inbound up to date?
My bad, it seems that it wasn't.
Comment 17•12 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 18•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•2 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•