Add support for CSS unicod encoding to the text codec.

Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
jochen: owners review

Thanks!
--brendan

BUG=582573
Committed: https://crrev.com/f9038701fd566168ee7c66b1cabd3547a5517178
Cr-Commit-Position: refs/heads/master@{#390561}

[bburns, 2016-02-18 20:42:02]

bburns@chromium.org changed reviewers:
+ dimich@chromium.org, haraken@chromium.org, tkent@chromium.org

[bburns, 2016-02-18 20:42:03]

Please review.

See https://code.google.com/p/chromium/issues/detail?id=582573

and

https://www.w3.org/TR/css-syntax-3/#escaping

for context.

[bburns, 2016-02-18 20:42:58]

Description was changed from

==========
Add support for CSS unicod encoding to the text codec.

BUG=582573
==========

to

==========
Add support for CSS style unicode encoding to the text codec.

BUG=582573
==========

[bburns, 2016-02-18 20:45:49]

On 2016/02/18 20:42:03, bburns wrote:
> Please review.
> 
> See https://code.google.com/p/chromium/issues/detail?id=582573
> 
> and
> 
> https://www.w3.org/TR/css-syntax-3/#escaping
> 
> for context.

Please feel free to reassign as necessary, its hard to tell who really owns this
particular code.

Thanks!
--brendan

[bburns, 2016-02-18 20:53:47]

bburns@chromium.org changed reviewers:
- haraken@chromium.org

[bburns, 2016-02-18 20:54:13]

Description was changed from

==========
Add support for CSS style unicode encoding to the text codec.

BUG=582573
==========

to

==========
Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
tkent: owners review

Thanks!
--brendan

BUG=582573
==========

[Dmitry Titov, 2016-02-18 21:26:48]

Nice find! 

This should be easy to test (see existing mhtml tests in
src/third_party/WebKit/LayoutTests/mhtml), so lets add a test.

Also:

https://codereview.chromium.org/1709293002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/TextCodecICU.cpp (right):

https://codereview.chromium.org/1709293002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/TextCodecICU.cpp:417: static void
cssEscapedEntityCallback(const void* context, UConverterFromUnicodeArgs*
fromUArgs, const UChar* codeUnits, int32_t length,
These two new functions are not really trivial copy/paste from "urlEscaped*"
variants. It's better to pull the common code into a separate one and call twice
with a parameter.

[Dmitry Titov, 2016-04-20 02:23:52]

Ping?

[blink-reviews, 2016-04-20 04:24:33]

I thought we had left it that you were going to do some work to make
testing MHTML export easier.

I'm more familiar with blink now, so if you'd rather I worked on the test
infrastructure for MHTML export, I can do that.

On Tue, Apr 19, 2016, 7:23 PM <dimich@chromium.org> wrote:

> Ping?
>
>
> https://codereview.chromium.org/1709293002/
>

-- 
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.

[chromium-reviews, 2016-04-20 04:24:33]

I thought we had left it that you were going to do some work to make
testing MHTML export easier.

I'm more familiar with blink now, so if you'd rather I worked on the test
infrastructure for MHTML export, I can do that.

On Tue, Apr 19, 2016, 7:23 PM <dimich@chromium.org> wrote:

> Ping?
>
>
> https://codereview.chromium.org/1709293002/
>

-- 
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.

[Dmitry Titov, 2016-04-20 05:07:32]

My bad if that was the case. I'm not working on this, please feel free to add
the MHTML dump, it'd be great addition to the test infra!

[bburns, 2016-04-21 21:28:14]

bburns@chromium.org changed reviewers:
+ rdsmith@chromium.org

[bburns, 2016-04-21 21:28:14]

rdsmith: for the relevant test files.

Finally got around to adding a browser test.

Please take another look.

Thanks!
--brendan

[Randy Smith (Not in Mondays), 2016-04-25 16:58:07]

CCing Lukasz FHI (I'll do the test review, but I want to make sure he's aware of
the CL).

[Randy Smith (Not in Mondays), 2016-04-25 17:07:30]

lgtm

[bburns, 2016-04-26 23:28:58]

bburns@chromium.org changed reviewers:
+ lukasza@chromium.org
- tkent@chromium.org

[bburns, 2016-04-26 23:28:59]

+lukasza for owners

thanks
--brendan

[Łukasz Anforowicz, 2016-04-27 04:28:13]

My apologies for the delay, I haven't realized you are waiting for me :-(.

https://codereview.chromium.org/1709293002/diff/60001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (left):

https://codereview.chromium.org/1709293002/diff/60001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:738: }
I am happy with the test above, but could you please try to see how much trouble
it would be to also test via SavePageOriginalVsSavedComparisonTest?  This would
1) cover both MHTML and CompleteHTML modes + 2) do more of an end-to-end
verification (via find-in-page in original-vs-saved page) and 3) be a little bit
resilient to implementation details (i.e. verification would pass regardless of
whether ' vs " is used and/or e vs E).  To do this you could:
1. Add some content (span?) to b.html that is affected by the .icon:before that
you've added to 1.css
2. Add a search string to one of the test cases
   - I think we can just pick an arbitrary existing testcase that uses b.html -
i.e. the new search string can be added to |arr| in NestedFrames
   - For how to encode the string, you can see Encoding testcase (basically -
have to encode unicode characters as utf8, using \x escapes as we've found out
in https://codereview.chromium.org/1407663004#msg56).

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/TextCodec.cpp (right):

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/TextCodec.cpp:52: snprintf(replacement,
sizeof(UnencodableReplacementArray), "\\%x", codePoint);
I don't understand how we handle "A 'real' space after the escape sequence must
be doubled." mentioned by the spec you linked to (thanks for the link) at
https://www.w3.org/TR/css-syntax-3/#escaping

Would it be reasonable to always include a space here (i.e. use "\\%x " format
string)?

Also - it might be also worthwhile to tweak 1.css to test this space-handling
scenario (i.e. content: "\e003 \e003  \e003x"; ?) and verify via
SavePageOriginalVsSavedComparisonTest (as I suggest in another comment).

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/TextCodec.h (right):

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/TextCodec.h:57:
CSSEncodedEntitiesForUnencodables
nit: I read that C++11 allows a trailing comma in enum definitions.  Maybe we
could add that here to minimize future changes (and streamline git blame) if we
ever need to add more enum values.

[bburns, 2016-04-27 20:24:21]

comments addressed, please re-check.

thanks
--brendan

https://codereview.chromium.org/1709293002/diff/60001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (left):

https://codereview.chromium.org/1709293002/diff/60001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:738: }
On 2016/04/27 04:28:12, Łukasz Anforowicz wrote:
> I am happy with the test above, but could you please try to see how much
trouble
> it would be to also test via SavePageOriginalVsSavedComparisonTest?  This
would
> 1) cover both MHTML and CompleteHTML modes + 2) do more of an end-to-end
> verification (via find-in-page in original-vs-saved page) and 3) be a little
bit
> resilient to implementation details (i.e. verification would pass regardless
of
> whether ' vs " is used and/or e vs E).  To do this you could:
> 1. Add some content (span?) to b.html that is affected by the .icon:before
that
> you've added to 1.css
> 2. Add a search string to one of the test cases
>    - I think we can just pick an arbitrary existing testcase that uses b.html
-
> i.e. the new search string can be added to |arr| in NestedFrames
>    - For how to encode the string, you can see Encoding testcase (basically -
> have to encode unicode characters as utf8, using \x escapes as we've found out
> in https://codereview.chromium.org/1407663004#msg56).

I tried adding this test, but it passes both with and without my change.  I
think this is due to the way that the serialization works for the complete test.

I appreciate the desire to have more end to end coverage, but at this point, I
don't think the effort is worthwhile.

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/TextCodec.cpp (right):

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/TextCodec.cpp:52: snprintf(replacement,
sizeof(UnencodableReplacementArray), "\\%x", codePoint);
On 2016/04/27 04:28:12, Łukasz Anforowicz wrote:
> I don't understand how we handle "A 'real' space after the escape sequence
must
> be doubled." mentioned by the spec you linked to (thanks for the link) at
> https://www.w3.org/TR/css-syntax-3/#escaping
> 
> Would it be reasonable to always include a space here (i.e. use "\\%x " format
> string)?
> 
> Also - it might be also worthwhile to tweak 1.css to test this space-handling
> scenario (i.e. content: "\e003 \e003  \e003x"; ?) and verify via
> SavePageOriginalVsSavedComparisonTest (as I suggest in another comment).

Added the space as suggested and augmented the test as well.

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/TextCodec.h (right):

https://codereview.chromium.org/1709293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/TextCodec.h:57:
CSSEncodedEntitiesForUnencodables
On 2016/04/27 04:28:12, Łukasz Anforowicz wrote:
> nit: I read that C++11 allows a trailing comma in enum definitions.  Maybe we
> could add that here to minimize future changes (and streamline git blame) if
we
> ever need to add more enum values.

Done.

[Łukasz Anforowicz, 2016-04-27 20:25:47]

lgtm

https://codereview.chromium.org/1709293002/diff/60001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (left):

https://codereview.chromium.org/1709293002/diff/60001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:738: }
On 2016/04/27 20:24:21, bburns wrote:
> On 2016/04/27 04:28:12, Łukasz Anforowicz wrote:
> > I am happy with the test above, but could you please try to see how much
> trouble
> > it would be to also test via SavePageOriginalVsSavedComparisonTest?  This
> would
> > 1) cover both MHTML and CompleteHTML modes + 2) do more of an end-to-end
> > verification (via find-in-page in original-vs-saved page) and 3) be a little
> bit
> > resilient to implementation details (i.e. verification would pass regardless
> of
> > whether ' vs " is used and/or e vs E).  To do this you could:
> > 1. Add some content (span?) to b.html that is affected by the .icon:before
> that
> > you've added to 1.css
> > 2. Add a search string to one of the test cases
> >    - I think we can just pick an arbitrary existing testcase that uses
b.html
> -
> > i.e. the new search string can be added to |arr| in NestedFrames
> >    - For how to encode the string, you can see Encoding testcase (basically
-
> > have to encode unicode characters as utf8, using \x escapes as we've found
out
> > in https://codereview.chromium.org/1407663004#msg56).
> 
> I tried adding this test, but it passes both with and without my change.  I
> think this is due to the way that the serialization works for the complete
test.
> 
> I appreciate the desire to have more end to end coverage, but at this point, I
> don't think the effort is worthwhile.

Acknowledged.

[bburns, 2016-04-27 21:23:04]

Description was changed from

==========
Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
tkent: owners review

Thanks!
--brendan

BUG=582573
==========

to

==========
Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
jochen: owners review

Thanks!
--brendan

BUG=582573
==========

[bburns, 2016-04-27 21:23:04]

bburns@chromium.org changed reviewers:
+ jochen@chromium.org
- lukasza@chromium.org, rdsmith@chromium.org

[bburns, 2016-04-27 22:51:28]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-04-27 22:51:28]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1709293002/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2016-04-27 22:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709293002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709293002/80001

[commit-bot: I haz the power, 2016-04-27 23:00:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 23:00:34]

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...)

[jochen (gone - plz use gerrit), 2016-04-28 14:48:29]

jochen@chromium.org changed reviewers:
+ lukasza@chromium.org, rdsmith@chromium.org

[jochen (gone - plz use gerrit), 2016-04-28 14:48:30]

can you add a unit test please?

[bburns, 2016-04-28 16:27:49]

On 2016/04/28 14:48:30, jochen wrote:
> can you add a unit test please?

There is a unit test in save_page_browsertest.cc or is there a different unit
test you had in mind?

Thanks!
--brendan

[jochen (gone - plz use gerrit), 2016-04-28 16:31:52]

browser tests are integration tests and can fail for many different reasons.

I'd prefer to have a TextCodecTest.cpp that just tests this case. I notice that
we don't have that many tests there yet...

[bburns, 2016-04-28 16:33:29]

On 2016/04/28 16:31:52, jochen wrote:
> browser tests are integration tests and can fail for many different reasons.
> 
> I'd prefer to have a TextCodecTest.cpp that just tests this case. I notice
that
> we don't have that many tests there yet...

ok sure, I will add that.

--brendan

[bburns, 2016-04-28 17:11:11]

On 2016/04/28 16:33:29, bburns wrote:
> On 2016/04/28 16:31:52, jochen wrote:
> > browser tests are integration tests and can fail for many different reasons.
> > 
> > I'd prefer to have a TextCodecTest.cpp that just tests this case. I notice
> that
> > we don't have that many tests there yet...
> 
> ok sure, I will add that.
> 
> --brendan

ok, added unittests for all of the different encodings in TextCodec.cpp.  I had
to add a new test file since all of that code was previously untested.

ptal.
thanks
--brendan

[jochen (gone - plz use gerrit), 2016-04-28 17:16:37]

Thank you, lgtm

[bburns, 2016-04-28 18:55:26]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-04-28 18:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, rdsmith@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1709293002/#ps120001
(title: "add a unit test")

[commit-bot: I haz the power, 2016-04-28 18:55:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709293002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709293002/120001

[commit-bot: I haz the power, 2016-04-28 19:11:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 19:11:33]

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...)

[bburns, 2016-04-28 19:31:49]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-04-28 19:31:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, jochen@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1709293002/#ps140001
(title: "add a test")

[commit-bot: I haz the power, 2016-04-28 19:32:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709293002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709293002/140001

[commit-bot: I haz the power, 2016-04-28 19:49:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 19:49:39]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dewittj, 2016-04-28 22:41:46]

dewittj@chromium.org changed reviewers:
+ dewittj@chromium.org

[dewittj, 2016-04-28 22:41:47]

https://codereview.chromium.org/1709293002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/wtf.gypi (right):

https://codereview.chromium.org/1709293002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/wtf.gypi:216: 'TreeNodeTest.cpp',
do you need to add TextCodecTest.cpp here?

[dewittj, 2016-04-28 22:43:14]

https://codereview.chromium.org/1709293002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/wtf.gypi (right):

https://codereview.chromium.org/1709293002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/wtf.gypi:216: 'TreeNodeTest.cpp',
On 2016/04/28 22:41:47, dewittj wrote:
> do you need to add TextCodecTest.cpp here?

Whoops was looking at wrong version. Sorry!

[bburns, 2016-04-28 23:18:05]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-04-28 23:18:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, jochen@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1709293002/#ps180001
(title: "add a unit test")

[commit-bot: I haz the power, 2016-04-28 23:18:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709293002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709293002/180001

[commit-bot: I haz the power, 2016-04-29 01:37:51]

Description was changed from

==========
Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
jochen: owners review

Thanks!
--brendan

BUG=582573
==========

to

==========
Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
jochen: owners review

Thanks!
--brendan

BUG=582573
==========

[commit-bot: I haz the power, 2016-04-29 01:37:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-30 17:23:25]

Patchset 10 (id:??) landed as
https://crrev.com/f9038701fd566168ee7c66b1cabd3547a5517178
Cr-Commit-Position: refs/heads/master@{#390561}

[commit-bot: I haz the power, 2016-04-30 17:58:51]

Description was changed from

==========
Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
jochen: owners review

Thanks!
--brendan

BUG=582573
==========

to

==========
Add support for CSS style unicode encoding to the text codec.

dimich: encoding review
jochen: owners review

Thanks!
--brendan

BUG=582573

Committed: https://crrev.com/f9038701fd566168ee7c66b1cabd3547a5517178
Cr-Commit-Position: refs/heads/master@{#390561}
==========