Issue 4353 - Update Js Shell to version 45

Issue 4353 - Update Js Shell to version 45

Note: In order to build adblockpluschrome with Js Shell 45 we will also need to update the adblockpluscore and adblockplustests dependencies and wait for these changes to land:

https://codereview.adblockplus.org/29350286/
https://codereview.adblockplus.org/29350155/

[kzar, 2016-08-24 11:18:45]

Patch Set 1

[Sebastian Noack, 2016-08-24 11:27:55]

https://codereview.adblockplus.org/29350158/diff/29350159/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29350158/diff/29350159/utils.py#newcode17
utils.py:17: "/2016/05/2016-05-29-00-15-03-%s/jsshell-%%s.zip" % JSSHELL_DIR)
Nit: While changing this line mind using str.format() instead % for compliance
with our coding style?

[Wladimir Palant, 2016-08-24 14:08:36]

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite.js
File scripts/abprewrite.js (right):

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite....
scripts/abprewrite.js:131: ast = global[func](ast);
How is this working now? You removed the `global` variable, and I don't see it
being listed as predefined on
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Shell_....

[kzar, 2016-08-24 14:25:21]

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite.js
File scripts/abprewrite.js (right):

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite....
scripts/abprewrite.js:131: ast = global[func](ast);
On 2016/08/24 14:08:36, Wladimir Palant wrote:
> How is this working now? You removed the `global` variable, and I don't see it
> being listed as predefined on
>
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Shell_....

`global` is already defined in astDecompile.js which is included by this script.
I removed it here since otherwise I got an error "TypeError: redeclaration of
let global".

[Wladimir Palant, 2016-08-24 14:45:17]

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite.js
File scripts/abprewrite.js (right):

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite....
scripts/abprewrite.js:131: ast = global[func](ast);
On 2016/08/24 14:25:20, kzar wrote:
> `global` is already defined in astDecompile.js which is included by this
script.
> I removed it here since otherwise I got an error "TypeError: redeclaration of
> let global".

Ok, this is really awkward... This should really be a lookup table, so that
fetching functions from global scope wouldn't be necessary.

[kzar, 2016-09-13 13:35:20]

Patch Set 2 : Rebased.

(I haven't tested this since the rebase as I'm waiting for the buildtools
dependency update to land https://codereview.adblockplus.org/29350286/ )

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite.js
File scripts/abprewrite.js (right):

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite....
scripts/abprewrite.js:131: ast = global[func](ast);
On 2016/08/24 14:45:17, Wladimir Palant wrote:
> On 2016/08/24 14:25:20, kzar wrote:
> > `global` is already defined in astDecompile.js which is included by this
> script.
> > I removed it here since otherwise I got an error "TypeError: redeclaration
of
> > let global".
> 
> Ok, this is really awkward... This should really be a lookup table, so that
> fetching functions from global scope wouldn't be necessary.

I agree this is awkward, but I don't understand what you're suggesting. What
should I change?

https://codereview.adblockplus.org/29350158/diff/29350159/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29350158/diff/29350159/utils.py#newcode17
utils.py:17: "/2016/05/2016-05-29-00-15-03-%s/jsshell-%%s.zip" % JSSHELL_DIR)
On 2016/08/24 11:27:55, Sebastian Noack wrote:
> Nit: While changing this line mind using str.format() instead % for compliance
> with our coding style?

Seems this was already done for me, fixed since I rebased.

[Wladimir Palant, 2016-09-13 14:23:00]

LGTM

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite.js
File scripts/abprewrite.js (right):

https://codereview.adblockplus.org/29350158/diff/29350159/scripts/abprewrite....
scripts/abprewrite.js:131: ast = global[func](ast);
On 2016/09/13 13:35:20, kzar wrote:
> I agree this is awkward, but I don't understand what you're suggesting. What
> should I change?

I'm not actually suggesting anything at this point, I guess we'll need to leave
this as is for now :-(

[Sebastian Noack, 2016-09-13 14:47:09]

LGTM.

However, I just noticed that jsshell has to be updated manually on the server,
since this code isn't used if the SPIDERMONKEY_BINARY environment variable is
set, see #4097. Hindsight, it would perhaps made more sense to just override the
directory where it caches the jsshell builds into.

[Wladimir Palant, 2016-09-13 15:06:35]

On 2016/09/13 14:47:09, Sebastian Noack wrote:
> However, I just noticed that jsshell has to be updated manually on the server,
> since this code isn't used if the SPIDERMONKEY_BINARY environment variable is
> set, see #4097. Hindsight, it would perhaps made more sense to just override
the
> directory where it caches the jsshell builds into.

Probably a good idea. For now I downloaded the new jsshell version however and
will update the settings once this is pushed.