[webkit-reviews] review granted: [Bug 16496] XMLSerializer doesn't include namespaces on nodes in HTML documents : [Attachment 206684] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 16496: XMLSerializer doesn't include namespaces on nodes in HTML documents
https://bugs.webkit.org/show_bug.cgi?id=16496

Attachment 206684: Patch
https://bugs.webkit.org/attachment.cgi?id=206684&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=206684&action=review


> Source/WebCore/ChangeLog:9
> +	   http://html5.org/specs/dom-parsing.html#xmlserializer (23 May 2013).
In this mode

Could you refer to a specific hash instead?

> Source/WebCore/ChangeLog:27
> +	   (WebCore::MarkupAccumulator::serializeNodesWithNamespaces): makes
sure xml namespace/prefix is known so it is never used in namespace
declarations.
> +	   (WebCore::MarkupAccumulator::appendNamespace): Avoid adding
namespace declarations that do not differ from current default namespace.
> +	   (WebCore::MarkupAccumulator::appendOpenTag): Print xml prefix if the
element's namespace is XML to avoid conflicts.
> +	   (WebCore::MarkupAccumulator::appendAttribute): Force any calculated
prefix/namespace pair into the generated namespace.
> +	   (WebCore::MarkupAccumulator::shouldAddNamespaceAttribute): Also take
into account xmlns attributes with no namespace.
> +	   (WebCore::MarkupAccumulator::shouldSelfClose): Force self-closing to
create well-formed XML elements.
> +	   * editing/MarkupAccumulator.h: Use EFragmentSerialization.

It's still unclear to me which parts are referring to the spec mentioned about
and which parts are matching Firefox/IE. It's helpful have that information in
the change log, not in the code.

> Source/WebCore/editing/MarkupAccumulator.cpp:144
> +	   // Make sure xml prefix and namespace are always known to ensure
these Namespace constraints:
> +	   // The prefix xml MUST NOT be declared as the default namespace.
> +	   // The prefix xml MAY, but need not, be declared (XMLNames
specification, Namespace constraint: Reserved Prefixes and Namespace Names)

Instead of copying & pasting the spec. text, can we refer to a specific section
from which this text is copied?

> Source/WebCore/editing/MarkupAccumulator.cpp:436
> +	   // According to DOM3 appendix B Namespace Normalization we now
should create a default namespace declaration to make this namespace
well-formed.
> +	   // However, XMLNames specification, Namespace constraint: Reserved
Prefixes and Namespace Names says:
> +	   // "The prefix xml MUST NOT be declared as the default namespace.",
so use the xml prefix explicitly.

Ditto about referring to sections of the specifications.