Compressing .pak resources with new option: "type=GZIPPABLE_BINDATA"

tools/grit/grit/node/include.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Handling of the <include> element.
 """
 
+import gzip
 import os
+import StringIO
+import subprocess
+import sys
 
+from grit import util
 import grit.format.html_inline
+import grit.format.rc
 import grit.format.rc_header
-import grit.format.rc
-
 from grit.node import base
-from grit import util
 
 class IncludeNode(base.Node):
   """An <include> element."""
   def __init__(self):
     super(IncludeNode, self).__init__()
 
     # Cache flattened data so that we don't flatten the same file
     # multiple times.
     self._flattened_data = None
     # Also keep track of the last filename we flattened to, so we can
     # avoid doing it more than once.
     self._last_flat_filename = None
 
   def _IsValidChild(self, child):
     return False
 
   def _GetFlattenedData(self, allow_external_script=False):
     if not self._flattened_data:
       filename = self.ToRealPath(self.GetInputPath())
       self._flattened_data = (
           grit.format.html_inline.InlineToString(filename, self,
               allow_external_script=allow_external_script))
     return self._flattened_data
 
+  def _GzipString(self, data):

[flackr, 2016/05/13 23:02:17]

This helper function (or functions if you follow my suggestion) should probably
be in a separate file in tools/grit/format/ alongside similar data
transformation functions like html_inline (maybe call it gzip_string.py).

[smaier, 2016/05/19 14:17:56]

Done.

+    if sys.platform == 'linux2':

[flackr, 2016/05/13 23:02:16]

nit: I'd prefer to see either two gzip functions (GzipString and
GzipStringRsyncable) or a parameter to the function, though I have a preference
for two functions since the behavior is drastically different.

Then at the call site decide which one to call with a short comment that we only
require rsyncable on linux. e.g.

# We only use rsyncable strings on linux platforms.
if sys.platform == 'linux2':
  data = GzipStringRsyncable(data)
else
  data = GzipString(data)

[smaier, 2016/05/19 14:17:56]

Done.

+      # Make call to linux's gzip to get access to --rsyncable option. This
+      # option makes updates much smaller - if one line is changed in the
+      # resource, it won't have to push the entire compressed resource with
+      # the update. Instead, --rsyncable breaks the file into small chunks,
+      # so that one doesn't affect the other in compression, and then only
+      # that chunk will have to be updated.
+      gzip_proc = subprocess.Popen(['gzip', '--stdout', '--rsyncable',
+                                    '--best', '--no-name'],
+                                   stdin=subprocess.PIPE,
+                                   stdout=subprocess.PIPE,
+                                   stderr=subprocess.PIPE)
+      data, stderr = gzip_proc.communicate(data)
+      if gzip_proc.returncode != 0:
+        raise subprocess.CalledProcessError(gzip_proc.returncode, 'gzip',
+                                            stderr)
+    else:
+      # Only expecting linux gzip calls to work: windows doesn't ship with

[flackr, 2016/05/13 23:02:17]

This comment needs updating, or to be removed.

[smaier, 2016/05/19 14:17:56]

Done.

+      # gzip, and OSX's gzip does not have an --rsyncable option built in.
+      # Therefore, fall back into the built in python function.
+      gzip_output = StringIO.StringIO()
+      with gzip.GzipFile('', 'wb', 9, gzip_output, 0) as gzip_file:
+        gzip_file.write(data)
+      data = gzip_output.getvalue()
+      gzip_output.close()
+    return data
+
   def MandatoryAttributes(self):
     return ['name', 'type', 'file']
 
   def DefaultAttributes(self):
     return {'translateable' : 'true',
             'generateid': 'true',
             'filenameonly': 'false',
             'mkoutput': 'false',
             'flattenhtml': 'false',
             'allowexternalscript': 'false',
@@ skipping 32 matching lines @@
     from grit.format import rc_header
     id_map = rc_header.GetIds(self.GetRoot())
     id = id_map[self.GetTextualIds()[0]]
     if self.attrs['flattenhtml'] == 'true':
       allow_external_script = self.attrs['allowexternalscript'] == 'true'
       data = self._GetFlattenedData(allow_external_script=allow_external_script)
     else:
       filename = self.ToRealPath(self.GetInputPath())
       data = util.ReadFile(filename, util.BINARY)
 
+    if self.attrs['type'] == 'GZIPPABLE_BINDATA':

[flackr, 2016/05/13 23:02:17]

I don't think this should be a new type. The type of the included file is still
BINDATA. Can we add an attribute like compress="gzip" or a boolean like
compress="true" or gzip="true" instead? Slight preference to compress since it
provides an easy path if we ever want to support a different compression.

[smaier, 2016/05/19 14:17:56]

Done.

+      data = self._GzipString(data)

[flackr, 2016/05/13 23:02:17]

This new behavior should have a test. There's some good examples of creating a
temporary directory / file structure for tests in
tools/grit/grit/gather/chrome_html_unittest.py and reading the output file in
tools/grit/grit/format/rc_unittest.py.

[smaier, 2016/05/19 14:17:56]

Let me know if the unit tests written are sufficient. 

+
     # Include does not care about the encoding, because it only returns binary
     # data.
     return id, data
 
   def Process(self, output_dir):
     """Rewrite file references to be base64 encoded data URLs.  The new file
     will be written to output_dir and the name of the new file is returned."""
     filename = self.ToRealPath(self.GetInputPath())
     flat_filename = os.path.join(output_dir,
         self.attrs['name'] + '_' + os.path.basename(filename))
@@ skipping 40 matching lines @@
     node.StartParsing('include', parent)
     node.HandleAttribute('name', name)
     node.HandleAttribute('type', type)
     node.HandleAttribute('file', file)
     node.HandleAttribute('translateable', translateable)
     node.HandleAttribute('filenameonly', filenameonly)
     node.HandleAttribute('mkoutput', mkoutput)
     node.HandleAttribute('relativepath', relativepath)
     node.EndParsing()
     return node