XMLSerializer escapes < > & correctly inside <script> and <style> tags.

XMLSerializer escapes < > & correctly inside <script> and <style> tags.

BUG=263754
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161476

[pwnall-personal, 2013-10-08 05:02:03]

The associated bug has a good description of the behavior that's getting change.

Regarding the changes in the existing test case:

1) The .html file was not consistent about using CDATA. I made it consistent.
The JS in it used innerHTML, which (I believe) required CDATA sections around
all the CSS.

2) The expectations file had & but the corresponding lines of the .html file had
& so I think my changes are reasonable.

[pwnall-personal, 2013-10-08 05:03:05]

On 2013/10/08 05:02:03, pwnall wrote:
> The associated bug has a good description of the behavior that's getting
change.
> 
> Regarding the changes in the existing test case:
> 
> 1) The .html file was not consistent about using CDATA. I made it consistent.
> The JS in it used innerHTML, which (I believe) required CDATA sections around
> all the CSS.
> 
> 2) The expectations file had & but the corresponding lines of the .html file
had
> & so I think my changes are reasonable.

I hit "Send" too soon. Sorry for the grammar mistakes and unintended rudeness.

Can you please take a look?

[abarth-chromium, 2013-10-08 05:26:38]

Hum...  This seems reasonable, but I need to study it a bit more.  How do other
browsers behave?

[pwnall-personal, 2013-10-08 07:57:22]

On 2013/10/08 05:26:38, abarth wrote:
> Hum...  This seems reasonable, but I need to study it a bit more.  How do
other
> browsers behave?

Firefox and IE 10 both behave according to the test result in this patch.

Safari in Mavericks has the same behavior that current Blink does. I'd be happy
to submit this patch to WebKit as well though, if it seems OK to you.

Also, the trybot failure is due to the fact that I don't have try server access
yet. Please ignore :)

[abarth-chromium, 2013-10-08 08:00:14]

Ok, that's good.  I'll review it for real in the morning when I'm not so tired. 
Thanks for the CL!

[pwnall-personal, 2013-10-08 08:02:38]

On 2013/10/08 08:00:14, abarth wrote:
> Ok, that's good.  I'll review it for real in the morning when I'm not so
tired. 
> Thanks for the CL!

Thank you for the incredibly fast response!

[pwnall-personal, 2013-10-10 08:09:40]

On 2013/10/08 08:02:38, pwnall wrote:
> On 2013/10/08 08:00:14, abarth wrote:
> > Ok, that's good.  I'll review it for real in the morning when I'm not so
> tired. 
> > Thanks for the CL!
> 
> Thank you for the incredibly fast response!

abarth: ping?

[pwnall-personal, 2013-10-15 04:31:40]

On 2013/10/10 08:09:40, pwnall wrote:
> On 2013/10/08 08:02:38, pwnall wrote:
> > On 2013/10/08 08:00:14, abarth wrote:
> > > Ok, that's good.  I'll review it for real in the morning when I'm not so
> > tired. 
> > > Thanks for the CL!
> > 
> > Thank you for the incredibly fast response!
> 
> abarth: ping?

abarth: sorry to bother you again about this. Is there anything I can do to make
your life easier here?

[pwnall-personal, 2013-10-24 15:05:57]

abarth: Sorry for bringing this up again. Is it a bad idea? If so, I can drop
the CL.

If I don't hear from you in a while, I'll assume you're really busy, and I'll
try to find someone else to review this.

On 2013/10/15 04:31:40, pwnall wrote:
> On 2013/10/10 08:09:40, pwnall wrote:
> > On 2013/10/08 08:02:38, pwnall wrote:
> > > On 2013/10/08 08:00:14, abarth wrote:
> > > > Ok, that's good.  I'll review it for real in the morning when I'm not so
> > > tired. 
> > > > Thanks for the CL!
> > > 
> > > Thank you for the incredibly fast response!
> > 
> > abarth: ping?
> 
> abarth: sorry to bother you again about this. Is there anything I can do to
make
> your life easier here?

[pwnall-personal, 2013-11-05 11:14:05]

Jochen, can you please take a look at this?

[jochen (gone - plz use gerrit), 2013-11-06 16:17:48]

looks like the desired behavior according to http://domparsing.spec.whatwg.org/

https://codereview.chromium.org/26193002/diff/5001/LayoutTests/fast/dom/XMLSe...
File LayoutTests/fast/dom/XMLSerializer-entities.html (right):

https://codereview.chromium.org/26193002/diff/5001/LayoutTests/fast/dom/XMLSe...
LayoutTests/fast/dom/XMLSerializer-entities.html:1: <html>
nit. <!DOCTYPE html>

https://codereview.chromium.org/26193002/diff/5001/Source/core/editing/Markup...
File Source/core/editing/MarkupAccumulator.cpp (right):

https://codereview.chromium.org/26193002/diff/5001/Source/core/editing/Markup...
Source/core/editing/MarkupAccumulator.cpp:313: return
text->document().isHTMLDocument() ? EntityMaskInHTMLPCDATA : EntityMaskInPCDATA;
can you simplilfy this method a bit?

if (!text->document().isHTMLDocument())
  return EntitytMaskInPCDATA;

// rest of method here with ?:

[pwnall-personal, 2013-11-06 17:34:30]

Thank you very much for the feedback! Can you please take another look?

https://codereview.chromium.org/26193002/diff/5001/Source/core/editing/Markup...
File Source/core/editing/MarkupAccumulator.cpp (right):

https://codereview.chromium.org/26193002/diff/5001/Source/core/editing/Markup...
Source/core/editing/MarkupAccumulator.cpp:313: return
text->document().isHTMLDocument() ? EntityMaskInHTMLPCDATA : EntityMaskInPCDATA;
On 2013/11/06 16:17:49, jochen wrote:
> can you simplilfy this method a bit?
> 
> if (!text->document().isHTMLDocument())
>   return EntitytMaskInPCDATA;
> 
> // rest of method here with ?:

Done.

Did you mean I should use a ? : in a return clause?

I thought the condition was rather long, so I removed the ? : from the two if
branches and turned them into simple returns. If you think I should use ? :, I
can make that change right away.

[jochen (gone - plz use gerrit), 2013-11-06 17:50:25]

sorry I meant "without ?:"

lgtm

[pwnall-personal, 2013-11-06 18:11:12]

On 2013/11/06 17:50:25, jochen wrote:
> sorry I meant "without ?:"
> 
> lgtm

Thank you very much for the quick review, Jochen!

[commit-bot: I haz the power, 2013-11-06 20:40:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/costan@gmail.com/26193002/94001

[commit-bot: I haz the power, 2013-11-06 22:27:23]

Change committed as 161476