Bug 190308 - Rename MarkupAccumulator::appendStartTag, appendElement, etc... for clarity
Summary: Rename MarkupAccumulator::appendStartTag, appendElement, etc... for clarity
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-05 01:23 PDT by Ryosuke Niwa
Modified: 2018-10-12 14:30 PDT (History)
6 users (show)

See Also:


Attachments
Cleanup (18.81 KB, patch)
2018-10-05 20:53 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (19.38 KB, patch)
2018-10-10 00:32 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated change log (19.46 KB, patch)
2018-10-10 00:37 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-10-05 01:23:47 PDT
Rename a bunch of member functions to reflect what they really do.
Comment 1 Ryosuke Niwa 2018-10-05 20:53:32 PDT
Created attachment 351713 [details]
Cleanup
Comment 2 Darin Adler 2018-10-07 21:02:23 PDT
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?
Comment 3 Ryosuke Niwa 2018-10-10 00:19:16 PDT
(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.
Comment 4 Ryosuke Niwa 2018-10-10 00:32:16 PDT
Created attachment 351949 [details]
Patch for landing
Comment 5 Ryosuke Niwa 2018-10-10 00:37:21 PDT
Created attachment 351950 [details]
Updated change log
Comment 6 Darin Adler 2018-10-10 16:44:45 PDT
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.
Comment 7 Ryosuke Niwa 2018-10-10 18:51:32 PDT
(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.
Comment 8 Ryosuke Niwa 2018-10-10 19:27:20 PDT
Committed r237025: <https://trac.webkit.org/changeset/237025>
Comment 9 Radar WebKit Bug Importer 2018-10-10 19:28:24 PDT
<rdar://problem/45182101>
Comment 10 Darin Adler 2018-10-12 12:13:02 PDT
The final version of this that landed still has one missing const for the localNames array.
Comment 11 Ryosuke Niwa 2018-10-12 13:03:53 PDT
(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.
Comment 12 Ryosuke Niwa 2018-10-12 14:30:27 PDT
Fixed it in https://trac.webkit.org/changeset/237077.