Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088123002:
Minimize bundled user-agent CSS: html.css (Closed)
|
Created:
4 years, 6 months ago by kouhei (in TOK) Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, kinuko, tzik, Dirk Pranke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMinimize bundled user-agent CSS
User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint.
This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file.
html.css:
original 23451 bytes
minimized 18168 bytes
22% reduction
BUG=472927
Committed: https://crrev.com/4e2d21167e5563cd7306c8d82bb125544c1aba3c
Cr-Commit-Position: refs/heads/master@{#430235}
Patch Set 1 #Patch Set 2 : format #Patch Set 3 : gyp support #Patch Set 4 : avoid touching colon #Patch Set 5 : perf try #
Total comments: 11
Patch Set 6 : remove comment #Patch Set 7 : add unittest #
Total comments: 14
Patch Set 8 : review #
Total comments: 2
Patch Set 9 : review #
Messages
Total messages: 48 (19 generated)
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2088123002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Minimize bundled CSS BUG= ========== to ========== Minimize bundled user-agent CSS User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint. This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file. BUG=472927 ==========
kouhei@chromium.org changed reviewers: + dstockwell@chromium.org, esprehn@chromium.org, timloh@chromium.org
Would you take a look?
esprehn@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@ can you review this Python? I'm not sure if there's a better way to do some of this :) https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/minimize_css.py (right): https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/minimize_css.py:23: WHITESPACE_PATTERN = re.compile(r"\s+", re.MULTILINE) You have this twice https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/minimize_css.py:73: else: This is a lot of code to not have any tests... https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/BUILD.gn:19: "//third_party/WebKit/public/*", Hmm this makes public capable of including core headers. I don't know if we want that https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/blink_resources.grd (right): https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/blink_resources.grd:11: <!-- <include name="IDR_UASTYLE_HTML_CSS" file="../Source/core/css/html.css" type="BINDATA"/> --> Let's make this a comment describing what's going on here instead of just commented out code? https://codereview.chromium.org/2088123002/diff/80001/tools/run-perf-test.cfg File tools/run-perf-test.cfg (right): https://codereview.chromium.org/2088123002/diff/80001/tools/run-perf-test.cfg... tools/run-perf-test.cfg:2: 'command': './tools/perf/run_benchmark --browser=release page_cycler_v2.typical_25', This seems like a mistake, probably not meant for this patch?
Btw please include numbers in the change description, how much smaller does this make the file?
Description was changed from ========== Minimize bundled user-agent CSS User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint. This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file. BUG=472927 ========== to ========== Minimize bundled user-agent CSS User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint. This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file. html.css: original 23451 bytes minimized 18168 bytes 22% reduction BUG=472927 ==========
https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/minimize_css.py (right): https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/minimize_css.py:23: WHITESPACE_PATTERN = re.compile(r"\s+", re.MULTILINE) On 2016/06/27 05:17:31, esprehn wrote: > You have this twice Done. https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/minimize_css.py:73: else: On 2016/06/27 05:17:31, esprehn wrote: > This is a lot of code to not have any tests... Ack. I'll add test before commit. https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/BUILD.gn:19: "//third_party/WebKit/public/*", On 2016/06/27 05:17:31, esprehn wrote: > Hmm this makes public capable of including core headers. I don't know if we want > that Agreed, but I'm not sure about the alternatives. This is to avoid duplicating "blink_core_output_dir". Do you have a suggestion? https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/blink_resources.grd (right): https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/blink_resources.grd:11: <!-- <include name="IDR_UASTYLE_HTML_CSS" file="../Source/core/css/html.css" type="BINDATA"/> --> On 2016/06/27 05:17:31, esprehn wrote: > Let's make this a comment describing what's going on here instead of just > commented out code? Done. https://codereview.chromium.org/2088123002/diff/80001/tools/run-perf-test.cfg File tools/run-perf-test.cfg (right): https://codereview.chromium.org/2088123002/diff/80001/tools/run-perf-test.cfg... tools/run-perf-test.cfg:2: 'command': './tools/perf/run_benchmark --browser=release page_cycler_v2.typical_25', On 2016/06/27 05:17:31, esprehn wrote: > This seems like a mistake, probably not meant for this patch? This is conf file for the perf bot run. Removed.
Can we just use more regexes instead of writing a state machine? e.g.
content = re.sub(r"/\*.*?\*/", "", content, re.S)
content = re.sub(r"\s+", " ", content)
# Remove spaces surrounding ,:;{}! and strings
content = re.sub(r"""\s*([,:;{}!]|".*?"|'.*?')\s*""", r"\1", content)
content = content.replace(";}", "}")
The regex doesn't handle "\"" correctly, but neither does the posted code. Also
removing space after : is not always valid because ": before" is an invalid
selector, but shouldn't matter for UA stylesheets.
I don't know this code, so it's hard to say too much about it without spending more time looking at how it all fits together, but the python all seems reasonable enough otherwise. I don't see any obvious ways that it would be vastly improved :).
esprehn@: Do you have a strong preference here? imo the trade-off of regex-only approach is: [pro] Code gets simpler. No state machine. [con] As we are making certain assumptions (no code like strings, etc.), there will be pitfalls. I'd prefer approach w/o pitfalls, but it is also true that these UA stylesheets are rarely modified and we have layout tests to catch unintended changes. On 2016/06/27 23:32:46, Dirk Pranke wrote: > I don't know this code, so it's hard to say too much about it without spending > more time looking at how it all fits together, but the python all seems > reasonable enough otherwise. I don't see any obvious ways that it would be > vastly improved :). Thanks!
On 2016/06/27 05:46:58, Timothy Loh wrote:
> Can we just use more regexes instead of writing a state machine? e.g.
>
> content = re.sub(r"/\*.*?\*/", "", content, re.S)
> content = re.sub(r"\s+", " ", content)
> # Remove spaces surrounding ,:;{}! and strings
> content = re.sub(r"""\s*([,:;{}!]|".*?"|'.*?')\s*""", r"\1", content)
> content = content.replace(";}", "}")
>
> The regex doesn't handle "\"" correctly, but neither does the posted code.
Also
> removing space after : is not always valid because ": before" is an invalid
> selector, but shouldn't matter for UA stylesheets.
For the record, I'm not generally a fan of using regexes for any but the
simplest things.
I would consider that list of lines at least as unmaintainable as the state
machine, which
you can much more easily step through manually.
The state machine is fine as long as we get some unit tests and it's not too slow in terms of build time. I'd prefer to do whatever is simplest to start, landing the minification is more important than getting the optimal or perfect system going. :) -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
The state machine is fine as long as we get some unit tests and it's not too slow in terms of build time. I'd prefer to do whatever is simplest to start, landing the minification is more important than getting the optimal or perfect system going. :) -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Added unittests + support for quote escapes. PTAL https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/minimize_css.py (right): https://codereview.chromium.org/2088123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/minimize_css.py:73: else: On 2016/06/27 05:39:22, kouhei wrote: > On 2016/06/27 05:17:31, esprehn wrote: > > This is a lot of code to not have any tests... > > Ack. I'll add test before commit. Done.
gentle ping.
On 2016/07/06 at 01:06:13, kouhei wrote: > gentle ping. I'm on vacation this week, maybe one of the other reviewers can help or I'll look next Monday. :)
dstockwell@chromium.org changed reviewers: - dpranke@chromium.org, dstockwell@chromium.org, esprehn@chromium.org
I cleaned up the reviewer list. Patch seems fine to me, but I'll leave it to Tim as he has the parser context.
Hopefully we can land this soon, 22% reduction is pretty sweet. :)
I'd still rather do everything using regexes but I guess this is OK for now. https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/minimize_css.py (right): https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:24: WHITESPACE_PATTERN = re.compile(r"\s+", re.MULTILINE) re.MULTILINE doesn't affect either of these regexes. Also I think it's better to not compile these and just use them directly (iirc the regular python interpreter will cache them anyway, and for the second regex it's easier to see the grouping reference) https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:25: OP_PADDING_PATTERN = re.compile(r";?\s*(?P<op>[\{\};])\s*", re.MULTILINE) I don't think you want the backslashes in [\{\};] https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:42: state = self.MAYBE_COMMENT_START "'" https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:47: elif c == '\"': '"' https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:74: elif c == '\'': "'" https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:118: self._outputs[out_path] = lambda fp=in_file_path: self.generate_implementation(fp) This line is a bit hard for me to read, I'd use partial from functools instead https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/core_generated.gyp (right): https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/core_generated.gyp:795: 'action_name': 'MakeMinimizedCSS', Do we still need to edit gyp files? :)
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/minimize_css.py (right): https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:24: WHITESPACE_PATTERN = re.compile(r"\s+", re.MULTILINE) On 2016/08/10 05:37:39, Timothy Loh wrote: > re.MULTILINE doesn't affect either of these regexes. Also I think it's better to > not compile these and just use them directly (iirc the regular python > interpreter will cache them anyway, and for the second regex it's easier to see > the grouping reference) Done. https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:25: OP_PADDING_PATTERN = re.compile(r";?\s*(?P<op>[\{\};])\s*", re.MULTILINE) On 2016/08/10 05:37:39, Timothy Loh wrote: > I don't think you want the backslashes in [\{\};] Done. https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:42: state = self.MAYBE_COMMENT_START On 2016/08/10 05:37:38, Timothy Loh wrote: > "'" Done. https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:47: elif c == '\"': On 2016/08/10 05:37:38, Timothy Loh wrote: > '"' Done. https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:74: elif c == '\'': On 2016/08/10 05:37:38, Timothy Loh wrote: > "'" Done. https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:118: self._outputs[out_path] = lambda fp=in_file_path: self.generate_implementation(fp) On 2016/08/10 05:37:39, Timothy Loh wrote: > This line is a bit hard for me to read, I'd use partial from functools instead Done. https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/core_generated.gyp (right): https://codereview.chromium.org/2088123002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/core_generated.gyp:795: 'action_name': 'MakeMinimizedCSS', On 2016/08/10 05:37:39, Timothy Loh wrote: > Do we still need to edit gyp files? :) No.
lgtm but I don't understand any of the gn stuff https://codereview.chromium.org/2088123002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/minimize_css.py (right): https://codereview.chromium.org/2088123002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:98: def output(self): Why do we need this? Can we just "return CSSMinimizer().parse(content)" in the function below or something like that?
kouhei@chromium.org changed reviewers: + tzik@chromium.org
+tzik for GN review https://codereview.chromium.org/2088123002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/minimize_css.py (right): https://codereview.chromium.org/2088123002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/minimize_css.py:98: def output(self): On 2016/11/03 06:07:59, Timothy Loh wrote: > Why do we need this? Can we just "return CSSMinimizer().parse(content)" in the > function below or something like that? Done. Removed
lgtm
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2088123002/#ps160001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kouhei@chromium.org changed reviewers: + haraken@chromium.org
+haraken: Would you rs public?
On 2016/11/07 05:51:50, kouhei wrote: > +haraken: Would you rs public? LGTM
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Minimize bundled user-agent CSS User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint. This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file. html.css: original 23451 bytes minimized 18168 bytes 22% reduction BUG=472927 ========== to ========== Minimize bundled user-agent CSS User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint. This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file. html.css: original 23451 bytes minimized 18168 bytes 22% reduction BUG=472927 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Minimize bundled user-agent CSS User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint. This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file. html.css: original 23451 bytes minimized 18168 bytes 22% reduction BUG=472927 ========== to ========== Minimize bundled user-agent CSS User agent CSS such as html.css is processed per renderer start, and blocks time-to-first-meaningful-paint. This CL introduces a simple CSS minimizer which processes html.css before bundling it into resource pak file. html.css: original 23451 bytes minimized 18168 bytes 22% reduction BUG=472927 Committed: https://crrev.com/4e2d21167e5563cd7306c8d82bb125544c1aba3c Cr-Commit-Position: refs/heads/master@{#430235} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4e2d21167e5563cd7306c8d82bb125544c1aba3c Cr-Commit-Position: refs/heads/master@{#430235} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
