XMLSerializer uses whichever namespace prefix was specified when creating a node, even if it conflicts with namespaces of other nodes, or is missing altogether.
Created attachment 21224 [details] test case
Created attachment 205633 [details] Patch
Comment on attachment 205633 [details] Patch Clearing review flag since this should go on top of bug 16496.
Created attachment 205745 [details] Patch
(In reply to comment #4) > Created an attachment (id=205745) [details] > Patch Rebase against new bug 16496 patch, and add the missing testcase.
Created attachment 205850 [details] Patch
Created attachment 206831 [details] Patch
Created attachment 207022 [details] Patch
Comment on attachment 207022 [details] Patch Attachment 207022 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1110701 New failing tests: editing/pasteboard/paste-noscript-svg.html
Created attachment 207038 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Created attachment 207041 [details] Patch
Comment on attachment 207041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207041&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:471 > + builder.append("NS"); why do we want to use capital letters? > Source/WebCore/editing/MarkupAccumulator.cpp:472 > + builder.appendNumber(m_prefixLevel++); What if the page had already contained NS1, NS2, etc...? It won't be unique, will it? Also, can we try to generate a prefix that resembles the namespace URL?
(In reply to comment #12) > (From update of attachment 207041 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207041&action=review > > > Source/WebCore/editing/MarkupAccumulator.cpp:471 > > + builder.append("NS"); It is stated like that in http://www.w3.org/TR/DOM-Level-3-Core/namespaces-algorithms.html#normalizeDocumentAlgo: // find a prefix following the pattern "NS" +index (starting at 1) Hmm, seems m_prefixLevel should start at 1, something else I should fix.... > why do we want to use capital letters? > > > Source/WebCore/editing/MarkupAccumulator.cpp:472 > > + builder.appendNumber(m_prefixLevel++); > > What if the page had already contained NS1, NS2, etc...? It won't be unique, will it? > Also, can we try to generate a prefix that resembles the namespace URL? Oops, you are right, the next line in the pseudo algorithm code is: // make sure this prefix is not declared in the current scope. I'll change the patch to support this as well as add a test for it.
Created attachment 207047 [details] Patch
Comment on attachment 207047 [details] Patch Attachment 207047 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1098776 New failing tests: fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts.html svg/custom/xlink-prefix-generation-in-attributes.html
Created attachment 207052 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 207047 [details] Patch Attachment 207047 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1098826 New failing tests: fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts.html svg/custom/xlink-prefix-generation-in-attributes.html
Created attachment 207059 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Created attachment 207164 [details] Patch
Comment on attachment 207164 [details] Patch Attachment 207164 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1253356 New failing tests: dom/xhtml/level2/events/dispatchEvent03.xhtml dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg dom/html/level2/events/dispatchEvent04.html dom/xhtml/level2/events/dispatchEvent02.xhtml dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg dom/xhtml/level2/events/dispatchEvent04.xhtml dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg dom/html/level2/events/dispatchEvent01.html dom/xhtml/level2/events/dispatchEvent01.xhtml dom/html/level1/core/hc_attrgetvalue2.html dom/html/level2/events/dispatchEvent03.html dom/html/level2/events/dispatchEvent02.html dom/html/level2/core/createDocumentType04.html dom/xhtml/level2/events/dispatchEvent05.xhtml dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html dom/html/level1/core/hc_attrappendchild2.html dom/html/level2/events/dispatchEvent06.html css1/basic/comments.html dom/html/level2/events/dispatchEvent07.html dom/html/level2/core/setAttributeNS10.html dom/html/level2/events/dispatchEvent05.html dom/html/level2/core/createAttributeNS06.html dom/html/level1/core/hc_attrappendchild4.html dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html dom/html/level2/core/hc_namednodemapinvalidtype1.html
Created attachment 207573 [details] Archive of layout-test-results from APPLE-EWS-9 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: APPLE-EWS-9 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
Comment on attachment 207164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207164&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:482 > + } while (namespaces && namespaces->get(builder.toAtomicString().impl())); > + prefixedName.setPrefix(builder.toString()); Shouldn't we update namespaces here?
Comment on attachment 207164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207164&action=review >> Source/WebCore/editing/MarkupAccumulator.cpp:482 >> + prefixedName.setPrefix(builder.toString()); > > Shouldn't we update namespaces here? Hmm, your comment leads me to believe I chose the wrong name, how about generateUniquePrefix? The namespace handling is not part of this method. D'oh even the m_prefixLevel name should have been a hint for me :) Also it looks like the param should be const Namespaces& to indicate it will not change. Will fix....
Created attachment 207771 [details] Patch
Comment on attachment 207771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207771&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:123 > + m_prefixLevel = 0; We don't call serializeNodes more than once, do we? > Source/WebCore/editing/MarkupAccumulator.cpp:475 > + // find a prefix following the pattern "NS" +index (starting at 1) > + // make sure this prefix is not declared in the current scope. Please capitalize words and put . at the end of each sentence. > Source/WebCore/editing/MarkupAccumulator.cpp:482 > + } while (namespaces.get(builder.toAtomicString().impl())); > + prefixedName.setPrefix(builder.toString()); It's kind of inefficient to call toAtomicString() and then toString(), isn't it? Can't we do something like: AtomicString name = builder.toAtomicString(); if (namespaces.get(name.impl()) { prefixedName.setPrefix(name); return; } ? > Source/WebCore/editing/MarkupAccumulator.cpp:499 > + // In case no prefix is given but a namespace is there, try to find the matching in-scope declaration by looking up the prefix in the namespace -> prefix mapping. > + // If still no prefix is found, we would lose the namespace associated with the attribute, so generate a unique prefix and namespace declaration for the prefix. It seems redundant to repeat what the spec. says. If anything, we should add this comment to the change log instead because it reflects the current state of the spec. > Source/WebCore/editing/MarkupAccumulator.cpp:506 > + // If the prefix is already mapped to a different namespace, generate a unique prefix. Otherwise two namespace declarations for the prefix > + // would have to be generated, violating Namespace constraint: Attributes Unique from the XMLNames specification. Instead of adding a comment like, I would prefer defining a boolean with a descriptive variable name. e.g. bool prefixIsAlreadyUsedForSomeotherNamespace = ns == attribute.namespaceURI().impl(). > LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict.html:1 > +<html> Is it intentional that DOCTYPE is omitted? > LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict.html:12 > + var doc = document.implementation.createDocument("", "", null); > + var parent = doc.firstChild; > + var el = doc.createElementNS("ns1", "x:y"); > + el.setAttributeNS("ns2", "x:z", "val"); Please spell out element.
Created attachment 209919 [details] Patch
Thanks for the review! (In reply to comment #25) > (From update of attachment 207771 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207771&action=review > > > Source/WebCore/editing/MarkupAccumulator.cpp:123 > > + m_prefixLevel = 0; > I tried to fix all issues. Note that I redid the logic in appendAttribute to handle attributes that are in namespaces to be exactly like stated in: http://www.w3.org/TR/DOM-Level-3-Core/namespaces-algorithms.html#normalizeDocumentAlgo This hopefully makes it more easy to check that we are doing the right thing.
Attachment 209919 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts-expected.txt', u'LayoutTests/fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts.html', u'LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict-expected.txt', u'LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict.html', u'LayoutTests/fast/dom/XMLSerializer-setAttributeNS-namespace-no-prefix-expected.txt', u'LayoutTests/fast/dom/XMLSerializer-setAttributeNS-namespace-no-prefix.html', u'LayoutTests/svg/custom/xlink-prefix-generation-in-attributes-expected.txt', u'LayoutTests/svg/custom/xlink-prefix-generation-in-attributes.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/editing/MarkupAccumulator.cpp', u'Source/WebCore/editing/MarkupAccumulator.h']" exit_code: 1 Source/WebCore/editing/MarkupAccumulator.cpp:512: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:487 > + AtomicString name = builder.toAtomicString(); Why not use a const reference? >> Source/WebCore/editing/MarkupAccumulator.cpp:512 >> + ; // declare prefix using appendNamespace > > Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] What happened here?
Comment on attachment 209919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:511 > + if (AtomicStringImpl* prefix = namespaces ? namespaces->get(attribute.namespaceURI().impl()) : 0) { > + prefixedName.setPrefix(AtomicString(prefix)); > + } else if (!attribute.prefix().isEmpty() && !foundNS) { There should be no curly brackets around a single-line statement.
Comment on attachment 209919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review >>> Source/WebCore/editing/MarkupAccumulator.cpp:512 >>> + ; // declare prefix using appendNamespace >> >> Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] > > What happened here? It seems like this case is hit when prefixIsAlreadyMappedToOtherNS is true, the prefix is present, and hasn't appeared elsewhere yet? Perhaps we can tweak the condition (and the name) of prefixIsAlreadyMappedToOtherNS so that we don't have to check it here again.
(In reply to comment #29) > (From update of attachment 209919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review > > > Source/WebCore/editing/MarkupAccumulator.cpp:487 > > + AtomicString name = builder.toAtomicString(); > > Why not use a const reference? Sure, will fix. > >> Source/WebCore/editing/MarkupAccumulator.cpp:512 > >> + ; // declare prefix using appendNamespace > > > > Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] > > What happened here? I had fixed it locally but it was not part of the commit :( Will fix.
Created attachment 209921 [details] Patch
(In reply to comment #31) > (From update of attachment 209919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review > > >>> Source/WebCore/editing/MarkupAccumulator.cpp:512 > >>> + ; // declare prefix using appendNamespace > >> > >> Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] > > > > What happened here? > > It seems like this case is hit when prefixIsAlreadyMappedToOtherNS is true, the prefix is present, and hasn't appeared elsewhere yet? > Perhaps we can tweak the condition (and the name) of prefixIsAlreadyMappedToOtherNS so that we don't have to check it here again. I don't understand this fully? I did notice in my previous patch prefixIsAlreadyMappedToOtherNS did not match what it does, i.e. it didn't check that a namespace was found in the first place (foundNS). With this new patch it should follow the pseudo code 1:1. Let me know if we can easily eliminate checks, I am not seeing it so far.
Comment on attachment 209921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209921&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:508 > + if (!attribute.prefix() || !foundNS || prefixIsAlreadyMappedToOtherNS) { What is this "!attribute.prefix()" checking here? prefix() is AtomicString&, right? > Source/WebCore/editing/MarkupAccumulator.cpp:511 > + else if (!attribute.prefix().isEmpty() && !foundNS) { Can we define a new boolean like shouldDeclaredUsingAppendNamespace and then check it in the following else if instead of having this empty block?
Created attachment 209923 [details] Patch
(In reply to comment #35) > (From update of attachment 209921 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209921&action=review > > > Source/WebCore/editing/MarkupAccumulator.cpp:508 > > + if (!attribute.prefix() || !foundNS || prefixIsAlreadyMappedToOtherNS) { > > What is this "!attribute.prefix()" checking here? prefix() is AtomicString&, right? Yeah that is possibly dangerous, using isEmpty() now. > > Source/WebCore/editing/MarkupAccumulator.cpp:511 > > + else if (!attribute.prefix().isEmpty() && !foundNS) { > > Can we define a new boolean like shouldDeclaredUsingAppendNamespace and then check it in the following else if instead of having this empty block? I think I see what you mean, fixed hopefully.
Comment on attachment 209923 [details] Patch Clearing flags on attachment: 209923 Committed r154779: <http://trac.webkit.org/changeset/154779>
All reviewed patches have been landed. Closing bug.
Mass moving XML DOM bugs to the "DOM" Component.