Issue 4552 - Drop jshydra dependency (buildtools)

Issue 4552 - Drop jshydra dependency (buildtools)

[kzar, 2016-11-18 17:25:48]

Patch Set 1

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:166: modules = []
List of tuples instead of a dict since the module names are occasionally the
same. For example "common" could mean a few different files. Only matters when
not using the module system of course however this way seemed simpler than
having two code paths. (Also order might matter.)

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
File templates/modules.js.tmpl (right):

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:1: {%- if args["module"] -%}
I went without indentation since it made the rendered output nicer. Otherwise I
had to pick between no newline and newline with indentation spaces and that made
things ugly fast.

[kzar, 2016-11-18 17:26:55]

(Publishing again since I forgot to Cc Vasily, sorry.)

[Wladimir Palant, 2016-11-30 11:39:33]

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:155: args[filename][arg] = item[1]
Nit: Frankly, I'd prefer `args.setdefault(filename, {})[arg] = item[1]` instead
of the obscure games with defaultdict (yes, you didn't introduce that code).

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:157: template = getTemplate('modules.js.tmpl',
autoEscape=True)
Please remove autoEscape=True, this isn't an HTML or XML document. Escaping
entities makes no sense in JavaScript context.

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:163: args["filename"]["autoload"] = set(args.get("autoload",
"").split(","))
This line has three bugs, I assume that it hasn't been tested. How about you
store args[filename] in a local variable? This would elegantly squash the first
two bugs (accessing the wrong dictionary). The third issue is that
set("".split(",")) won't produce an empty set as you probably expected. Plus,
autoload order might be relevant - this shouldn't be a set. Altogether this
should probably be:

  current_args = args.get(filename, {})
  current_args['autoload'] = [
    module for module in current_args.get('autoload', '').split(',') if module
!= ''
  ]

Also: single quotation marks please (affects a bunch of other code parts as
well). You should run tox, it will point out these PEP-8 compliance issues.

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:173: with open(os.path.join(base_dir, input_filename), 'r') as
f:
Nit: file rather than f?

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
File templates/modules.js.tmpl (right):

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:3: require.modules[{{ module_name|json|safe }}] =
function(module, exports, require)
No point passing in require as parameter, the globally defined require function
will do for now.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:5: {{ script|safe }}
Enabling autoescaping and then marking each variable as safe sort of defeats the
purpose of it ;)

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:10: {% for module_name, script in modules -%}
This should be {%- for ... -%}, no point adding an extra newline here.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:11: {%- if module_name in args["autoload"] -%}
This should not iterate over modules again, rather over args["autoload"]. The
autoload order should logically be defined by the autoload argument, not by the
module order.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:19: {% endif %}
This should be:

{% endfor -%}
{%- endif -%}

In other words, there should be a newline after each script, not only newlines
(currently two) at the end of file.

[Wladimir Palant, 2016-11-30 11:42:14]

Overlooked one more issue.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
File templates/modules.js.tmpl (right):

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:6: return exports;
This should return module.exports, in case the module decides to replace it by
its own object (which is valid with CommonJS).

[kzar, 2016-11-30 14:29:45]

Patch Set 2 : Addressed feedback, make tox pass

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:155: args[filename][arg] = item[1]
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> Nit: Frankly, I'd prefer `args.setdefault(filename, {})[arg] = item[1]` 
> instead of the obscure games with defaultdict (yes, you didn't introduce 
> that code).

Done.

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:157: template = getTemplate('modules.js.tmpl',
autoEscape=True)
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> Please remove autoEscape=True, this isn't an HTML or XML document. Escaping
> entities makes no sense in JavaScript context.

Done.

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:163: args["filename"]["autoload"] = set(args.get("autoload",
"").split(","))
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> This line has three bugs...

Ouch, done.

> Also: single quotation marks please (affects a bunch of other 
> code parts as well).

Done.

> You should run tox, it will point out these PEP-8 compliance issues.

In fairness I did run tox, unfortunately due to issue #4575 I didn't get any
useful output. (I had assumed I would get the useful output too, but apparently
it was masked by that issue instead.)

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:173: with open(os.path.join(base_dir, input_filename), 'r') as
f:
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> Nit: file rather than f?

Done.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
File templates/modules.js.tmpl (right):

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:3: require.modules[{{ module_name|json|safe }}] =
function(module, exports, require)
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> No point passing in require as parameter, the globally defined require 
> function will do for now.

Done.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:5: {{ script|safe }}
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> Enabling autoescaping and then marking each variable as safe sort
> of defeats the purpose of it ;)

Lol good point, done.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:6: return exports;
On 2016/11/30 11:42:14, Wladimir Palant wrote:
> This should return module.exports, in case the module decides to replace it by
> its own object (which is valid with CommonJS).

Done.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:10: {% for module_name, script in modules -%}
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> This should be {%- for ... -%}, no point adding an extra newline here.

Done.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:11: {%- if module_name in args["autoload"] -%}
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> This should not iterate over modules again, rather over args["autoload"]. The
> autoload order should logically be defined by the autoload argument, 
> not by the module order.

Done.

https://codereview.adblockplus.org/29363565/diff/29363566/templates/modules.j...
templates/modules.js.tmpl:19: {% endif %}
On 2016/11/30 11:39:32, Wladimir Palant wrote:
> This should be:
> 
> {% endfor -%}
> {%- endif -%}
> 
> In other words, there should be a newline after each script, not only newlines
> (currently two) at the end of file.

Done.

https://codereview.adblockplus.org/29363565/diff/29365832/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29363565/diff/29365832/releaseAutomation.p...
releaseAutomation.py:13: 
Tox was complaining about this.

https://codereview.adblockplus.org/29363565/diff/29365832/templates/modules.j...
File templates/modules.js.tmpl (right):

https://codereview.adblockplus.org/29363565/diff/29365832/templates/modules.j...
templates/modules.js.tmpl:16: {% endfor -%}
`{%` not `{%-` so that the require calls are separated by newlines.

[Wladimir Palant, 2016-11-30 14:54:45]

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:150: for item in params['metadata'].items('convert_js'):
Nit: `for name, value in ...`? This should make the code below easier to
understand.

https://codereview.adblockplus.org/29363565/diff/29365832/templates/modules.j...
File templates/modules.js.tmpl (right):

https://codereview.adblockplus.org/29363565/diff/29365832/templates/modules.j...
templates/modules.js.tmpl:11: require({{ module_name|json }});
It's probably a good idea to check first whether the module exists, e.g. on
Firefox we are currently removing popupBlocker module but it cannot be removed
from autoload. Something like this:

{%- set modules = dict(modules) -%}

...

{%- if module_name in modules %-}

[kzar, 2016-11-30 15:26:18]

Patch Set 3 : Check module exists before auto-loading it

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29363565/diff/29363566/packagerChrome.py#n...
packagerChrome.py:150: for item in params['metadata'].items('convert_js'):
On 2016/11/30 14:54:44, Wladimir Palant wrote:
> Nit: `for name, value in ...`? This should make the code below easier 
> to understand.

Well we need item still for `item.source`. How about this?

https://codereview.adblockplus.org/29363565/diff/29365832/templates/modules.j...
File templates/modules.js.tmpl (right):

https://codereview.adblockplus.org/29363565/diff/29365832/templates/modules.j...
templates/modules.js.tmpl:11: require({{ module_name|json }});
On 2016/11/30 14:54:44, Wladimir Palant wrote:
> It's probably a good idea to check first whether the module exists, e.g. on
> Firefox we are currently removing popupBlocker module but it cannot be removed
> from autoload. Something like this:
> 
> {%- set modules = dict(modules) -%}
> 
> ...
> 
> {%- if module_name in modules %-}

Done.

[Wladimir Palant, 2016-11-30 15:41:26]

LGTM