Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249493002:
Fixes html inlining if external stylesheet has include directive (Closed)
|
Created:
4 years, 4 months ago by ahmetemirercin Modified:
4 years, 3 months ago CC:
aberent, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionhtml_inline.py ignores <include> directive in external stylesheets. It replaces src with base64 value in externals but when when src attr belongs to include directive, final output has include directive.This commit recheck inline css that can have include directive.
BUG=634704
Committed: https://crrev.com/f0f1f24c00093e0112f7c7956fb07518fc49aa29
Cr-Commit-Position: refs/heads/master@{#416470}
Patch Set 1 #Patch Set 2 : Added unit test for external files containing include directive #
Total comments: 1
Patch Set 3 : Fixes recursive include directives in css files #
Total comments: 10
Patch Set 4 : Fixes recursive include directives in css files #
Total comments: 6
Patch Set 5 : Added new test case #
Messages
Total messages: 18 (5 generated)
Description was changed from ========== Fixes html inlining if external stylesheet has include directive BUG= ========== to ========== html_inline.py ignores <include> directive in external stylesheets. It replaces src with base64 value in externals but when when src attr belongs to include directive, final output has include directive.This commit recheck inline css that can have include directive. BUG=634704 ==========
ahmetemiremir@gmail.com changed reviewers: + flackr@chromium.org, thakis@chromium.org, thestig@chromium.org
It would be good to add a test for this in html_inline_unittest.py.
https://codereview.chromium.org/2249493002/diff/20001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2249493002/diff/20001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:355: flat_text = _INCLUDE_RE.sub(InlineIncludeFiles, flat_text) This won't strip whitespace in the included CSS - which is I think why the include regular expression was moved before this block. Can we do this as part of InlineCSSFile and strip during inlining? Then this could handle a css file with an include to another css file, with another include, etc, right?
On 2016/08/15 14:35:26, flackr wrote: > https://codereview.chromium.org/2249493002/diff/20001/tools/grit/grit/format/... > File tools/grit/grit/format/html_inline.py (right): > > https://codereview.chromium.org/2249493002/diff/20001/tools/grit/grit/format/... > tools/grit/grit/format/html_inline.py:355: flat_text = > _INCLUDE_RE.sub(InlineIncludeFiles, flat_text) > This won't strip whitespace in the included CSS - which is I think why the > include regular expression was moved before this block. Can we do this as part > of InlineCSSFile and strip during inlining? Then this could handle a css file > with an include to another css file, with another include, etc, right? While moving _INCLUDE_RE block into InlineCSSFile, I realized that recursive including files doesn't update |inline_files| list.It is only updated if |name_only| is set true ( from GetResourceFilenames). So I modified InlineFileContents that when including file it gets inlined text and included file list and merge in with old one. So we can update |inlined_file| list recursively. Unit tests have a case that recursively includes files but as I said it is calling GetResourceFilenames and this update |inlined_file| (with name_only parameter) and pretends everything is ok.
https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:242: #To recursively save inlined files,we need InlinedData instance returned nit: Space after comment character here and later. https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:249: inlined_files.update(inlined_data_inst.inlined_files) Do we update the inlined_files list if we're calling this with names_only=True? Can you add a test to verify the correct list in the case of recusrively inlined files? https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:297: text=_INCLUDE_RE.sub(InlineIncludeFiles, nit: spacing "text = _INCLUDE_RE..." https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:302: return pattern % InlineCSSText(text,filepath) nit: only a single space after "return" https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:342: flat_text = _INCLUDE_RE.sub(InlineIncludeFiles, flat_text) Is this still necessary if DoInline is inlining these files? https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:366: flat_text = _INCLUDE_RE.sub(InlineIncludeFiles, flat_text) Is this still necessary? It seems like DoInline is inlining these files.
https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:249: inlined_files.update(inlined_data_inst.inlined_files) On 2016/08/17 17:33:28, flackr wrote: > Do we update the inlined_files list if we're calling this with names_only=True? > Can you add a test to verify the correct list in the case of recusrively inlined > files? "testGetResourceFilenames" in unit test file (1.test case) is actually checking inlined files list with "name_only=true".So I think it's enough,isn't it? https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:366: flat_text = _INCLUDE_RE.sub(InlineIncludeFiles, flat_text) On 2016/08/17 17:33:28, flackr wrote: > Is this still necessary? It seems like DoInline is inlining these files. Yes this line is now unnecessary.
https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:249: inlined_files.update(inlined_data_inst.inlined_files) On 2016/08/18 at 21:53:31, ahmetemirercin wrote: > On 2016/08/17 17:33:28, flackr wrote: > > Do we update the inlined_files list if we're calling this with names_only=True? > > Can you add a test to verify the correct list in the case of recusrively inlined > > files? > > "testGetResourceFilenames" in unit test file (1.test case) is actually checking inlined files list with "name_only=true".So I think it's enough,isn't it? That test doesn't have an include within an inlined CSS file though, perhaps add that to that test. https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:242: # To recursively save inlined files,we need InlinedData instance returned nit, space after comma. https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:298: util.ReadFile(filepath, util.BINARY)) nit: align with InlineIncludeFiles above. https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:302: return pattern % InlineCSSText(text,filepath) nit: space after comma.
https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2249493002/diff/40001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:249: inlined_files.update(inlined_data_inst.inlined_files) On 2016/08/22 14:41:00, flackr wrote: > On 2016/08/18 at 21:53:31, ahmetemirercin wrote: > > On 2016/08/17 17:33:28, flackr wrote: > > > Do we update the inlined_files list if we're calling this with > names_only=True? > > > Can you add a test to verify the correct list in the case of recusrively > inlined > > > files? > > > > "testGetResourceFilenames" in unit test file (1.test case) is actually > checking inlined files list with "name_only=true".So I think it's enough,isn't > it? > > That test doesn't have an include within an inlined CSS file though, perhaps add > that to that test. Added new test covering includes in css files. https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:242: # To recursively save inlined files,we need InlinedData instance returned On 2016/08/22 14:41:00, flackr wrote: > nit, space after comma. Done. https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:298: util.ReadFile(filepath, util.BINARY)) On 2016/08/22 14:41:00, flackr wrote: > nit: align with InlineIncludeFiles above. Done. https://codereview.chromium.org/2249493002/diff/60001/tools/grit/grit/format/... tools/grit/grit/format/html_inline.py:302: return pattern % InlineCSSText(text,filepath) On 2016/08/22 14:41:00, flackr wrote: > nit: space after comma. Done.
lgtm
On 2016/08/23 at 14:45:21, flackr wrote: > lgtm Sorry I didn't notice this earlier, but in the long term we should probably move to using the @import syntax for inlining CSS since that's actually proper CSS for an include: https://developer.mozilla.org/en-US/docs/Web/CSS/@import
The CQ bit was checked by ahmetemiremir@gmail.com
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 ========== html_inline.py ignores <include> directive in external stylesheets. It replaces src with base64 value in externals but when when src attr belongs to include directive, final output has include directive.This commit recheck inline css that can have include directive. BUG=634704 ========== to ========== html_inline.py ignores <include> directive in external stylesheets. It replaces src with base64 value in externals but when when src attr belongs to include directive, final output has include directive.This commit recheck inline css that can have include directive. BUG=634704 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== html_inline.py ignores <include> directive in external stylesheets. It replaces src with base64 value in externals but when when src attr belongs to include directive, final output has include directive.This commit recheck inline css that can have include directive. BUG=634704 ========== to ========== html_inline.py ignores <include> directive in external stylesheets. It replaces src with base64 value in externals but when when src attr belongs to include directive, final output has include directive.This commit recheck inline css that can have include directive. BUG=634704 Committed: https://crrev.com/f0f1f24c00093e0112f7c7956fb07518fc49aa29 Cr-Commit-Position: refs/heads/master@{#416470} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f0f1f24c00093e0112f7c7956fb07518fc49aa29 Cr-Commit-Position: refs/heads/master@{#416470} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
