Closed Bug 1218870 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: OS.File, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: florian, Assigned: hanue, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 1 obsolete file)

|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") {
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`).
Mentor: dteller
Whiteboard: [lang=js][good first bug]
Attached patch rev1 - Statement check changes (obsolete) — Splinter Review
Hello there! Please find attached my patch.
Attachment #8681647 - Flags: review?(dteller)
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
Attachment #8681647 - Flags: review?(dteller) → review+
Hello David! Thank you for your feedback. I have just updated the commit message accordingly.
Attachment #8681659 - Flags: review?(dteller)
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.
Attachment #8681659 - Flags: review?(dteller) → review+
Attachment #8681647 - Attachment is obsolete: true
Assignee: nobody → hanue
Awesome! Do I need to do anything else or that's it?
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?
Keywords: checkin-needed
(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 ;)
https://hg.mozilla.org/mozilla-central/rev/20ec21d1280b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(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.
You need to log in before you can comment on or make changes to this bug.