[OS.File] getCurrentDirectory/setCurrentDirectory for worker threads

RESOLVED FIXED in mozilla19

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: WangJun)

Tracking

unspecified
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

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.
(Assignee)

Comment 1

5 years ago
Can I get assigned on this?
Sure, but I would like you to finish bug 793435 first.
(Assignee)

Comment 3

5 years ago
(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?
Assignee: nobody → w1990ww1990w
(Assignee)

Comment 4

5 years ago
Created attachment 671415 [details] [diff] [review]
testing needed

testing is needed
Attachment #671415 - Flags: review?(dteller)
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+
(Assignee)

Updated

5 years ago
Attachment #671415 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 671442 [details] [diff] [review]
new patch
Attachment #671442 - Flags: review?(dteller)
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+
(Assignee)

Updated

5 years ago
Attachment #671442 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 671454 [details] [diff] [review]
testing is need for this patch.
Attachment #671454 - Flags: review+
This doesn't apply cleanly to inbound. Please rebase.
Keywords: checkin-needed
Created attachment 672200 [details] [diff] [review]
get/setCurrentDirectory for worker threads

Here we go.
Attachment #671454 - Attachment is obsolete: true
Attachment #672200 - Flags: review+
Keywords: checkin-needed
Summary: [OS.File] getCurrentDirectory/setCurrentDirectory for the main thread → [OS.File] getCurrentDirectory/setCurrentDirectory for worker threads
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

5 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.
(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.
Created attachment 672768 [details] [diff] [review]
get/setCurrentDirectory for worker threads
Attachment #672200 - Attachment is obsolete: true
Attachment #672768 - Flags: review+
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/d0916aa78e09
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.