Issue 4552 - Drop jshydra dependency (buildtools)

packagerChrome.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import errno
 import io
 import json
 import os
 import re
 from StringIO import StringIO
@@ skipping 127 matching lines @@
     return manifest.encode('utf-8')
 
 
 def createInfoModule(params):
     template = getTemplate('chromeInfo.js.tmpl')
     return template.render(params).encode('utf-8')
 
 
 def convertJS(params, files):
     output_files = collections.OrderedDict()
-    args = collections.defaultdict(dict)
+    args = {}
 
     for item in params['metadata'].items('convert_js'):

[Wladimir Palant, 2016/11/30 14:54:44]

Nit: `for name, value in ...`? This should make the code below easier to
understand.

[kzar, 2016/11/30 15:26:17]

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

-        filename, arg = re.search(r'^(.*?)(?:\[(.*)\])?$', item[0]).groups()
+        name, value = item
+        filename, arg = re.search(r'^(.*?)(?:\[(.*)\])?$', name).groups()
         if arg is None:
-            output_files[filename] = (item[1].split(), item.source)
+            output_files[filename] = (value.split(), item.source)
         else:
-            args[filename][arg] = item[1]

[Wladimir Palant, 2016/11/30 11:39:32]

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).

[kzar, 2016/11/30 14:29:44]

Done.

-
-    template = getTemplate('modules.js.tmpl', autoEscape=True)

[Wladimir Palant, 2016/11/30 11:39:32]

Please remove autoEscape=True, this isn't an HTML or XML document. Escaping
entities makes no sense in JavaScript context.

[kzar, 2016/11/30 14:29:44]

Done.

+            args.setdefault(filename, {})[arg] = value
+
+    template = getTemplate('modules.js.tmpl')
 
     for filename, (input_files, origin) in output_files.iteritems():
         if '/' in filename and not files.isIncluded(filename):
             continue
 
-        args["filename"]["autoload"] = set(args.get("autoload", "").split(","))

[Wladimir Palant, 2016/11/30 11:39:32]

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.

[kzar, 2016/11/30 14:29:44]

Ouch, done.
Done.
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.)

+        current_args = args.get(filename, {})
+        current_args['autoload'] = [module for module in
+                                    current_args.get('autoload', '').split(',')
+                                    if module != '']
 
         base_dir = os.path.dirname(origin)
         modules = []

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

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.)

 
         for input_filename in input_files:
             module_name = os.path.splitext(os.path.basename(input_filename))[0]
             prefix = os.path.basename(os.path.dirname(input_filename))
-            if prefix != "lib":
-                module_name = "{}_{}".format(prefix, module_name)
-            with open(os.path.join(base_dir, input_filename), 'r') as f:

[Wladimir Palant, 2016/11/30 11:39:32]

Nit: file rather than f?

[kzar, 2016/11/30 14:29:44]

Done.

-                modules.append((module_name, f.read().decode('utf-8')))
+            if prefix != 'lib':
+                module_name = '{}_{}'.format(prefix, module_name)
+            with open(os.path.join(base_dir, input_filename), 'r') as file:
+                modules.append((module_name, file.read().decode('utf-8')))
             files.pop(input_filename, None)
 
         files[filename] = template.render(
-            args=args[filename],
+            args=current_args,
             modules=modules
         ).encode('utf-8')
 
 
 def toJson(data):
     return json.dumps(
         data, ensure_ascii=False, sort_keys=True,
         indent=2, separators=(',', ': ')
     ).encode('utf-8') + '\n'
 
@@ skipping 205 matching lines @@
         files['qunit/index.html'] = createScriptPage(params, 'testIndex.html.tmpl',
                                                      ('general', 'testScripts'))
 
     zipdata = files.zipToString()
     signature = None
     pubkey = None
     if keyFile != None:
         signature = signBinary(zipdata, keyFile)
         pubkey = getPublicKey(keyFile)
     writePackage(outFile, pubkey, signature, zipdata)