diff --git a/chrome/app/theme/PRESUBMIT.py b/chrome/app/theme/PRESUBMIT.py index 9d0fc31a957e31..5c5353f75fb520 100644 --- a/chrome/app/theme/PRESUBMIT.py +++ b/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 diff --git a/tools/resources/ico_tools.py b/tools/resources/ico_tools.py index 9c3128b56a7e0d..db0cda11853c20 100644 --- a/tools/resources/ico_tools.py +++ b/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('= 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 diff --git a/tools/resources/optimize-ico-files.py b/tools/resources/optimize-ico-files.py index 2635e9c509ba5c..5f0f7163990df7 100755 --- a/tools/resources/optimize-ico-files.py +++ b/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()) diff --git a/ui/resources/resource_check/ico_files.py b/ui/resources/resource_check/ico_files.py new file mode 100644 index 00000000000000..26bd36788c9d8f --- /dev/null +++ b/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