Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
534583002:
Implemented XMLSerializer in PrivateScript. (Closed)
|
Created:
6 years, 3 months ago by tasak Modified:
5 years, 11 months ago CC:
arv+blink, blink-reviews, Inactive Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionImplemented XMLSerializer in PrivateScript.
Since Document.xmlVersion and Document.xmlStandalone will be removed, C++ XMLSerializer::serializeToString creates XML declaration and provides the string for JS serializeToString.
Currently _SERIALIZATION_TYPE_AS_OWNER_DOCUMENT is not used, but it is important for XSLT in PrivateScript.
BUG=
Patch Set 1 : Need to fix fast/dom/cross-frame-accessor-throw.html #Patch Set 2 : Removed isHTMLDocument #
Total comments: 28
Patch Set 3 : Removed backdoors added to Document. #
Total comments: 16
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #Patch Set 6 : #
Total comments: 35
Patch Set 7 : #
Total comments: 1
Messages
Total messages: 29 (9 generated)
tasak@google.com changed reviewers: + yosin@chromium.org
On 2014/09/02 12:47:26, tasak wrote: > mailto:tasak@google.com changed reviewers: > + mailto:yosin@chromium.org WIP. Need to fix fast/xsl/xslt-processor.html's regression.
https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/Document... File Source/core/xml/DocumentXMLSerializer.idl (right): https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/Document... Source/core/xml/DocumentXMLSerializer.idl:6: [OnlyExposedToPrivateScript] readonly attribute boolean hasXMLDeclaration; Can we pass them as arguments for JS XML serializer? This means we still keep C++ XMLSerializer::serializeToString() and it calls BlinkInJS. String XMLSerializer::serializeToString(...) { String result; V8XMLSerializer::serializeToStringInJavaScript(..., hasXMLDeclaration, isXMLStandaloneSpecified); return result; } https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:5: "use strict"; Let's use one kind of quote. I suggest to use single-quote as of Google JS coding style: https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml Note: JS in Chromium follow this: http://dev.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:24: if (!uri) nit: return uri === '' ? '' : uri.replace(/\/$/g, ''); https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:29: function cloneNamespace(obj) { This function make clone of object, not sure why we call |cloneNamespace|. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:30: var clone = {}; nit: return Object.create(obj); https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:37: if (isHTMLElement(node) && node.tagName.toLowerCase() == 'template') nit: Just use |return| statement. nit: We don't need to use |toLowerCase()|, just compare to 'TEMPLATE'. nit: You can use 'node.nodeName', if so, you don't need to call isHTMLElement(). https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:45: var candidate = 'ns' + Date.now(); We should have closed variable, rather than using current time as unique counter. var uniquePrefixCounter = 0; function generatePrefix(namespaces) { ... ++uniquePrefixCounter; var candidate = 'ns' + uniquePrefixCounter; ... } https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:53: var tagName = node.tagName.toLowerCase(); not obvious what you want to accomplish. Can we use node.namespaceURI for checking qualified name? https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:61: if (namespaceURI == _XML_NAMESPACE_URI || We should have |Set| which member is well know namespace URI, rather than serialize of equality checks. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:79: if (prefix) { nit: No need to have "{}" https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:115: while (node) { nit: it is better to use for statement for (var runner = node; node; node = node.parentNode) { ... } https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:126: function elementCannotHaveEndTag(node) { nit: HTML5 use term "Tag ommission in text/html" http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#th... So, better name is isEndTagOmissible(element) https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:132: var tagName = node.tagName.toLowerCase(); nit: To avoid string object construction, we should use upper case letters for key. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:138: XMLSerializerPrototype.serializeAsHTMLDocument = function(node) { I suggest to use Object.defineProperties(XMLSerializePrototype, { serializeAsHTMLDocument: {value: serializeAsHTMLDocument}, ... }); In this way, you can see function name in stack trace.
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Thank you for reviewing. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:5: "use strict"; On 2014/09/03 03:35:02, Yosi_UTC9 wrote: > Let's use one kind of quote. > I suggest to use single-quote as of Google JS coding style: > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml > > Note: JS in Chromium follow this: > http://dev.chromium.org/developers/web-development-style-guide Done. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:24: if (!uri) On 2014/09/03 03:35:02, Yosi_UTC9 wrote: > nit: return uri === '' ? '' : uri.replace(/\/$/g, ''); We need to consier the case that uri is null. So this should be "return uri ? uri.replace(...) : '';". https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:29: function cloneNamespace(obj) { On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > This function make clone of object, not sure why we call |cloneNamespace|. When we visit child nodes, one of the child nodes might update namespaces. However, other child nodes should not be affected by the update. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:30: var clone = {}; On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > nit: return Object.create(obj); I think, this doesn't work. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:37: if (isHTMLElement(node) && node.tagName.toLowerCase() == 'template') On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > nit: We don't need to use |toLowerCase()|, just compare to 'TEMPLATE'. I don't think so. If we always compare with 'TEMPLATE', LayoutTests/fast/dom/HTMLTemplateElement/xhtml-parsing-and-serialization.xml fails. > nit: You can use 'node.nodeName', if so, you don't need to call isHTMLElement(). xml document also has an element whose tagname is 'template' and whose namespace uri is not html. In this case, we should not treat the element as HTMLTemplateElement. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:45: var candidate = 'ns' + Date.now(); On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > We should have closed variable, rather than using current time as unique > counter. > > var uniquePrefixCounter = 0; > function generatePrefix(namespaces) { > ... > ++uniquePrefixCounter; > var candidate = 'ns' + uniquePrefixCounter; > ... > } I tried another approach. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:53: var tagName = node.tagName.toLowerCase(); On 2014/09/03 03:35:02, Yosi_UTC9 wrote: > not obvious what you want to accomplish. > Can we use node.namespaceURI for checking qualified name? I updated. If a given node is HTMLElement, we should use node.tagName.toLowerCase(). Otherwise, we should just return node.tagName. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:61: if (namespaceURI == _XML_NAMESPACE_URI || On 2014/09/03 03:35:02, Yosi_UTC9 wrote: > We should have |Set| which member is well know namespace URI, rather than > serialize of equality checks. Done. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:79: if (prefix) { On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > nit: No need to have "{}" Done. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:115: while (node) { On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > nit: it is better to use for statement > > for (var runner = node; node; node = node.parentNode) { > ... > } Done. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:126: function elementCannotHaveEndTag(node) { On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > nit: HTML5 use term "Tag ommission in text/html" > http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#th... > > So, better name is isEndTagOmissible(element) Done. https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:132: var tagName = node.tagName.toLowerCase(); On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > nit: To avoid string object construction, we should use upper case letters for > key. I found layout test failure if using upper case letters for key and removing tagName.toLowerCase(). https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:138: XMLSerializerPrototype.serializeAsHTMLDocument = function(node) { On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > I suggest to use > Object.defineProperties(XMLSerializePrototype, { > serializeAsHTMLDocument: {value: serializeAsHTMLDocument}, > ... > }); > > In this way, you can see function name in stack trace. I'm not sure whether we allow users to see internal function names in stack trace or not.
tasak@google.com changed reviewers: + haraken@chromium.org, jl@opera.com
+jl@opera.com and haraken@chromium.org Would you review this CL?
This is a rather shallow review. Also note that I'm not a core/ OWNER. What's the ambition level of this serializer in terms of, say, guarantee to produce well-formed XML? Is it a conversion to JS of our existing C++ serializer, more of a newly implemented one? https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.cpp (right): https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.cpp:43: builder.appendLiteral("<?xml version=\""); IMO it seems a bit odd to serialize an XML declaration here, rather than just passing the data in to the private script function and let it do all the serializing. Exposing the information (publicly) on the Document interface would have made sense too (back when the DOM APIs were first designed, perhaps,) but at this point adding XML stuff to it is probably not the direction we want it to move. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.idl (right): https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.idl:26: [RaisesException] DOMString serializeToString([Default=Undefined] optional Node node); Unrelated to this CL: We could add [TypeChecking=Interface] to verify the 'node' argument is valid instead of checking in C++. (And maybe drop 'optional' and '[Default=Undefined]', since the argument doesn't really appear to be very optional.) https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.idl:28: [OnlyExposedToPrivateScript, ImplementedInPrivateScript] DOMString serializeToStringInternal(Node node, [Default=Undefined] optional DOMString xmlDeclaration); Make this "xmlDeclaration = ''" instead of [Default=Undefined]? (I doubt the string "undefined" will work very well as an XML declaration.) https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:29: } I think \u0022 can be written as ". What's the reason not to? https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:91: result += '=\"' + namespaceURI + '"'; No need to escape " in either string here. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:220: return a.name <= b.name ? -1 : 0; This is an unusual comparison function: If a < b, say "a is less than b" If a == b, say "a is less than b" If a > b, say "they are equal" AFAICT, the result in V8 is that it leaves the array in the order it was to begin with. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:282: if (node.systemId) There can also be just a system id, in which case the output should be <!DOCTYPE name SYSTEM "url">. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:364: namespaces['xml'] = _XML_NAMESPACE_URI; Could add 'xmlns' as well, I guess. It too is implicitly declared (and IIRC illegal to declare explicitly.)
Thank you for reviewing. >Is it a conversion to JS of our existing C++ serializer, more of a newly implemented one? Yes. This is a conversion to JS of our existing C++ serializer. Since I would like to modify XMLSerializer for XSLT in PrivateScript, I'm trying to convert XMLSerializer. I need serializeToStringAsOwnerDocument. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.cpp (right): https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.cpp:43: builder.appendLiteral("<?xml version=\""); On 2014/09/03 10:29:33, Jens Widell wrote: > IMO it seems a bit odd to serialize an XML declaration here, rather than just > passing the data in to the private script function and let it do all the > serializing. > > Exposing the information (publicly) on the Document interface would have made > sense too (back when the DOM APIs were first designed, perhaps,) but at this > point adding XML stuff to it is probably not the direction we want it to move. Firstly I tried to serialize an XML declaration in XMLSerializer.js. However, I found that there is no way to find whether a given document has an XML declaration or not. I tried "if (document.xmlVersion) { ... }", but document.xmlVersion is always "1.0". Since the API has been already shipped, I'm not sure whether we can change the behavior or not. And looking at Document.idl, it says "xmlVersion, xmlStandalone and xmlEncoding are removed from DOM4". I think, we cannot create an XML declaration by using only web-exposed API in the future. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.idl (right): https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.idl:26: [RaisesException] DOMString serializeToString([Default=Undefined] optional Node node); On 2014/09/03 10:29:33, Jens Widell wrote: > Unrelated to this CL: We could add [TypeChecking=Interface] to verify the 'node' > argument is valid instead of checking in C++. (And maybe drop 'optional' and > '[Default=Undefined]', since the argument doesn't really appear to be very > optional.) Done. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.idl:28: [OnlyExposedToPrivateScript, ImplementedInPrivateScript] DOMString serializeToStringInternal(Node node, [Default=Undefined] optional DOMString xmlDeclaration); On 2014/09/03 10:29:33, Jens Widell wrote: > Make this "xmlDeclaration = ''" instead of [Default=Undefined]? (I doubt the > string "undefined" will work very well as an XML declaration.) Done. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:29: } On 2014/09/03 10:29:33, Jens Widell wrote: > I think \u0022 can be written as ". What's the reason not to? Done. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:91: result += '=\"' + namespaceURI + '"'; On 2014/09/03 10:29:33, Jens Widell wrote: > No need to escape " in either string here. Done. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:220: return a.name <= b.name ? -1 : 0; On 2014/09/03 10:29:33, Jens Widell wrote: > This is an unusual comparison function: > > If a < b, say "a is less than b" > If a == b, say "a is less than b" > If a > b, say "they are equal" > > AFAICT, the result in V8 is that it leaves the array in the order it was to > begin with. I see. I found that we don't need to sort attributes. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:282: if (node.systemId) On 2014/09/03 10:29:33, Jens Widell wrote: > There can also be just a system id, in which case the output should be <!DOCTYPE > name SYSTEM "url">. Done. https://codereview.chromium.org/534583002/diff/100001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:364: namespaces['xml'] = _XML_NAMESPACE_URI; On 2014/09/03 10:29:34, Jens Widell wrote: > Could add 'xmlns' as well, I guess. It too is implicitly declared (and IIRC > illegal to declare explicitly.) Done.
haraken@chromium.org changed reviewers: + abarth@chromium.org
tasak@ has thoughts about backdoors. I think we should first discuss the backdoors with abarth@ before looking into the detailed implementation.
+abarth@chromium.org. abarth, would you review this CL, mainly, XMLSerializer.idl? To serialize a document, we need (a) or (b): (a) add a backdoor to document to obtain XMLDeclaration. In this case, serializeToString is implemented in PrivateScript. (b) add serializeToStringInternal (implemented in PrivateScript) to XMLSerializer. serializeToString creates XMLDeclaration and provides the string for serializeToStringInternal. I implemented (b) in the newest patch. However, I'm not sure which is better solution.
https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/20001/Source/core/xml/XMLSeria... Source/core/xml/XMLSerializer.js:138: XMLSerializerPrototype.serializeAsHTMLDocument = function(node) { On 2014/09/03 09:38:00, tasak wrote: > On 2014/09/03 03:35:01, Yosi_UTC9 wrote: > > I suggest to use > > Object.defineProperties(XMLSerializePrototype, { > > serializeAsHTMLDocument: {value: serializeAsHTMLDocument}, > > ... > > }); > > > > In this way, you can see function name in stack trace. > > I'm not sure whether we allow users to see internal function names in stack > trace or not. When you use this JS file in standalone environment, e.g. debugging JS w/o Blink-In-JS, we can see function names in inspector. As adamb@ said, it is better that JS code for Blink-In-JS works in other browsers, including Chrome w/o Blink-In-JS.
yosin@chromium.org changed reviewers: + arv@chromium.org
+arv@ for JS style review. https://codereview.chromium.org/534583002/diff/120001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.idl (right): https://codereview.chromium.org/534583002/diff/120001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.idl:28: [OnlyExposedToPrivateScript, ImplementedInPrivateScript] DOMString serializeToStringInternal(Node node, optional DOMString xmlDeclaration = ""); Q: Do we need to make |xmlDeclaration| parameter as optional? C++ code always pass this parameter. I think it is better to check empty string, null and |undefined| in JS. I guess major call sites of this function is in JS. If so, handling of |xmlDeclaration| as optional parameter in JS makes writing JS call sites easier. In the future, we can use ES6 optional parameter. https://codereview.chromium.org/534583002/diff/120001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/120001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:299: } else if (node.nodeType == Node.COMMENT_NODE) { nit: We don't need to have |else| after |return|. nit: Is it better to use |switch|? it seems this function supports most(all?) of node type.
On 2014/09/03 11:46:37, tasak wrote: > mailto:+abarth@chromium.org. > > abarth, would you review this CL, mainly, XMLSerializer.idl? > > To serialize a document, we need (a) or (b): > > (a) add a backdoor to document to obtain XMLDeclaration. In this case, > serializeToString is implemented in PrivateScript. > > (b) add serializeToStringInternal (implemented in PrivateScript) to > XMLSerializer. serializeToString creates XMLDeclaration and provides the string > for serializeToStringInternal. > > I implemented (b) in the newest patch. However, I'm not sure which is better > solution. abarth, would you see my backdoor idea?
https://codereview.chromium.org/534583002/diff/120001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/120001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:299: } else if (node.nodeType == Node.COMMENT_NODE) { On 2014/09/04 01:10:42, Yosi_UTC9 wrote: > nit: We don't need to have |else| after |return|. > nit: Is it better to use |switch|? it seems this function supports most(all?) of > node type. Done.
On 2014/09/03 11:46:37, tasak wrote: > mailto:+abarth@chromium.org. > > abarth, would you review this CL, mainly, XMLSerializer.idl? > > To serialize a document, we need (a) or (b): > > (a) add a backdoor to document to obtain XMLDeclaration. In this case, > serializeToString is implemented in PrivateScript. > > (b) add serializeToStringInternal (implemented in PrivateScript) to > XMLSerializer. serializeToString creates XMLDeclaration and provides the string > for serializeToStringInternal. > > I implemented (b) in the newest patch. However, I'm not sure which is better > solution. ping.
abarth@chromium.org changed reviewers: - abarth@chromium.org
I'm sorry, but I don't have the bandwidth to review this CL. Looks like you've got a bunch of people listed as reviewers already.
On 2014/10/07 01:15:02, abarth wrote: > I'm sorry, but I don't have the bandwidth to review this CL. Looks like you've > got a bunch of people listed as reviewers already. We just want to ask you to review the private API in XMLSerealizer.idl. (https://codereview.chromium.org/516273002/ is in the same situation.) (Personally I'm not that strict about introducing private APIs. Private APIs are just used internally, so it's easy to update the APIs and iterate. Unless we don't introduce too many private APIs (which makes it harder to let the third party develop the feature in the future), I guess it won't cause a big deal.)
This is great. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:9: var _EMPTY = ''; No leading underscores for variable names. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:32: return text.replace(/&/g, '&').replace(/"/g, '"').replace(/\u00A0/g, ' '); It is kind of sad that we have to create all these extra strings. Maybe do: .replace(/&|"|\u00a0/g, function(c) { switch (c) { case '&': return '&'; ... https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:40: var clone = {}; Object.create(null) or {__proto__: null} or your map will break. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:47: return isHTMLElement(node) && node.tagName.toLowerCase() == 'template'; node.localName === 'template' or node.tagName === 'TEMPLATE' no need to do extra work. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:51: var nsRegex = new RegExp('^ns([0-9]+)$', 'g'); /^ns([0-9]+)$/g https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:56: var value = parseInt(match[1]); parseInt(x, 10) or you might get octal you can also use Number() https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:66: return node.tagName.toLowerCase(); localName should be lowercase already https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:71: var _SERIALIZED_NAMESPACES = { no leading underscores https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:72: _XML_NAMESPACE_URI:true, _XLINK_NAMESPACE_URI:true, bad formatting https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:135: var _END_TAG_OMISSIBLE_TAGS = { move out of function? https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:136: 'area':true, 'base':true, 'basefont':true, 'br':true, 'col':true, 'embed':true, bad formatting https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:147: } missing ; https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:214: var attr = node.attributes.item(i); node.attributes[i] https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:215: if (attr.name.indexOf('_moz') == 0) what? Is this part of the spec? https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:241: var sink = this.serializeOpenTag_(node, namespaces); return this.serializeOpenTag_(node, namespaces) + this.serializeAttributes_(node, namespaces) + this.serializeCloseTag_(node, namespaces); https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:264: return '<!--' + node.nodeValue + '-->'; escape nodeValue? https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:357: var namespaces = {}; Object.create(null) every time you use an object as a map/set
Nit: Let's rename XMLSerializer::serializeToStringInternal to XMLSerializer::serializeToStringForPrivateScript. I'll start a discussion in blink-dev@ about a decision-making process of private APIs like this.
Patchset #7 (id:180001) has been deleted
Thank you for reviewing. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:9: var _EMPTY = ''; On 2014/10/07 15:37:10, arv wrote: > No leading underscores for variable names. Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:32: return text.replace(/&/g, '&').replace(/"/g, '"').replace(/\u00A0/g, ' '); On 2014/10/07 15:37:11, arv wrote: > It is kind of sad that we have to create all these extra strings. Maybe do: > > .replace(/&|"|\u00a0/g, function(c) { > switch (c) { > case '&': return '&'; > ... > Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:40: var clone = {}; On 2014/10/07 15:37:11, arv wrote: > Object.create(null) or {__proto__: null} or your map will break. Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:47: return isHTMLElement(node) && node.tagName.toLowerCase() == 'template'; On 2014/10/07 15:37:11, arv wrote: > node.localName === 'template' > > or > > node.tagName === 'TEMPLATE' > > no need to do extra work. Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:51: var nsRegex = new RegExp('^ns([0-9]+)$', 'g'); On 2014/10/07 15:37:11, arv wrote: > /^ns([0-9]+)$/g Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:56: var value = parseInt(match[1]); On 2014/10/07 15:37:11, arv wrote: > parseInt(x, 10) or you might get octal > > you can also use Number() Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:66: return node.tagName.toLowerCase(); On 2014/10/07 15:37:11, arv wrote: > localName should be lowercase already Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:71: var _SERIALIZED_NAMESPACES = { On 2014/10/07 15:37:11, arv wrote: > no leading underscores Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:72: _XML_NAMESPACE_URI:true, _XLINK_NAMESPACE_URI:true, On 2014/10/07 15:37:11, arv wrote: > bad formatting Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:135: var _END_TAG_OMISSIBLE_TAGS = { On 2014/10/07 15:37:11, arv wrote: > move out of function? Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:136: 'area':true, 'base':true, 'basefont':true, 'br':true, 'col':true, 'embed':true, On 2014/10/07 15:37:11, arv wrote: > bad formatting Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:147: } On 2014/10/07 15:37:11, arv wrote: > missing ; Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:214: var attr = node.attributes.item(i); On 2014/10/07 15:37:11, arv wrote: > node.attributes[i] Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:215: if (attr.name.indexOf('_moz') == 0) On 2014/10/07 15:37:11, arv wrote: > what? Is this part of the spec? Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:241: var sink = this.serializeOpenTag_(node, namespaces); On 2014/10/07 15:37:10, arv wrote: > return this.serializeOpenTag_(node, namespaces) + > this.serializeAttributes_(node, namespaces) + > this.serializeCloseTag_(node, namespaces); Done. https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:264: return '<!--' + node.nodeValue + '-->'; On 2014/10/07 15:37:11, arv wrote: > escape nodeValue? Looking at //src/third_party/WebKit/Source/core/editing/MarkupAccumulator.cpp, we don't need to escape nodeValue. i.e. case Node::COMMENT_NODE: appendComment(result, toComment(node).data()); break; https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:357: var namespaces = {}; On 2014/10/07 15:37:11, arv wrote: > Object.create(null) every time you use an object as a map/set Done. https://codereview.chromium.org/534583002/diff/200001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/200001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:368: XMLSerializerPrototype.serializeToStringForPrivateScript = function(root, xmlDeclaration) { Done.
https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... File Source/core/xml/XMLSerializer.js (right): https://codereview.chromium.org/534583002/diff/160001/Source/core/xml/XMLSeri... Source/core/xml/XMLSerializer.js:264: return '<!--' + node.nodeValue + '-->'; On 2014/10/08 12:04:25, tasak wrote: > On 2014/10/07 15:37:11, arv wrote: > > escape nodeValue? > > Looking at //src/third_party/WebKit/Source/core/editing/MarkupAccumulator.cpp, > we don't need to escape nodeValue. > i.e. > case Node::COMMENT_NODE: > appendComment(result, toComment(node).data()); > break; This will definitely not round trip but I guess that has always been the case: var x = new XMLSerializer; var c = new Comment('-->'); x.serializeToString(c); // "<!---->-->"
On 2014/10/08 13:38:13, arv wrote:
> This will definitely not round trip but I guess that has always been the case:
>
> var x = new XMLSerializer;
> var c = new Comment('-->');
> x.serializeToString(c); // "<!---->-->"
I see.
However, before applying this patch, console.log(x.serializeToString(c)) shows:
CONSOLE MESSAGE: line 4: <!---->-->
and after applying this patch,
CONSOLE MESSAGE: line 4: <!---->-->
So this patch doesn't change XMLSerializer's behavior.
I also checked Firefox's behavior. It shows "<!---->-->".
I think, it is difficult to change public web API's behavior after shipping.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
