Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710933002:
grit: add support for inlining img srcset (Closed)
|
Created:
3 years, 10 months ago by Alexander Alekseev Modified:
3 years, 10 months ago Reviewers:
Nico CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongrit: add support for inlining img srcset
This CL adds support for inlining srcset attributes of img tags.
R=thakis@chromium.org
BUG=690725
Review-Url: https://codereview.chromium.org/2710933002
Cr-Commit-Position: refs/heads/master@{#452297}
Committed: https://chromium.googlesource.com/chromium/src/+/841b84cb8f7eeb783b22af63a2c9f317d5b24691
Patch Set 1 #
Total comments: 19
Patch Set 2 : Update after review. #
Total comments: 2
Patch Set 3 : Update after review #
Dependent Patchsets: Messages
Total messages: 19 (12 generated)
Please review. We need this to insert set of icons into ChromeOS OOBE code.
The CQ bit was checked by alemate@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/2710933002/diff/1/tools/grit/grit/format/html... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:53: _SRCSET_RE = lazy_re.compile( i always find it useful to have "# matches `<img srcset="...">` type comments above regexes https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:57: r'\s*(?P<url>[^,]\S+)\s+(?P<dsc>[\deE.-]+[wx])\s*', [^,]\S+ looks weird to me. Why the [^,]? Use a more descriptive capturing group name than "dsc" Your dsc matches things like "....eeee" so why not just do \S+ for the scale part. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:78: def File2DataURL(filename, base_path, distribution, inlined_files, names_only): no cute spelling please. ConvertFileToDataURL. and since this does more than what the name says, give it a comment explaining what it does https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:125: return src_match.group(0) File2DataURL() does this too; did you mean to delete these 3 lines? https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:128: inlined_files, names_only) weird indent https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:139: filename_expansion_function=None): made expansion_function default to `lambda x: x` https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:140: """regex replace function. ? https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:179: for el in parts: s/el/part https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:200: filename = filename_expansion_function(filename) with the new default, you can do this unconditionally
https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:53: _SRCSET_RE = lazy_re.compile( On 2017/02/22 21:11:22, Nico wrote: > i always find it useful to have "# matches `<img srcset="...">` type comments > above regexes Done. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:57: r'\s*(?P<url>[^,]\S+)\s+(?P<dsc>[\deE.-]+[wx])\s*', On 2017/02/22 21:11:22, Nico wrote: > [^,]\S+ looks weird to me. Why the [^,]? Because otherwize "img1.png 1x,img2.png 2x" will be split into ["img1.png", "1x", ",img2.png", "2x"]. HTML 5.2 also states that "URL cannot start with comma". On the other hand, URL can contain comma: "data:image/png;base64,YTU= 1x,data:image/png;base64,YTY= 2x" > Use a more descriptive capturing group name than "dsc" http://w3c.github.io/html/semantics-embedded-content.html#element-attrdef-img... HTML 5.2 names the second term "width descriptor" () and "pixel density descriptor". So I used "descriptor" for both. > Your dsc matches things like "....eeee" so why not just do \S+ > for the scale part. The regular expression to check for "valid floating-point number" is too complex to be readable. On the other hand, we do not want to silently replace something unexpected. (It obviously cannot contain ','). So I decided that restricting a list of characters to "valid non-negative integer + letter 'x'" plus "valid floating-point number + letter 'w'" is a reasonable compromise. I added comment on this. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:78: def File2DataURL(filename, base_path, distribution, inlined_files, names_only): On 2017/02/22 21:11:22, Nico wrote: > no cute spelling please. ConvertFileToDataURL. > > and since this does more than what the name says, give it a comment explaining > what it does Done. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:125: return src_match.group(0) On 2017/02/22 21:11:22, Nico wrote: > File2DataURL() does this too; did you mean to delete these 3 lines? Done. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:128: inlined_files, names_only) On 2017/02/22 21:11:22, Nico wrote: > weird indent Done. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:139: filename_expansion_function=None): On 2017/02/22 21:11:22, Nico wrote: > made expansion_function default to `lambda x: x` It turned out that we would need to change defaults also in tools/grit/grit/gather/chrome_html.py which seems more suitable to a different CL/ https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:140: """regex replace function. On 2017/02/22 21:11:22, Nico wrote: > ? Updated. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:179: for el in parts: On 2017/02/22 21:11:22, Nico wrote: > s/el/part Done. https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:200: filename = filename_expansion_function(filename) On 2017/02/22 21:11:22, Nico wrote: > with the new default, you can do this unconditionally Done.
The CQ bit was checked by alemate@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.
lgtm https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2710933002/diff/1/tools/grit/grit/format/html... tools/grit/grit/format/html_inline.py:57: r'\s*(?P<url>[^,]\S+)\s+(?P<dsc>[\deE.-]+[wx])\s*', On 2017/02/22 22:20:20, Alexander Alekseev wrote: > On 2017/02/22 21:11:22, Nico wrote: > > [^,]\S+ looks weird to me. Why the [^,]? > > Because otherwize "img1.png 1x,img2.png 2x" will be split into ["img1.png", > "1x", ",img2.png", "2x"]. HTML 5.2 also states that "URL cannot start with > comma". > > On the other hand, URL can contain comma: > "data:image/png;base64,YTU= 1x,data:image/png;base64,YTY= 2x" Hm, ok. I would've put a `,?` at the end so that the comma gets matched when parsing the first part instead of the second, but I guess it's a matter of taste. https://codereview.chromium.org/2710933002/diff/20001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2710933002/diff/20001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:64: # that form both of them. Add "# Matches for example 'asdf.png 3.3x'. Having a concrete string is super useful imho.
https://codereview.chromium.org/2710933002/diff/20001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2710933002/diff/20001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:64: # that form both of them. On 2017/02/22 22:44:42, Nico wrote: > Add "# Matches for example 'asdf.png 3.3x'. Having a concrete string is super > useful imho. Done.
The CQ bit was checked by alemate@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2710933002/#ps40001 (title: "Update after review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487806785576090,
"parent_rev": "dfb7cbbba62e8e408a52bafdb3dab575f4ad1dbc", "commit_rev":
"841b84cb8f7eeb783b22af63a2c9f317d5b24691"}
Message was sent while issue was closed.
Description was changed from ========== grit: add support for inlining img srcset This CL adds support for inlining srcset attributes of img tags. R=thakis@chromium.org BUG=690725 ========== to ========== grit: add support for inlining img srcset This CL adds support for inlining srcset attributes of img tags. R=thakis@chromium.org BUG=690725 Review-Url: https://codereview.chromium.org/2710933002 Cr-Commit-Position: refs/heads/master@{#452297} Committed: https://chromium.googlesource.com/chromium/src/+/841b84cb8f7eeb783b22af63a2c9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/841b84cb8f7eeb783b22af63a2c9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
