Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(131)

Issue 29589668: Issue 5938 - Use top-level var for global variables (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by Manish Jethani
Modified:
2 years ago
Reviewers:
kzar, Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 5938 - Use top-level var for global variables

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -13 lines) Patch
M composer.postload.js View 1 chunk +2 lines, -5 lines 1 comment Download
M ext/common.js View 1 chunk +2 lines, -2 lines 0 comments Download
M include.preload.js View 1 chunk +0 lines, -5 lines 3 comments Download
M polyfill.js View 2 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 4
Manish Jethani
2 years ago (2017-10-26 20:44:27 UTC) #1
Manish Jethani
Patch Set 1 As explained in my comment [1] on the issue, there is a ...
2 years ago (2017-10-26 20:47:56 UTC) #2
kzar
I'm not against working around the problem if possible, but I'm also not against just ...
2 years ago (2017-10-27 11:17:40 UTC) #3
Manish Jethani
2 years ago (2017-10-27 11:40:09 UTC) #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5