Bug 190308 – Rename MarkupAccumulator::appendStartTag, appendElement, etc... for clarity

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Rename a bunch of member functions to reflect what they really do.

[Ryosuke Niwa]

Created attachment 351713 [details]
Cleanup

[Darin Adler]

Comment on attachment 351713 [details]
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?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #2)
> Comment on attachment 351713 [details]
> 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(), ...

Good point. Fixed.

> > 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?

I think so. Element::innerHTML, for example, always trigger HTML serialization but could be called on a MathML element or on an HTML element inside a XML document for example.

We should probably consult https://w3c.github.io/DOM-Parsing/ and other relevant specs and what we're doing here makes sense, or update the spec as needed. This is a pretty subtly weird behavior we have.

> > 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?

template<unsigned characterCount>
ALWAYS_INLINE void appendLiteral(const char (&characters)[characterCount]) { append(characters, characterCount - 1); }

ends up invoking append function whereas append has a fast path so I'd imagine calling two append is faster here.

> > 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".

Oh, those names sound better. Yeah, I was bothered by it too.
I'd use startAppendingNode and endAppendingNode to match startTag / endTag terminology.

> > 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.

Start tags and end tags are totally things in the spec:
https://html.spec.whatwg.org/multipage/syntax.html#start-tags
https://html.spec.whatwg.org/multipage/syntax.html#end-tags

> > 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?

Oh, that's a good catch. Will add appendString which takes StringView and use it here.

[Ryosuke Niwa]

Created attachment 351949 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 351950 [details]
Updated change log

[Darin Adler]

Comment on attachment 351950 [details]
Updated change log

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

> Source/WebCore/editing/MarkupAccumulator.cpp:118
> +    auto& element = downcast<HTMLElement>(node);

I think we should have an elementName local variable instead of an element local variable to hoist it out of the loop.

> Source/WebCore/editing/MarkupAccumulator.h:76
> +    void appendStringView(const StringView&);

Normally we use StringView instead of const StringView& since it is more efficient and there is no worry about reference count churn.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #6)
> Comment on attachment 351950 [details]
> Updated change log
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351950&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:118
> > +    auto& element = downcast<HTMLElement>(node);
> 
> I think we should have an elementName local variable instead of an element
> local variable to hoist it out of the loop.

Oh, good point. Will fix.

> > Source/WebCore/editing/MarkupAccumulator.h:76
> > +    void appendStringView(const StringView&);
> 
> Normally we use StringView instead of const StringView& since it is more
> efficient and there is no worry about reference count churn.

Will fix.

[Ryosuke Niwa]

Committed r237025: <https://trac.webkit.org/changeset/237025>

[Radar WebKit Bug Importer]

<rdar://problem/45182101>

[Darin Adler]

The final version of this that landed still has one missing const for the localNames array.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #10)
> The final version of this that landed still has one missing const for the
> localNames array.

Oh oops. When I changed it to AtomicStringImpl, I somehow lost const.

[Ryosuke Niwa]

Fixed it in https://trac.webkit.org/changeset/237077.