Block "]]>" in createCDataSection

Block "]]>" in createCDataSection

It's not possible to serialize a CData section with ]]> in it
so disallow that already in xmldoc.createCDataSection.

This is compatible with MSIE and Gecko.

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

[Daniel Bratell, 2013-08-19 15:04:44]

leviw and dominicc, can you please review this 4 line patch for a minor
compatibility issue reported the other day.

[leviw_travelin_and_unemployed, 2013-08-19 17:22:47]

You'll definitely want to add a test for this. I'm not the best reviewer for
this. Adding a few others.

[abarth-chromium, 2013-08-19 17:30:14]

Yeah, this definitely needs a test.  Does the spec say to do this?

[Daniel Bratell, 2013-08-20 06:40:30]

On 2013/08/19 17:30:14, abarth wrote:
> Yeah, this definitely needs a test.  Does the spec say to do this?

There is no normative spec for createCDataSection worth calling "spec". The
closest thing is DOM2 and we all know how useless that is for implementors.

(http://www.w3.org/2003/01/dom2-javadoc/org/w3c/dom/Document.html#createCDATAS...
just says "Creates a CDATASection node whose value is the specified string")

So this is based on the argumentation of the reporter of bug 268762 that only
WebKit allows people to create CData with "]]>" and that is a source of breakage
when converting a tree back to a string. (I've verified that he's right)

I will add tests for this.

[abarth-chromium, 2013-08-20 07:16:21]

Ok.  I'm happy to approve this CL once you've added some tests.

[Daniel Bratell, 2013-08-20 09:24:45]

On 2013/08/20 07:16:21, abarth wrote:
> Ok.  I'm happy to approve this CL once you've added some tests.

Now with tests. I'm not sure if there is anything else I have to do to make it
picked up correctly by the google test bots. Also, I didn't test the "expected"
file..

[arv (Not doing code reviews), 2013-08-20 14:09:53]

https://codereview.chromium.org/22911020/diff/7001/LayoutTests/fast/dom/Docum...
File LayoutTests/fast/dom/Document/createCDATASection.html (right):

https://codereview.chromium.org/22911020/diff/7001/LayoutTests/fast/dom/Docum...
LayoutTests/fast/dom/Document/createCDATASection.html:16:
assert("createCDATASection" in xmlDoc, "createCDATASection exists")
shouldBe('"createCDATASection" in xmlDoc', 'true');

https://codereview.chromium.org/22911020/diff/7001/LayoutTests/fast/dom/Docum...
LayoutTests/fast/dom/Document/createCDATASection.html:26: for (var i = 0; i <
testData.length; i++) {
This test is overly complex.

shouldBeEqualToString('xmlDoc.createCDATASection("Hello").data', 'Hello');

shouldThrow('xmlDoc.createCDATASection("Ha]]>o")', "InvalidCharacterException:
String cannot contain ']]>'since that is the end delimiter of a CData
section.");

shouldBeEqualToString('xmlDoc.createCDATASection(null).data', 'null');
shouldBeEqualToString('xmlDoc.createCDATASection(new Date(1234567890)).data',
'Thu Jan 15 1970 01:56:07 GMT-0500 (EST)');
shouldBeEqualToString('xmlDoc.createCDATASection(42).data', '42');
shouldBeEqualToString('xmlDoc.createCDATASection('').data', '');

But you really do not need to check that conversion to string works, so only
shouldThrow is really needed.

https://codereview.chromium.org/22911020/diff/7001/LayoutTests/fast/dom/Docum...
LayoutTests/fast/dom/Document/createCDATASection.html:36: if ("code" in
testData[i])
Don't test the code. Test the whole string. See above

[Daniel Bratell, 2013-08-20 15:37:57]

Better now I think.

https://codereview.chromium.org/22911020/diff/7001/LayoutTests/fast/dom/Docum...
File LayoutTests/fast/dom/Document/createCDATASection.html (right):

https://codereview.chromium.org/22911020/diff/7001/LayoutTests/fast/dom/Docum...
LayoutTests/fast/dom/Document/createCDATASection.html:16:
assert("createCDATASection" in xmlDoc, "createCDATASection exists")
On 2013/08/20 14:09:54, arv wrote:
> shouldBe('"createCDATASection" in xmlDoc', 'true');

Thanks! I have still a lot to learn about all the available tools and utilities!

[arv (Not doing code reviews), 2013-08-20 17:47:06]

LGTM with nits

https://codereview.chromium.org/22911020/diff/12001/LayoutTests/fast/dom/Docu...
File LayoutTests/fast/dom/Document/createCDATASection.html (right):

https://codereview.chromium.org/22911020/diff/12001/LayoutTests/fast/dom/Docu...
LayoutTests/fast/dom/Document/createCDATASection.html:9: window.xmlDoc =
domParser.parseFromString("<example></example>", "text/xml");
xmlDoc is not a property of window. Use var instead:

var xmlDoc = ...

https://codereview.chromium.org/22911020/diff/12001/LayoutTests/fast/dom/Docu...
LayoutTests/fast/dom/Document/createCDATASection.html:9: window.xmlDoc =
domParser.parseFromString("<example></example>", "text/xml");
You do not need to create a DOMParser for this test.

var xmlDoc = document.implementation.createDocument(null, 'example', null);

https://codereview.chromium.org/22911020/diff/12001/LayoutTests/fast/dom/Docu...
LayoutTests/fast/dom/Document/createCDATASection.html:10:
shouldBe('"createCDATASection" in window.xmlDoc', 'true');
s/window.xmlDoc/xmlDoc/

[Daniel Bratell, 2013-08-20 20:58:35]

On 2013/08/20 17:47:06, arv wrote:
> LGTM with nits
> 
>
https://codereview.chromium.org/22911020/diff/12001/LayoutTests/fast/dom/Docu...
> File LayoutTests/fast/dom/Document/createCDATASection.html (right):
> 
>
https://codereview.chromium.org/22911020/diff/12001/LayoutTests/fast/dom/Docu...
> LayoutTests/fast/dom/Document/createCDATASection.html:9: window.xmlDoc =
> domParser.parseFromString("<example></example>", "text/xml");
> xmlDoc is not a property of window. Use var instead:
> 
> var xmlDoc = ...

var xmlDoc at top level will not create a local variable. It will still be a
variable at the global scope. Global scope for a browser is the window object.
So

var xmlDoc = "foo"

and

window.xmlDoc = "foo"

will do identical things (the lookup process in the script engine will be
slightly different but that is only interesting if you're really digging deep
and since I don't do much work at the Presto script engine anymore I might not
need to care about that very often).

So from there on it's a matter of taste. Since the test helper functions eval a
string I thought it would look bad to pass a reference to something that looked
like a local variable to them. Who knows, maybe some day someone moves the code
to a local function and then it will suddenly stop working. Better to have
explicit references that will work regardless of which scope the code is run or
evaled.

IMO.

[Daniel Bratell, 2013-08-22 08:41:39]

arv, can you LGTM the last patch? Or is the "LGTM with nits" on the patch before
that enough?

[arv (Not doing code reviews), 2013-08-22 13:26:50]

Still LGTM

[commit-bot: I haz the power, 2013-08-22 14:12:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/22911020/17001

[commit-bot: I haz the power, 2013-08-22 15:35:03]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Daniel Bratell, 2013-08-23 11:33:31]

On 2013/08/22 15:35:03, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

The "expected" file was missing a trailing linebreak. Fixed now.

arv, please, can you review that addition?

[arv (Not doing code reviews), 2013-08-23 14:51:45]

LGTM

[commit-bot: I haz the power, 2013-08-23 14:51:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/22911020/30001

[commit-bot: I haz the power, 2013-08-23 17:47:53]

Change committed as 156640