[Settings] Use `window` explicitly instead of `this` in IIFE

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: arthurcc, Assigned: arthurcc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

In strict mode functions "this" is actually "undefined" if it is not specified[1].

Following the work done in bug 1151105, we should pass `window` explicitly instead of `this` in IIFE. Scripts need update are listed as below. The scripts may not be wrapped in a strict mode function but they could be error-prone.

/apps/settings/js/activity_handler.js
/apps/settings/js/startup.js
/apps/settings/js/utils.js
/apps/settings/js/downloads/download_api_manager.js
/apps/settings/js/downloads/downloads_list.js

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode#.22Securing.22_JavaScript
Created attachment 8588939 [details] [review]
[gaia] crh0716:1151723 > mozilla-b2g:master
To add a bit more context, I think most of these files are fine as-is, as long as the code is not wrapped inside another function at some point. So, for the following:

    (function(exports) {
    }(this));

That is OK if it is not inside some other function, if this is a top level IIFE[1]. So I do not believe these changes are needed unless you see that code wrapped in another function either by a build or if some time later the source is converted to a define()-wrapped module. Of course, if this is done just as a protective measure for the future, it seems fine to do.

The trade-off is if you ever expect thos IIFEs to be run in an environment that does not have a window as the global, the change to explicitly use `window` will make that harder. I do not think that is a concern here, but just mentioning it for completeness, for discussing the trade-offs for this kind of change.

[1] http://en.wikipedia.org/wiki/Immediately-invoked_function_expression
Thanks for the supplementary, the changes were made for preventing from the code being wrapped in strict mode functions by mistake.
Comment on attachment 8588939 [details] [review]
[gaia] crh0716:1151723 > mozilla-b2g:master

EJ, given the context above, would you mind take a look at the patch? Thanks.
Attachment #8588939 - Flags: review?(ejchen)
James, as you can see, these patches with IIFE patten are all in old codes without being refactored. For codes in the future, I think we would keep using define() style without IIFE if possible to avoid these. 

I still remembered the reason why we would use IIFE pattern is because we want to follow nodejs coding style and assumed that we may export our code to different context. No matter how our codes would be used in different context or not, we can still achieve the same thing by other ways. (For example, with define() style, we can break our components into smaller pieces and make developers to join them easily to achieve the same thing, and we even't don't have to export anything to the main context like what we did before.)

For now, let's fix the context from `this` to `window` first !
Comment on attachment 8588939 [details] [review]
[gaia] crh0716:1151723 > mozilla-b2g:master

Thanks Arthur, r+ !
Attachment #8588939 - Flags: review?(ejchen) → review+
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#Q1nSA6m4Sk-GewPPZWpUlw

The pull request failed to pass integration tests. It could not be landed, please try again.
master: a290b11627ec2b7c25980f5687a98da86641cfe4

treeherder passed: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=62439ba5407518cf94db5631fa51d31aa139af4e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.