Issue 5938 - Use top-level var for global variables

Issue 5938 - Use top-level var for global variables

[Manish Jethani, 2017-10-26 20:47:56]

Patch Set 1

As explained in my comment [1] on the issue, there is a bug in Firefox. I'll
report it, but meanwhile this is a good idea anyway. We shouldn't be using the
window object except for stuff that's placed there by the browser.

Note that we must use the window object to add our wrappers (like
window.WebSocket). I don't know if this Firefox bug will break something or if
the wrappers will simply fail quietly.

[1]: https://issues.adblockplus.org/ticket/5938#comment:10

https://codereview.adblockplus.org/29589668/diff/29589669/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29589668/diff/29589669/composer.postload.j...
composer.postload.js:18: /* globals checkCollapse, elemhide, getURLsFromElement,
typeMap */
This is back to where it was.

https://codereview.adblockplus.org/29589668/diff/29589669/include.preload.js
File include.preload.js (left):

https://codereview.adblockplus.org/29589668/diff/29589669/include.preload.js#...
include.preload.js:527: window.checkCollapse = checkCollapse;
This didn't really make sense, the variables are accessible in
composer.postload.js anyway.

[kzar, 2017-10-27 11:17:40]

I'm not against working around the problem if possible, but I'm also not against
just marking this issue as externaldependencies since it's only a symptom of
https://bugzilla.mozilla.org/show_bug.cgi?id=1408996

https://codereview.adblockplus.org/29589668/diff/29589669/include.preload.js
File include.preload.js (left):

https://codereview.adblockplus.org/29589668/diff/29589669/include.preload.js#...
include.preload.js:527: window.checkCollapse = checkCollapse;
On 2017/10/26 20:47:56, Manish Jethani wrote:
> This didn't really make sense, the variables are accessible in
> composer.postload.js anyway.

Have you tested that? include.preload.js in the build is actually a webpack
bundle consisting of include.preload.js and inject.preload.js, so I would have
expected variables in here to be inaccessible from composer.postload.js.

[Manish Jethani, 2017-10-27 11:40:09]

On 2017/10/27 11:17:40, kzar wrote:
> I'm not against working around the problem if possible, but I'm also not
against
> just marking this issue as externaldependencies since it's only a symptom of
> https://bugzilla.mozilla.org/show_bug.cgi?id=1408996

Since this patch doesn't even work, I suppose there's no argument to be made.
Yeah, let's do that.

I'll close this.

https://codereview.adblockplus.org/29589668/diff/29589669/include.preload.js
File include.preload.js (left):

https://codereview.adblockplus.org/29589668/diff/29589669/include.preload.js#...
include.preload.js:527: window.checkCollapse = checkCollapse;
On 2017/10/27 11:17:40, kzar wrote:
> On 2017/10/26 20:47:56, Manish Jethani wrote:
> > This didn't really make sense, the variables are accessible in
> > composer.postload.js anyway.
> 
> Have you tested that? include.preload.js in the build is actually a webpack
> bundle consisting of include.preload.js and inject.preload.js, so I would have
> expected variables in here to be inaccessible from composer.postload.js.

I see, you're right, it doesn't actually work, and it looks like we have no
options here. Even when we add the custom loader, it'll have to work with the
window object.