[webkit-reviews] review granted: [Bug 190308] Rename MarkupAccumulator::appendStartTag, appendElement, etc... for clarity : [Attachment 351713] Cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 190308: Rename MarkupAccumulator::appendStartTag, appendElement, etc... for
clarity
https://bugs.webkit.org/show_bug.cgi?id=190308

Attachment 351713: Cleanup

https://bugs.webkit.org/attachment.cgi?id=351713&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 351713
  --> https://bugs.webkit.org/attachment.cgi?id=351713
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=351713&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:99
> +    static const HTMLQualifiedName* tags[] = { &areaTag.get(),
&baseTag.get(), &basefontTag.get(), &bgsoundTag.get(),
> +	   &brTag.get(), &colTag.get(), &embedTag.get(), &frameTag.get(),
&hrTag.get(), &imgTag.get(), &inputTag.get(),
> +	   &keygenTag.get(), &linkTag.get(), &metaTag.get(), &paramTag.get(),
&sourceTag.get(), &trackTag.get(), &wbrTag.get() };

Just moved code, but:

1) This is missing a const. Should be "const HTMLQualifiedName* const tags[]".

2) This could just be an array of local names and we could use hasLocalName
instead; would be *slightly* more efficient since we would have to follow one
fewer pointer each time through the loop.

    static const AtomicString* const localNames[] = {
&areaTag.get().localName(), ...

> Source/WebCore/editing/MarkupAccumulator.cpp:118
> +    if (syntax != SerializationSyntax::XML &&
element.document().isHTMLDocument())
> +	   return false;

Just moved code, but:

Why does the document matter at all here? Is there actually a case where
serialization syntax is HTML and the document is not an HTML document? And in
that case *do* we really need to use self-closing syntax?

> Source/WebCore/editing/MarkupAccumulator.cpp:242
> +    result.append('<');
> +    result.append('/');

Just moved code, but:

Is this more efficient than appendLiteral with a two character literal?

> Source/WebCore/editing/MarkupAccumulator.h:78
> +    void appendNodeStart(const Node&, Namespaces* = nullptr);
> +    void appendNodeEnd(const Node& node)

Not thrilled with the names here. This doesn’t append a "node start" and then
later append a "node end", it starts the process of appending the markup for a
node. So logical name would be more like "startAppendingNode" and
"finishAppendingNode".

> Source/WebCore/editing/MarkupAccumulator.h:85
> +    virtual void appendStartTag(StringBuilder&, const Element&,
Namespaces*);
> +    virtual void appendEndTag(StringBuilder&, const Element&);

I would call these "append tag start" and "append tag end". Naming them "append
start tag" makes it sound like there are things, called "start tags" and "end
tags", but there are not.

> Source/WebCore/editing/markup.cpp:715
> -	   appendTextSubstring(textChild, start, msoListDefinitionsEnd - start
+ 3);
> +	   appendString(textChild.data().substring(start, msoListDefinitionsEnd
- start + 3));

This removes an optimization to avoid an extra copy of the substring. Is there
some way to preserve that optimization? Maybe involving StringView? Or is it
not an important optimization?