Skip to content

Commit

Permalink
Reland "Add brotli compression to grit"
Browse files Browse the repository at this point in the history
This reverts commit 1b4225a.

Reason for revert: Bug filed to fix broken Asan Builders, added temp workaround

Original change's description:
> Revert "Add brotli compression to grit"
>
> This reverts commit a0d64f3.
>
> Reason for revert: Suspecting this CL of breaking generate_build_files step in Mac ASan 64 Builder.
>
> First failure here: https://ci.chromium.org/p/chromium/builders/ci/Mac%20ASan%2064%20Builder/86877
>
> Original change's description:
> > Add brotli compression to grit
> >
> > -Add the "compress='brotli'" flag option in grdp files
> > -Grit will now automatically brotli compress if flag is included
> > -Previous Cl that was split into this one:
> > https://chromium-review.googlesource.com/c/chromium/src/+/1660352
> >
> >
> > Bug: 826858
> > Change-Id: I010ad1650394e4bdd58e066486b161cf74f2fff5
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692234
> > Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> > Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#675763}
>
> TBR=dbeam@chromium.org,dpapad@chromium.org,agrieve@chromium.org,tiborg@chromium.org,smaier@chromium.org,jacqueschen@google.com
>
> Change-Id: I724ed738b56529fe4b4ac1c2ad3e0b9adbee9671
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 826858
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1693809
> Reviewed-by: Hector Carmona <hcarmona@chromium.org>
> Commit-Queue: Hector Carmona <hcarmona@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#675839}


Change-Id: I82ff2a6de6ea754923318900d60c1f845fa59a33
Bug: 826858
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1695881
Commit-Queue: Jacques Chen <jacqueschen@google.com>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Sam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677818}
  • Loading branch information
Jacques Chen authored and Commit Bot committed Jul 16, 2019
1 parent bf73b60 commit 6d681c6
Show file tree
Hide file tree
Showing 9 changed files with 157 additions and 27 deletions.
1 change: 1 addition & 0 deletions tools/binary_size/supersize.pydeps
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
../grit/grit/lazy_re.py
../grit/grit/node/__init__.py
../grit/grit/node/base.py
../grit/grit/node/brotli_util.py
../grit/grit/node/include.py
../grit/grit/node/message.py
../grit/grit/node/misc.py
Expand Down
15 changes: 15 additions & 0 deletions tools/grit/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
# This target creates a stamp file that depends on all the sources in the grit
# directory. By depending on this, a target can force itself to be rebuilt if
# grit itself changes.

import("//build/config/sanitizers/sanitizers.gni")

action("grit_sources") {
depfile = "$target_out_dir/grit_sources.d"
script = "stamp_grit_sources.py"
Expand Down Expand Up @@ -37,3 +40,15 @@ group("grit_python_unittests") {
"//third_party/catapult/third_party/typ/",
]
}

# See https://crbug.com/983200
if (is_mac && is_asan) {
create_bundle("brotli_mac_asan_workaround") {
bundle_root_dir = "$target_out_dir/$target_name"
bundle_executable_dir = bundle_root_dir

public_deps = [
"//third_party/brotli:brotli($host_toolchain)",
]
}
}
5 changes: 5 additions & 0 deletions tools/grit/grit/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,8 @@
CONSTANT_LANGUAGE = 'x_constant'

FAKE_BIDI = 'fake-bidi'

# Magic number added to the header of resources brotli compressed by grit. Used
# to easily identify resources as being brotli compressed. See
# ui/base/resource/resource_bundle.h for decompression usage.
BROTLI_CONST = '\x1e\x9b'
39 changes: 30 additions & 9 deletions tools/grit/grit/node/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@

import ast
import os
import struct
import sys
import types
from xml.sax import saxutils

from grit import constants
from grit import clique
from grit import exception
from grit import util
from grit.node import brotli_util
import grit.format.gzip_string


Expand Down Expand Up @@ -604,18 +607,36 @@ def CompressDataIfNeeded(self, data):
Args:
data: The data to compressed.
Returns:
The data in compressed format. If the format was unknown then this returns
the data uncompressed.
The data in gzipped or brotli compressed format. If the format is
unspecified then this returns the data uncompressed.
'''
if self.attrs.get('compress') != 'gzip':
if self.attrs.get('compress') == 'gzip':
# We only use rsyncable compression on Linux.
# We exclude ChromeOS since ChromeOS bots are Linux based but do not have
# the --rsyncable option built in for gzip. See crbug.com/617950.
if sys.platform == 'linux2' and 'chromeos' not in self.GetRoot().defines:
return grit.format.gzip_string.GzipStringRsyncable(data)
return grit.format.gzip_string.GzipString(data)

elif self.attrs.get('compress') == 'brotli':
# The length of the uncompressed data as 8 bytes little-endian.
size_bytes = struct.pack("<q", len(data))
data = brotli_util.BrotliCompress(data)
# BROTLI_CONST is prepended to brotli decompressed data in order to
# easily check if a resource has been brotli compressed.
# The length of the uncompressed data is also appended to the start,
# truncated to 6 bytes, little-endian. size_bytes is 8 bytes,
# need to truncate further to 6.
formatter = '%ds %dx %ds' % (6, 2, len(size_bytes) - 8)
return (constants.BROTLI_CONST +
b''.join(struct.unpack(formatter, size_bytes)) +
data)

elif self.attrs.get('compress') == 'false':
return data

# We only use rsyncable compression on Linux.
# We exclude ChromeOS since ChromeOS bots are Linux based but do not have
# the --rsyncable option built in for gzip. See crbug.com/617950.
if sys.platform == 'linux2' and 'chromeos' not in self.GetRoot().defines:
return grit.format.gzip_string.GzipStringRsyncable(data)
return grit.format.gzip_string.GzipString(data)
else:
raise Exception('Invalid value for compression')


class ContentNode(Node):
Expand Down
24 changes: 24 additions & 0 deletions tools/grit/grit/node/brotli_util.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Copyright 2019 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.

"""Framework for compressing resources using Brotli."""

import subprocess

__brotli_executable = None


def SetBrotliCommand(brotli):
# brotli is a list. In production it contains the path to the Brotli executable.
# During testing it contains [python, mock_brotli.py] for testing on Windows.
global __brotli_executable
__brotli_executable = brotli


def BrotliCompress(data):
if not __brotli_executable:
raise Exception('SetBrotliCommand has not been called yet!')
compress = subprocess.Popen(__brotli_executable + ['-', '-f'],
stdin=subprocess.PIPE, stdout=subprocess.PIPE)
return compress.communicate(data)[0]
10 changes: 10 additions & 0 deletions tools/grit/grit/node/mock_brotli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env python
# Copyright 2019 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.

"""Mock Brotli Executable for testing purposes."""

import sys

sys.stdout.write('This has been mock compressed!')
49 changes: 41 additions & 8 deletions tools/grit/grit/node/structure_unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@
import tempfile
import unittest
import StringIO
import struct

from grit import constants
from grit import util
from grit.node import brotli_util
from grit.node import empty
from grit.node import misc
from grit.node import structure
Expand Down Expand Up @@ -106,27 +109,57 @@ def testCompressGzip(self):
<structure name="TEST_TXT" file="test_text.txt"
compress="gzip" type="chrome_html" />
</structures>''', base_dir=test_data_root)
struct, = root.GetChildrenOfType(structure.StructureNode)
struct.RunPreSubstitutionGatherer()
compressed = struct.GetDataPackValue(lang='en', encoding=1)
node, = root.GetChildrenOfType(structure.StructureNode)
node.RunPreSubstitutionGatherer()
compressed = node.GetDataPackValue(lang='en', encoding=1)

decompressed_data = zlib.decompress(compressed, 16 + zlib.MAX_WBITS)
self.assertEqual(util.ReadFile(
os.path.join(test_data_root, "test_text.txt"), util.BINARY),
os.path.join(test_data_root, 'test_text.txt'), util.BINARY),
decompressed_data)

def testCompressBrotli(self):
test_data_root = util.PathFromRoot('grit/testdata')
root = util.ParseGrdForUnittest(
'''
<structures>
<structure name="TEST_TXT" file="test_text.txt"
compress="brotli" type="chrome_html" />
</structures>''',
base_dir=test_data_root)
node, = root.GetChildrenOfType(structure.StructureNode)
node.RunPreSubstitutionGatherer()

# Using the mock brotli decompression executable.
brotli_util.SetBrotliCommand([sys.executable,
os.path.join(os.path.dirname(__file__),
'mock_brotli.py')])
compressed = node.GetDataPackValue(lang='en', encoding=1)
# Assert that the first two bytes in compressed format is BROTLI_CONST.
self.assertEqual(constants.BROTLI_CONST, compressed[0:2])

# Compare the actual size of the uncompressed test data with
# the size appended during compression.
actual_size = len(util.ReadFile(
os.path.join(test_data_root, 'test_text.txt'), util.BINARY))
uncompress_size = struct.unpack('<i', compressed[2:6])[0]
uncompress_size += struct.unpack('<h', compressed[6:8])[0] << 4*8
self.assertEqual(actual_size, uncompress_size)

self.assertEqual('This has been mock compressed!', compressed[8:])

def testNotCompressed(self):
test_data_root = util.PathFromRoot('grit/testdata')
root = util.ParseGrdForUnittest('''
<structures>
<structure name="TEST_TXT" file="test_text.txt" type="chrome_html" />
</structures>''', base_dir=test_data_root)
struct, = root.GetChildrenOfType(structure.StructureNode)
struct.RunPreSubstitutionGatherer()
data = struct.GetDataPackValue(lang='en', encoding=1)
node, = root.GetChildrenOfType(structure.StructureNode)
node.RunPreSubstitutionGatherer()
data = node.GetDataPackValue(lang='en', encoding=1)

self.assertEqual(util.ReadFile(
os.path.join(test_data_root, "test_text.txt"), util.BINARY), data)
os.path.join(test_data_root, 'test_text.txt'), util.BINARY), data)


if __name__ == '__main__':
Expand Down
23 changes: 13 additions & 10 deletions tools/grit/grit/tool/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from grit import shortcuts
from grit import util
from grit.format import minifier
from grit.node import brotli_util
from grit.node import include
from grit.node import message
from grit.node import structure
Expand Down Expand Up @@ -133,6 +134,10 @@ class RcBuilder(interface.Tool):
minified Javascript to standard output. A non-zero exit
status will be taken as indicating failure.
--brotli The full path to the brotli executable generated by
third_party/brotli/BUILD.gn, required if any entries use
compress="brotli".
Conditional inclusion of resources only affects the output of files which
control which resources get linked into a binary, e.g. it affects .rc files
meant for compilation but it does not affect resource header files (that define
Expand All @@ -158,16 +163,12 @@ def Run(self, opts, args):
depend_on_stamp = False
js_minifier = None
replace_ellipsis = True
(own_opts, args) = getopt.getopt(args, 'a:p:o:D:E:f:w:t:',
('depdir=','depfile=','assert-file-list=',
'help',
'output-all-resource-defines',
'no-output-all-resource-defines',
'no-replace-ellipsis',
'depend-on-stamp',
'js-minifier=',
'write-only-new=',
'whitelist-support'))
(own_opts, args) = getopt.getopt(
args, 'a:p:o:D:E:f:w:t:',
('depdir=', 'depfile=', 'assert-file-list=', 'help',
'output-all-resource-defines', 'no-output-all-resource-defines',
'no-replace-ellipsis', 'depend-on-stamp', 'js-minifier=',
'write-only-new=', 'whitelist-support', 'brotli='))
for (key, val) in own_opts:
if key == '-a':
assert_output_files.append(val)
Expand Down Expand Up @@ -207,6 +208,8 @@ def Run(self, opts, args):
js_minifier = val
elif key == '--whitelist-support':
whitelist_support = True
elif key == '--brotli':
brotli_util.SetBrotliCommand([os.path.abspath(val)])
elif key == '--help':
self.ShowUsage()
sys.exit(0)
Expand Down
18 changes: 18 additions & 0 deletions tools/grit/grit_rule.gni
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ import("//build/config/compiler/compiler.gni")
import("//build/config/compute_inputs_for_analyze.gni")
import("//build/config/crypto.gni")
import("//build/config/features.gni")
import("//build/config/sanitizers/sanitizers.gni")
import("//build/config/ui.gni")
import("//build/toolchain/gcc_toolchain.gni") # For enable_resource_whitelist_generation
import("//third_party/closure_compiler/closure_args.gni")
Expand Down Expand Up @@ -348,6 +349,16 @@ template("grit") {
depfile = "$depfile_dir/${grit_output_name}_stamp.d"
outputs = [ "${depfile}.stamp" ] + grit_outputs + pak_info_outputs

brotli_target = "//third_party/brotli:brotli($host_toolchain)"

brotli_executable = get_label_info(brotli_target, "root_out_dir") + "/" +
get_label_info(brotli_target, "name")
if (host_os == "win") {
brotli_executable += ".exe"
}

inputs += [ brotli_executable ]

args = [
"-i",
source_path,
Expand All @@ -360,6 +371,8 @@ template("grit") {
rebase_path(depfile, root_build_dir),
"--write-only-new=1",
"--depend-on-stamp",
"--brotli",
rebase_path(brotli_executable, root_build_dir),
] + grit_defines

# Add extra defines with -D flags.
Expand Down Expand Up @@ -443,6 +456,11 @@ template("grit") {
deps = [
"//tools/grit:grit_sources",
]
if (is_mac && is_asan) {
deps += [ "//tools/grit:brotli_mac_asan_workaround" ]
} else {
deps += [ brotli_target ]
}
if (defined(invoker.deps)) {
deps += invoker.deps
}
Expand Down

0 comments on commit 6d681c6

Please sign in to comment.