WebUI: Replace vulcanize with polymer-bundler

chrome/browser/resources/vulcanize_gn.py

Index: chrome/browser/resources/vulcanize_gn.py
diff --git a/chrome/browser/resources/vulcanize_gn.py b/chrome/browser/resources/vulcanize_gn.py
index e6983018f02b42ae9df154a1b09c9aa504f76e8c..7485bf1729b046e593fadd76d4231e66a1b8f93d 100755
--- a/chrome/browser/resources/vulcanize_gn.py
+++ b/chrome/browser/resources/vulcanize_gn.py
@@ -10,6 +10,8 @@ import platform
 import re
 import sys
 import tempfile
+import json

[michaelpg, 2017/07/19 18:51:59]

nit: alphabetical

[dpapad, 2017/07/19 20:49:38]

Done.

+import shutil
 
 
 _HERE_PATH = os.path.dirname(__file__)
@@ -43,16 +45,19 @@ _POLYMER_PATH = os.path.join(
 _VULCANIZE_BASE_ARGS = [
   # These files are already combined and minified.
   '--exclude', 'chrome://resources/html/polymer.html',
-  '--exclude', 'web-animations-next-lite.min.js',
+  '--exclude', 'chrome://resources/polymer/v1_0/polymer/polymer.html',
+  '--exclude', 'chrome://resources/polymer/v1_0/polymer/polymer-micro.html',
+  '--exclude', 'chrome://resources/polymer/v1_0/polymer/polymer-mini.html',
+  '--exclude', 'chrome://resources/polymer/v1_0/web-animations-js/web-animations-next-lite.min.js',
 
-  # These files are dynamically created by C++.
-  '--exclude', 'load_time_data.js',
-  '--exclude', 'strings.js',
-  '--exclude', 'text_defaults.css',
-  '--exclude', 'text_defaults_md.css',
+  '--exclude', 'chrome://resources/css/roboto.css',
+  '--exclude', 'chrome://resources/css/text_defaults.css',
+  '--exclude', 'chrome://resources/css/text_defaults_md.css',
+  '--exclude', 'chrome://resources/js/load_time_data.js',
 
   '--inline-css',
   '--inline-scripts',
+  '--rewrite-urls-in-templates',
   '--strip-comments',
 ]
 
@@ -77,32 +82,24 @@ def _undo_mapping(mappings, url):
   # TODO(dbeam): can we make this stricter?
   return url
 
-def _request_list_path(out_path, html_out_file):
-  return os.path.join(out_path, html_out_file + '_requestlist.txt')
+def _request_list_path(out_path, host):
+  return os.path.join(out_path, host + '_requestlist.txt')
 
-# Get a list of all files that were bundled with Vulcanize and update the
-# depfile accordingly such that Ninja knows when to trigger re-vulcanization.
-def _update_dep_file(in_folder, args):
+# Get a list of all files that were bundled with polymer-bundler and update the
+# depfile accordingly such that Ninja knows when to re-trigger.
+def _update_dep_file(in_folder, args, manifest):
   in_path = os.path.join(_CWD, in_folder)
-  out_path = os.path.join(_CWD, args.out_folder)
 
-  # Prior call to vulcanize already generated the deps list, grab it from there.
-  request_list_path = _request_list_path(out_path, args.html_out_file)
-  request_list = open(request_list_path, 'r').read().splitlines()
-
-  if platform.system() == 'Windows':
-    # TODO(dbeam): UGH. For some reason Vulcanize is interpreting the target
-    # file path as a URL and using the drive letter (e.g. D:\) as a protocol.
-    # This is a little insane, but we're fixing here by normalizing case (which
-    # really shouldn't matter, these are all file paths and generally are all
-    # lower case) and writing from / to \ (file path) and then back again. This
-    # is compounded by NodeJS having a bug in url.resolve() that handles
-    # chrome:// protocol URLs poorly as well as us using startswith() to strip
-    # file paths (which isn't crazy awesome either). Don't remove unless you
-    # really really know what you're doing.
-    norm = lambda u: u.lower().replace('/', '\\')
-    request_list = [norm(u).replace(norm(in_path), '').replace('\\', '/')
-        for u in request_list]
+  # Gather the dependencies of all bundled root HTML files.
+  request_list = []
+  for html_file in manifest:
+    request_list += manifest[html_file]
+
+  # Add a slash in front of every dependency that is not a chrome:// URL, so
+  # that we can map it to the correct source file path below.
+  request_list = map(
+      lambda dep: '/' + dep if not dep.startswith('chrome://') else dep,
+      request_list)
 
   # Undo the URL mappings applied by vulcanize to get file paths relative to
   # current working directory.
@@ -121,81 +118,86 @@ def _update_dep_file(in_folder, args):
     deps = [d for d in deps if not d.startswith(filter_url)]
 
   with open(os.path.join(_CWD, args.depfile), 'w') as f:
-    deps_file_header = os.path.join(args.out_folder, args.html_out_file)
+    deps_file_header = os.path.join(args.out_folder, args.html_out_files[0])
     f.write(deps_file_header + ': ' + ' '.join(deps))
 
 
 def _vulcanize(in_folder, args):
   in_path = os.path.normpath(os.path.join(_CWD, in_folder))
   out_path = os.path.join(_CWD, args.out_folder)
-
-  html_out_path = os.path.join(out_path, args.html_out_file)
-  js_out_path = os.path.join(out_path, args.js_out_file)
+  manifest_out_path = _request_list_path(out_path, args.host)
 
   exclude_args = []
   for f in args.exclude or []:
     exclude_args.append('--exclude')
     exclude_args.append(f)
 
-  output = node.RunNode(
-      [node_modules.PathToVulcanize()] +
-      _VULCANIZE_BASE_ARGS + _VULCANIZE_REDIRECT_ARGS + exclude_args +
-      ['--out-request-list', _request_list_path(out_path, args.html_out_file),
-       '--redirect', '"/|%s"' % in_path,
-       '--redirect', '"chrome://%s/|%s"' % (args.host, in_path),
-       # TODO(dpapad): Figure out why vulcanize treats the input path
-       # differently on Windows VS Linux/Mac.
-       os.path.join(
-           in_path if platform.system() == 'Windows' else os.sep,
-           args.html_in_file)])
-
-  # Grit includes are not supported, use HTML imports instead.
-  output = output.replace('<include src="', '<include src-disabled="')
+  in_html_args = []
+  for f in args.html_in_files:
+    in_html_args.append('--in-html')
+    in_html_args.append(f)
 
-  if args.insert_in_head:
-    assert '<head>' in output
-    # NOTE(dbeam): Vulcanize eats <base> tags after processing. This undoes
-    # that by adding a <base> tag to the (post-processed) generated output.
-    output = output.replace('<head>', '<head>' + args.insert_in_head)
-
-  crisper_input = tempfile.NamedTemporaryFile(mode='wt+', delete=False)
-  crisper_input.write(output)
-  crisper_input.close()
+  tmp_out_dir = os.path.join(out_path, 'bundled')
+  node.RunNode(
+      [node_modules.PathToBundler()] +
+      _VULCANIZE_BASE_ARGS + _VULCANIZE_REDIRECT_ARGS + exclude_args +
+      [# This file is dynamically created by C++. Need to specify an exlusion

[calamity, 2017/07/19 06:15:33]

nit: exclusion

[dpapad, 2017/07/19 20:49:38]

Done.

+       # URL for both the relative URL and chrome:// URL syntax.
+       '--exclude', 'strings.js',
+       '--exclude', 'chrome://%s/strings.js' % args.host,
 
-  crisper_output = tempfile.NamedTemporaryFile(mode='wt+', delete=False)
-  crisper_output.close()
+       '--manifest-out', manifest_out_path,
+       '--root', in_path,
+       '--redirect', '"chrome://%s/|%s"' % (args.host, in_path),
+       '--out-dir', os.path.relpath(tmp_out_dir, _CWD),
+       '--shell', args.html_in_files[0],
+      ] + in_html_args)
+
+  for index, html_file in enumerate(args.html_in_files):
+    with open(
+        os.path.join(os.path.relpath(tmp_out_dir, _CWD), html_file), 'r') as f:
+      output = f.read()
+
+      # Grit includes are not supported, use HTML imports instead.
+      output = output.replace('<include src="', '<include src-disabled="')
+
+      if args.insert_in_head:
+        assert '<head>' in output
+        # NOTE(dbeam): polymer-bundler eats <base> tags after processing. This
+        # undoes that by adding a <base> tag to the (post-processed) generated
+        # output.
+        output = output.replace('<head>', '<head>' + args.insert_in_head)
+
+    # Open file again with 'w' such that the previous contents are overwritten.
+    with open(
+        os.path.join(os.path.relpath(tmp_out_dir, _CWD), html_file), 'w') as f:
+      f.write(output)
+      f.close()
 
   try:
-    node.RunNode([node_modules.PathToCrisper(),
-                 '--source', crisper_input.name,
-                 '--script-in-head', 'false',
-                 '--only-split',
-                 '--html', html_out_path,
-                 '--js', crisper_output.name])
-
-    # Crisper by default inserts a <script> tag with the name of the --js file,
-    # but since we are using a temporary file, need to manually insert a
-    # <script> tag with the correct final filename (in combination with
-    # --only-split flag). There is no way currently to manually specify the
-    # <script> tag's path, see https://github.com/PolymerLabs/crisper/issues/46.
-    with open(html_out_path, 'r+') as f:
-      data = f.read()
-      new_data = data.replace(
-          '</body></html>',
-          '<script src="' + args.js_out_file + '"></script></body></html>')
-      assert new_data != data, 'Expected to find </body></html> token.'
-      f.seek(0)
-      f.write(new_data)
-      f.truncate()
-
-    node.RunNode([node_modules.PathToUglify(), crisper_output.name,
-                  '--comments', '"/Copyright|license|LICENSE|\<\/?if/"',
-                  '--output', js_out_path])
+    for index, html_in_file in enumerate(args.html_in_files):
+      html_out_file = args.html_out_files[index]
+      js_out_file = args.js_out_files[index]
+
+      # Run crisper to separate the JS from the HTML file.
+      node.RunNode([node_modules.PathToCrisper(),
+                   '--source', os.path.join(tmp_out_dir, html_in_file),
+                   '--script-in-head', 'false',
+                   '--html', os.path.join(tmp_out_dir, html_out_file),
+                   '--js', os.path.join(tmp_out_dir, js_out_file)])

[calamity, 2017/07/19 06:15:33]

If we move the polymer_css_build step into this script, we'll end up with the
same file name for html_in_file and html_out_file, right? It might be good add a
prefix or suffix to each of the files that explains the processes that have been
done on each of them. This will help prevent name collisions and also makes the
temporary files easier to debug.

[dpapad, 2017/07/19 20:49:38]

Acknowledged. As said in previous comment, prefer to merge polymer_css_build
into this script in a follow up CL, so will address this in that CL.

+
+      # Move the HTML file to its final destination.
+      shutil.copy(os.path.join(tmp_out_dir, html_out_file), out_path)
+
+      # Pass the JS file through Uglify and write the output to its final
+      # destination.
+      node.RunNode([node_modules.PathToUglify(),
+                    os.path.join(tmp_out_dir, js_out_file),
+                    '--comments', '"/Copyright|license|LICENSE|\<\/?if/"',
+                    '--output', os.path.join(out_path, js_out_file)])
   finally:
-    if os.path.exists(crisper_input.name):
-      os.remove(crisper_input.name)
-    if os.path.exists(crisper_output.name):
-      os.remove(crisper_output.name)
+    shutil.rmtree(tmp_out_dir)

[calamity, 2017/07/19 06:15:33]

If any of the commands here fail, will the build still succeed?

[michaelpg, 2017/07/19 18:51:59]

Is it worth leaving the output files around (instead of using "finally") for
debugging purposes?

[dpapad, 2017/07/19 20:49:38]

The build still fails if any command fails.

If you run a build again after the 1st failure, Ninja will decide whether to
rebuild based on the timestamps of the declared output files, so
 
 - if the build failed before one of those files was produced (say before line
194), it will rebuild,
 - if there is an error after all output files have been produced it will do
nothing.

I believe this is the same behavior as before.
I prefer not to do this, because it increases the chances that a subsequent
build will use stale files. I've had this happened in the past where a local
build was succeeding, because stale files were present under out/../gen/..., but
the build was failing on the bots, and it was fairly confusing.

One can always comment out this line locally as needed, which is what I did
while developing this CL.

[michaelpg, 2017/07/21 00:57:11]

d'oh, yeah I saw "tmp" and thought it was a unique `mktemp -d` style temporary
directory.

+  return manifest_out_path
 
 
 def main(argv):
@@ -203,11 +205,11 @@ def main(argv):
   parser.add_argument('--depfile', required=True)
   parser.add_argument('--exclude', nargs='*')
   parser.add_argument('--host', required=True)
-  parser.add_argument('--html_in_file', required=True)
-  parser.add_argument('--html_out_file', required=True)
+  parser.add_argument('--html_in_files', nargs='*', required=True)
+  parser.add_argument('--html_out_files', nargs='*', required=True)
   parser.add_argument('--input', required=True)
   parser.add_argument('--insert_in_head')
-  parser.add_argument('--js_out_file', required=True)
+  parser.add_argument('--js_out_files', nargs='*', required=True)
   parser.add_argument('--out_folder', required=True)
   args = parser.parse_args(argv)
 
@@ -218,8 +220,20 @@ def main(argv):
   args.input = os.path.normpath(args.input)
   args.out_folder = os.path.normpath(args.out_folder)
 
-  _vulcanize(args.input, args)
-  _update_dep_file(args.input, args)
+  manifest_out_path = _vulcanize(args.input, args)
+
+  # Prior call to _vulcanize() generated an output manifest file, containing
+  # information about all files that were bundled. Grab it from there.
+  manifest = json.loads(open(manifest_out_path, 'r').read())
+
+  # polymer-bundler reports any missing files in the output manifest, instead of
+  # directly failing. Ensure that no such files were encountered.
+  if '_missing' in manifest:
+    raise Exception(
+        'polymer-bundler could not find files for the following URLs:\n' +
+        '\n'.join(manifest['_missing']))
+
+  _update_dep_file(args.input, args, manifest)
 
 
 if __name__ == '__main__':