Add a PRESUBMIT check for broken .ico files.

This is in reaction to crbug.com/526622, where the Chrome .ico files
had a bad mask, making them render incorrectly on Windows. The presubmit
now checks that icons have a properly formed mask, as well as that the
large image is a compressed PNG (not an uncompressed BMP).

Also adds a --lint argument to the optimize-ico-files script that
manually runs the same checks on a given .ico file.

BUG=534679

Review-Url: https://codereview.chromium.org/1372113003
Cr-Commit-Position: refs/heads/master@{#430777}

chrome/app/theme/PRESUBMIT.py

@@ -36,11 +36,13 @@ def _CommonChecks(input_api, output_api):
 
   try:
     sys.path = [resources] + old_path
-    from resource_check import resource_scale_factors
+    from resource_check import resource_scale_factors, ico_files
 
     for paths in path_scales:
       results.extend(resource_scale_factors.ResourceScaleFactors(
           input_api, output_api, paths).RunChecks())
+
+    results.extend(ico_files.IcoFiles(input_api, output_api).RunChecks())
   finally:
     sys.path = old_path
 

tools/resources/ico_tools.py

@@ -70,6 +70,11 @@ def OptimizePng(png_data, optimization_level=None):
       os.unlink(png_filename)
     os.rmdir(temp_dir)
 
+def BytesPerRowBMP(width, bpp):
+  """Computes the number of bytes per row in a Windows BMP image."""
+  # width * bpp / 8, rounded up to the nearest multiple of 4.
+  return int(math.ceil(width * bpp / 32.0)) * 4
+
 def ExportSingleEntry(icon_dir_entry, icon_data, outfile):
   """Export a single icon dir entry to its own ICO file.
 
@@ -171,11 +176,46 @@ def ComputeANDMaskFromAlpha(image_data, width, height):
   and_bytes = ''.join(map(chr, and_bytes))
   return and_bytes
 
-def RebuildANDMask(iconimage):
-  """Rebuild the AND mask in an icon image.
+def CheckANDMaskAgainstAlpha(xor_data, and_data, width, height):
+  """Checks whether an AND mask is "good" for 32-bit BGRA image data.
+
+  This checks that the mask is opaque wherever the alpha channel is not fully
+  transparent. Pixels that violate this condition will show up as black in some
+  contexts in Windows (http://crbug.com/526622). Also checks the inverse
+  condition, that the mask is transparent wherever the alpha channel is fully
+  transparent. While this does not appear to be strictly necessary, it is good
+  practice for backwards compatibility.
+
+  Returns True if the AND mask is "good", False otherwise.
+  """
+  xor_bytes_per_row = width * 4
+  and_bytes_per_row = BytesPerRowBMP(width, 1)
+
+  for y in range(height):
+    for x in range(width):
+      alpha = ord(xor_data[y * xor_bytes_per_row + x * 4 + 3])
+      mask = bool(ord(and_data[y * and_bytes_per_row + x // 8]) &
+                  (1 << (7 - (x % 8))))
+
+      if mask:
+        if alpha > 0:
+          # mask is transparent, alpha is partially or fully opaque. This pixel
+          # can show up as black on Windows due to a rendering bug.
+          return False
+      else:
+        if alpha == 0:
+          # mask is opaque, alpha is transparent. This pixel should be marked as
+          # transparent in the mask, for legacy reasons.
+          return False
+
+  return True
+
+def CheckOrRebuildANDMask(iconimage, rebuild=False):
+  """Checks the AND mask in an icon image for correctness, or rebuilds it.
 
   GIMP (<=2.8.14) creates a bad AND mask on 32-bit icon images (pixels with <50%
-  opacity are marked as transparent, which end up looking black on Windows). So,
+  opacity are marked as transparent, which end up looking black on Windows).
+  With rebuild == False, checks whether the mask is bad. With rebuild == True,
   if this is a 32-bit image, throw the mask away and recompute it from the alpha
   data. (See: https://bugzilla.gnome.org/show_bug.cgi?id=755200)
 
@@ -185,7 +225,8 @@ def RebuildANDMask(iconimage):
       is not 32-bit, this is a no-op).
 
   Returns:
-    An updated |iconimage|, with the AND mask re-computed using
+    If rebuild == False, a bool indicating whether the mask is "good". If
+    rebuild == True, an updated |iconimage|, with the AND mask re-computed using
     ComputeANDMaskFromAlpha.
   """
   # Parse BITMAPINFOHEADER.
@@ -195,20 +236,85 @@ def RebuildANDMask(iconimage):
   if bpp != 32:
     # No alpha channel, so the mask cannot be "wrong" (it is the only source of
     # transparency information).
-    return iconimage
+    return iconimage if rebuild else True
 
   height /= 2
-  xor_size = int(math.ceil(width * bpp / 32.0)) * 4 * height
+  xor_size = BytesPerRowBMP(width, bpp) * height
 
   # num_colors can be 0, implying 2^bpp colors.
   xor_palette_size = (num_colors or (1 << bpp if bpp < 24 else 0)) * 4
   xor_data = iconimage[40 + xor_palette_size :
                        40 + xor_palette_size + xor_size]
 
-  and_data = ComputeANDMaskFromAlpha(xor_data, width, height)
+  if rebuild:
+    and_data = ComputeANDMaskFromAlpha(xor_data, width, height)
+
+    # Replace the AND mask in the original icon data.
+    return iconimage[:40 + xor_palette_size + xor_size] + and_data
+  else:
+    and_data = iconimage[40 + xor_palette_size + xor_size:]
+    return CheckANDMaskAgainstAlpha(xor_data, and_data, width, height)
+
+def LintIcoFile(infile):
+  """Read an ICO file and check whether it is acceptable.
+
+  This checks for:
+  - Basic structural integrity of the ICO.
+  - Large BMPs that could be converted to PNGs.
+  - 32-bit BMPs with buggy AND masks.
+
+  It will *not* check whether PNG images have been compressed sufficiently.
+
+  Args:
+    infile: The file to read from. Must be a seekable file-like object
+      containing a Microsoft ICO file.
+
+  Returns:
+    A sequence of strings, containing error messages. An empty sequence
+    indicates a good icon.
+  """
+  filename = os.path.basename(infile.name)
+  icondir = infile.read(6)
+  zero, image_type, num_images = struct.unpack('<HHH', icondir)
+  if zero != 0:
+    yield 'Invalid ICO: First word must be 0.'
+    return
+
+  if image_type not in (1, 2):
+    yield 'Invalid ICO: Image type must be 1 or 2.'
+    return
+
+  # Read and unpack each ICONDIRENTRY.
+  icon_dir_entries = []
+  for i in range(num_images):
+    icondirentry = infile.read(16)
+    icon_dir_entries.append(struct.unpack('<BBBBHHLL', icondirentry))
+
+  # Read each icon's bitmap data.
+  current_offset = infile.tell()
+  icon_bitmap_data = []
+  for i in range(num_images):
+    width, height, num_colors, r1, r2, r3, size, _ = icon_dir_entries[i]
+    width = width or 256
+    height = height or 256
+    offset = current_offset
+    icon_data = infile.read(size)
+    if len(icon_data) != size:
+      yield 'Invalid ICO: Unexpected end of file'
+      return
+
+    entry_is_png = IsPng(icon_data)
+    logging.debug('%s entry #%d: %dx%d, %d bytes (%s)', filename, i + 1, width,
+                  height, size, 'PNG' if entry_is_png else 'BMP')
+
+    if not entry_is_png:
+      if width >= 256 or height >= 256:
+        yield ('Entry #%d is a large image in uncompressed BMP format. It '
+               'should be in PNG format.' % (i + 1))
 
-  # Replace the AND mask in the original icon data.
-  return iconimage[:40 + xor_palette_size + xor_size] + and_data
+      if not CheckOrRebuildANDMask(icon_data, rebuild=False):
+        yield ('Entry #%d has a bad mask that will display incorrectly in some '
+               'places in Windows.' % (i + 1))
 
 def OptimizeIcoFile(infile, outfile, optimization_level=None):
   """Read an ICO file, optimize its PNGs, and write the output to outfile.
@@ -259,7 +365,7 @@ def OptimizeIcoFile(infile, outfile, optimization_level=None):
       # all of the images. https://crbug.com/663136
       icon_data = OptimizeBmp(icon_dir_entries[i], icon_data)
     else:
-      new_icon_data = RebuildANDMask(icon_data)
+      new_icon_data = CheckOrRebuildANDMask(icon_data, rebuild=True)
       if new_icon_data != icon_data:
         logging.info('  * Rebuilt AND mask for this image from alpha channel.')
         icon_data = new_icon_data

tools/resources/optimize-ico-files.py

@@ -32,6 +32,9 @@ def main(args=None):
                       nargs='+', help='.ico files to be crushed')
   parser.add_argument('-o', dest='optimization_level', metavar='OPT', type=int,
                       help='optimization level')
+  parser.add_argument('--lint', dest='lint', action='store_true',
+                      help='test the ICO file without modifying (set status '
+                      'to 1 on error)')
   parser.add_argument('-d', '--debug', dest='debug', action='store_true',
                       help='enable debug logging')
 
@@ -40,11 +43,20 @@ def main(args=None):
   if args.debug:
     logging.getLogger().setLevel(logging.DEBUG)
 
+  failed = False
   for file in args.files:
     buf = StringIO.StringIO()
     file.seek(0, os.SEEK_END)
     old_length = file.tell()
     file.seek(0, os.SEEK_SET)
+
+    if args.lint:
+      for error in ico_tools.LintIcoFile(file):
+        logging.warning('%s: %s', file.name, error)
+        # Any errors should cause this process to exit with a status of 1.
+        failed = True
+      continue
+
     ico_tools.OptimizeIcoFile(file, buf, args.optimization_level)
 
     new_length = len(buf.getvalue())
@@ -63,5 +75,7 @@ def main(args=None):
       logging.info('%s : %d => %d (%d bytes : %d %%)', file.name, old_length,
                    new_length, saving, int(saving_percent * 100))
 
+  return failed
+
 if __name__ == '__main__':
   sys.exit(main())

ui/resources/resource_check/ico_files.py

@@ -0,0 +1,57 @@
+# Copyright 2016 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.
+
+"""Presubmit script for Chromium browser resources.
+
+See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
+for more details about the presubmit API built into depot_tools, and see
+http://www.chromium.org/developers/web-development-style-guide for the rules
+we're checking against here.
+"""
+
+import os
+import sys
+
+class IcoFiles(object):
+  """Verifier of ICO files for Chromium resources.
+  """
+
+  def __init__(self, input_api, output_api):
+    """ Initializes IcoFiles with path."""
+    self.input_api = input_api
+    self.output_api = output_api
+
+    tool_path = input_api.os_path.join(input_api.PresubmitLocalPath(),
+        '../../../tools/resources')
+    sys.path.insert(0, tool_path)
+
+  def RunChecks(self):
+    """Verifies the correctness of the ICO files.
+
+    Returns:
+        An array of presubmit errors if any ICO files were broken in some way.
+    """
+    results = []
+    affected_files = self.input_api.AffectedFiles(include_deletes=False)
+    for f in affected_files:
+      path = f.LocalPath()
+      if os.path.splitext(path)[1].lower() != '.ico':
+        continue
+
+      # Import this module from here (to avoid importing it in the highly common
+      # case where there are no ICO files being changed).
+      import ico_tools
+
+      repository_path = self.input_api.change.RepositoryRoot()
+
+      with open(os.path.join(repository_path, path), 'rb') as ico_file:
+        errors = list(ico_tools.LintIcoFile(ico_file))
+        if errors:
+          error_string = '\n'.join('    * ' + e for e in errors)
+          results.append(self.output_api.PresubmitError(
+              '%s: This file does not meet the standards for Chromium ICO '
+              'files.\n%s\n    Please run '
+              'tools/resources/optimize-ico-files.py on this file. See '
+              'chrome/app/theme/README for details.' % (path, error_string)))
+    return results