Opened on 08/23/2016 at 02:03:39 PM

Closed on 09/14/2016 at 11:10:03 AM

#4353 closed change (fixed)

Update Js Shell version, replace non-standard "for each" syntax

Reported by: kzar Assignee: kzar
Priority: P3 Milestone:
Module: Automation Keywords:
Cc: sebastian, trev, fhd Blocked By:
Blocking: #4350, #4429 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29350140/
https://codereview.adblockplus.org/29350155/
https://codereview.adblockplus.org/29350158/
https://codereview.adblockplus.org/29350387/
https://codereview.adblockplus.org/29350382/
https://codereview.adblockplus.org/29353000/

Description (last modified by kzar)

Background

It would be nice to update Js Shell at some point. Unfortunately the newer versions of Js Shell (for example 45) do not support the non-standard and deprecated for each syntax which is used in many of the Js Hydra scripts:

astDecompile.js:10:33 SyntaxError: missing ( after for:
astDecompile.js:10:33   return [decompileAST(stmt) for each (stmt in ast.body)].join('\n');
astDecompile.js:10:33 .................................^

What to change

Update Js Shell to version 45 in utils.py:

JSSHELL_DIR = "mozilla-esr45"
JSSHELL_URL = ("https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly"
               "/2016/05/2016-05-29-00-15-03-%s/jsshell-%%s.zip" % JSSHELL_DIR)

Replace any non standard "for each" syntax used by the Js Hydra scripts with standard JavaScript.

Attachments (0)

Change History (21)

comment:1 Changed on 08/23/2016 at 02:06:43 PM by kzar

  • Description modified (diff)

comment:2 Changed on 08/23/2016 at 02:47:21 PM by kzar

  • Resolution set to rejected
  • Status changed from new to closed

Even modern versions of Js Shell do not support the for (const foo of bar) syntax, so I will have to instead modify punycode.js.

comment:3 Changed on 08/23/2016 at 02:52:51 PM by trev

For reference, this is bug 1101653.

comment:4 Changed on 08/23/2016 at 07:08:39 PM by sebastian

  • Priority changed from P2 to P3
  • Resolution rejected deleted
  • Status changed from closed to reopened

I thought we removed all for each loops with #301. If we missed some code there, or regressed since then, we should fix that. Feel free to update the issue description to exclude the jsshell update.

comment:5 Changed on 08/23/2016 at 07:13:47 PM by kzar

  • Description modified (diff)

Alright then, since I've done most of it already I'll finish it off and upload a review.

comment:6 Changed on 08/23/2016 at 07:42:57 PM by trev

Strictly speaking, JSHydra isn't our code (particularly astDecompile.js that this is about) - which is why we didn't touch it yet. At some point we won't be able to avoid it any more however so we need to remove the for each loops there as well.

comment:7 Changed on 08/24/2016 at 11:11:32 AM by kzar

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

So far I've just posted a review for removing for each. Updating JS Shell to version 45 required further changes to adblockpluschrome, adblockpluscore and jshdyra. I still haven't got it quite working!

comment:8 Changed on 08/24/2016 at 11:13:15 AM by kzar

  • Review URL(s) modified (diff)

comment:9 Changed on 08/24/2016 at 11:15:56 AM by kzar

I'm stuck for now with Js Shell 45, getting this message when adblockpluscore/lib/filterClasses.js is included:

uncaught exception: Unknown kind undefined
(Unable to print stack trace)

comment:10 Changed on 08/24/2016 at 11:19:31 AM by kzar

  • Review URL(s) modified (diff)

(Posting a review for my changes to update Js Shell in case any of you guys fancy trying to get it to work!)

comment:11 Changed on 08/30/2016 at 12:05:28 PM by abpbot

A commit referencing this issue has landed:
Issue 4353 - Remove non standard and unused code

comment:12 Changed on 09/01/2016 at 08:39:21 PM by kzar

OK apparently it's the __proto__ syntax inside filterClasses.js that it's complaining about. Stuff like this:

RegExpFilter.prototype =
{
  __proto__: ActiveFilter.prototype,
  ...
};

Changing it to use Object.create seems to help, shame it's more verbose:

RegExpFilter.prototype = Object.create(ActiveFilter.prototype, {
  ...
};

Will post a review to change all those when I get a chance.

comment:13 Changed on 09/01/2016 at 11:50:28 PM by kzar

  • Review URL(s) modified (diff)

Yep, once I replaced {__proto__: ...} with Object.create(...) everywhere in adblockpluscore and adblockplustests I finally got adblockpluschrome to build using Js Shell 45!

comment:14 Changed on 09/13/2016 at 12:35:44 PM by abpbot

A commit referencing this issue has landed:
Issue 4353 - Remove deprecated __proto__ syntax

comment:15 Changed on 09/13/2016 at 12:52:18 PM by abpbot

A commit referencing this issue has landed:
Issue 4353 - Remove deprecated __proto__ syntax

comment:16 Changed on 09/13/2016 at 02:53:46 PM by abpbot

A commit referencing this issue has landed:
Issue 4353 - Avoid redeclaring images parameter

comment:17 Changed on 09/14/2016 at 09:06:19 AM by kzar

  • Blocking 4429 added

comment:18 Changed on 09/14/2016 at 10:42:53 AM by abpbot

A commit referencing this issue has landed:
Issue 4353 - Update Js Shell to version 45

comment:19 Changed on 09/14/2016 at 10:50:34 AM by kzar

  • Review URL(s) modified (diff)

comment:20 Changed on 09/14/2016 at 10:55:07 AM by abpbot

A commit referencing this issue has landed:
Issue 4353 - Update jshydra dependency

comment:21 Changed on 09/14/2016 at 11:10:03 AM by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from kzar.
 
Note: See TracTickets for help on using tickets.