Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1424503006:
Page Serializer: Specify encoding for serialized CSS files (Closed)
DescriptionPage Serializer: Specify encoding for serialized CSS files
The input encoding (per a @charset attribute or Content-Type header)
was used when serializing CSS files, but the encoding directive
was not written back out.
To fix, always write out the @charset directive per
http://www.w3.org/International/questions/qa-css-charset
BUG=548031
R=yosin@chromium.org
TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS'
Committed: https://crrev.com/f7030ffe17ea03cf8a2e9189d7d764dfd07da26a
Cr-Commit-Position: refs/heads/master@{#357859}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Review feedback #
Messages
Total messages: 25 (10 generated)
jsbell@chromium.org changed reviewers: + yosin@google.com
yosin@ - please take a look? jshin@ - just FYI
https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/PageSerializer.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/PageSerializer.cpp:400: cssText.appendLiteral("@charset \""); We could skip this if the encoding is the default (windows-1252). https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/PageSerializer.cpp:418: // FIXME: We should check whether a charset has been specified and if none was found add one. Not sure if the FIXME here is referring to the lack of a charset in the styleSheet itself (it has one implicitly, so far as I can tell) or @charset (in which case, addressed by this CL) Remove?
Description was changed from ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding was not specified when serializing, so it would be treated as the default (windows-1252) on load. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ========== to ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding was not specified when serializing, so it would be treated as the default (windows-1252) on load. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ==========
jsbell@chromium.org changed reviewers: - yosin@google.com
https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/PageSerializer.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/PageSerializer.cpp:400: cssText.appendLiteral("@charset \""); On 2015/10/27 at 17:52:43, jsbell wrote: > We could skip this if the encoding is the default (windows-1252). Really? I think default charset depends of Chrome's language settings or charset from HTML document. Not sure. https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/PageSerializerTest.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/PageSerializerTest.cpp:343: EXPECT_TRUE(getSerializedData("link_styles.css", "text/css").startsWith("@charset \"windows-1252\";")); Does this work systems uses non-US locale? I'm not sure, whether test_runner uses "windows-1252" as default charset or not.
Thanks! Counter-questions... https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/PageSerializer.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/PageSerializer.cpp:400: cssText.appendLiteral("@charset \""); On 2015/10/28 00:14:57, Yosi_OOO_til_Oct_30 wrote: > On 2015/10/27 at 17:52:43, jsbell wrote: > > We could skip this if the encoding is the default (windows-1252). > > Really? I think default charset depends of Chrome's language > settings or charset from HTML document. Not sure. Oh, good point. Always outputting seems sensible then, especially as the context could change for serialized files. https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/PageSerializerTest.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/PageSerializerTest.cpp:343: EXPECT_TRUE(getSerializedData("link_styles.css", "text/css").startsWith("@charset \"windows-1252\";")); On 2015/10/28 00:14:57, Yosi_OOO_til_Oct_30 wrote: > Does this work systems uses non-US locale? I'm not sure, whether test_runner > uses "windows-1252" as > default charset or not. Pretty sure it does; most of the Web is defined as windows-1252 by default, regardless of system settings, but users can override. jshin@ - do you happen to know? Seemed to pass on the bots. I could reduce this down to asserting startsWith("@charset") - thoughts?
https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/PageSerializer.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/PageSerializer.cpp:418: // FIXME: We should check whether a charset has been specified and if none was found add one. On 2015/10/27 at 17:52:43, jsbell wrote: > Not sure if the FIXME here is referring to the lack of a charset in the styleSheet itself (it has one implicitly, so far as I can tell) or @charset (in which case, addressed by this CL) > > Remove? Agree. This patch always adds @charset. https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/PageSerializerTest.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/PageSerializerTest.cpp:343: EXPECT_TRUE(getSerializedData("link_styles.css", "text/css").startsWith("@charset \"windows-1252\";")); On 2015/10/28 at 18:16:55, jsbell wrote: > On 2015/10/28 00:14:57, Yosi_OOO_til_Oct_30 wrote: > > Does this work systems uses non-US locale? I'm not sure, whether test_runner > > uses "windows-1252" as > > default charset or not. > > Pretty sure it does; most of the Web is defined as windows-1252 by default, regardless of system settings, but users can override. > > jshin@ - do you happen to know? > > Seemed to pass on the bots. I could reduce this down to asserting startsWith("@charset") - thoughts? Since value of @charset for no @charset case isn't matter, I think startsWith("@charset") is enough.
Updated - please take a look? https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/PageSerializer.cpp (right): https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/PageSerializer.cpp:400: cssText.appendLiteral("@charset \""); On 2015/10/28 18:16:55, jsbell wrote: > On 2015/10/28 00:14:57, Yosi_OOO_til_Oct_30 wrote: > > On 2015/10/27 at 17:52:43, jsbell wrote: > > > We could skip this if the encoding is the default (windows-1252). > > > > Really? I think default charset depends of Chrome's language > > settings or charset from HTML document. Not sure. > > Oh, good point. Always outputting seems sensible then, especially as the context > could change for serialized files. Done. https://codereview.chromium.org/1424503006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/PageSerializer.cpp:418: // FIXME: We should check whether a charset has been specified and if none was found add one. On 2015/10/30 01:57:01, Yosi_OOO_til_Oct_30 wrote: > On 2015/10/27 at 17:52:43, jsbell wrote: > > Not sure if the FIXME here is referring to the lack of a charset in the > styleSheet itself (it has one implicitly, so far as I can tell) or @charset (in > which case, addressed by this CL) > > > > Remove? > > Agree. This patch always adds @charset. Done.
lgtm
On 2015/11/04 at 02:24:45, Yosi_OOO_til_Oct_30 wrote: > lgtm In description: "so it would be treated as the default (windows-1252) on load." We may not need to have this.
Description was changed from ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding was not specified when serializing, so it would be treated as the default (windows-1252) on load. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ========== to ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding was not specified when serializing, so it would be treated as the default on load. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ==========
Description was changed from ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding was not specified when serializing, so it would be treated as the default on load. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ========== to ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding was not specified when serializing. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ==========
Description was changed from ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding was not specified when serializing. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ========== to ========== Page Serializer: Specify encoding for serialized CSS files The input encoding (per a @charset attribute or Content-Type header) was used when serializing CSS files, but the encoding directive was not written back out. To fix, always write out the @charset directive per http://www.w3.org/International/questions/qa-css-charset BUG=548031 R=yosin@chromium.org TEST=webkit_unit_tests --gtest_filter='PageSerializerTest.CSS' ==========
The CQ bit was checked by jsbell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424503006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424503006/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jsbell@chromium.org changed reviewers: + pdr@chromium.org
Whoops, need owner. pdr@ - can you approve?
The CQ bit was checked by pdr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424503006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424503006/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f7030ffe17ea03cf8a2e9189d7d764dfd07da26a Cr-Commit-Position: refs/heads/master@{#357859} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
