793682 - [OS.File] getCurrentDirectory/setCurrentDirectory for worker threads

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[WangJun]

Can I get assigned on this?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Sure, but I would like you to finish bug 793435 first.

Comment 3

[WangJun]

(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?

Comment 4

[WangJun]

Attachment: testing needed

testing is needed

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 6

[WangJun]

Attachment: new patch

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 671442 [details] [diff] [review]
new patch

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

Looks good, thanks.

Comment 8

[WangJun]

Attachment: testing is need for this patch.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=592888322bcb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to inbound. Please rebase.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: get/setCurrentDirectory for worker threads

Here we go.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Benjamin Smedberg]

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.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

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

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: get/setCurrentDirectory for worker threads

Comment 16

[David Teller [:Yoric] - still alive but not very active]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0916aa78e09

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d0916aa78e09