1218870 - typeof "foo" != "undefined" tests are always true

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Florian Quèze [:florian]]

|typeof "foo"| returns "string", so there tests in the osfile code that always pass:

/toolkit/components/osfile/modules/osfile_unix_allthreads.jsm
    line 31 -- } else if (typeof "module" != "undefined" && typeof "require" != "undefined") {

/toolkit/components/osfile/modules/osfile_win_allthreads.jsm
    line 31 -- } else if (typeof "module" != "undefined" && typeof "require" != "undefined") {

/toolkit/components/osfile/modules/ospath_unix.jsm
    line 28 -- } else if (typeof "module" == "undefined" || typeof "exports" == "undefined") {

/toolkit/components/osfile/modules/ospath_win.jsm
    line 37 -- } else if (typeof "module" == "undefined" || typeof "exports" == "undefined") {

Comment 1

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

The objective of this bug is to take the few lines mentioned in comment 0 and replace `typeof "foo"` with `typeof foo` (where `foo` is `module` and `exports`).

Comment 2

[Eduard Hanu]

Attachment: rev1 - Statement check changes

Hello there! Please find attached my patch.

Comment 3

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

Comment on attachment 8681647 [details] [diff] [review]
rev1 - Statement check changes

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

Thanks for the patch, it looks good to me. Could you just change the commit message to something along the lines of:

Bug 1218870 - Fix uses of typeof "foo" in OS.File;r=yoric

Let's see how it works on our tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35b9f232b192

Comment 4

[Eduard Hanu]

Attachment: rev2 - Fix uses of typeof "foo" in OS.File;r=yoric

Hello David! Thank you for your feedback. I have just updated the commit message accordingly.

Comment 5

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

Comment on attachment 8681659 [details] [diff] [review]
rev2 - Fix uses of typeof "foo" in OS.File;r=yoric

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

Looks good to me.
Also, the tests pass, so if you're ready, we can mark that code for landing.

Comment 6

[Eduard Hanu]

Awesome! Do I need to do anything else or that's it?

Comment 7

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

I just marked the code for landing. It should land shortly (we're currently experimenting with a new system, so I can't tell you exactly how long). You'll be recontacted in the unlikely case it causes a problem.

Are you interested in working on another mentored bug? For instance, would you be interested in bug 1218746?

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/20ec21d1280b

Comment 9

[Carsten Book [:Tomcat]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #7)
> I just marked the code for landing. It should land shortly (we're currently
> experimenting with a new system, so I can't tell you exactly how long).
> You'll be recontacted in the unlikely case it causes a problem.
> 
> Are you interested in working on another mentored bug? For instance, would
> you be interested in bug 1218746?

Landed as https://hg.mozilla.org/integration/fx-team/rev/20ec21d1280b (and no, i'm not the new system ;)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20ec21d1280b

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/20ec21d1280b

Comment 12

[Eduard Hanu]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #7)
> I just marked the code for landing. It should land shortly (we're currently
> experimenting with a new system, so I can't tell you exactly how long).
> You'll be recontacted in the unlikely case it causes a problem.
> 
> Are you interested in working on another mentored bug? For instance, would
> you be interested in bug 1218746?

I am happy I could help, David! Sure, I will have a look on this bug as well.