Bug 47749 – serializeNodesWithNamespaces should be in MarkupAccumulator

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

serializeNodesWithNamespaces should be a member function of MarkupAccumulator, and serializeNodes should be a member function of StyledMarkupAccumulator.

[Ryosuke Niwa]

Created attachment 70925 [details]
cleanup

[Adam Barth]

Comment on attachment 70925 [details]
cleanup

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

I think there are more webkit data structures we can use here to help, but this is clearly an improvement.

> WebCore/editing/MarkupAccumulator.cpp:133
> +    size_t vectorSize = strings.size();
> +    for (size_t i = 0; i < vectorSize; ++i)

No need to make a temporary for strings.size()

> WebCore/editing/MarkupAccumulator.cpp:141
> -String MarkupAccumulator::takeResults()
> +void MarkupAccumulator::concatenateMarkups(Vector<UChar>& out)

concatenateMarkups => concatenateMarkup ?

> WebCore/editing/MarkupAccumulator.h:78
> +        out.reserveInitialCapacity(length());
> +        concatenateMarkups(out);
> +        return String::adopt(out);

Why not String::createUninitialized?

[Tony Chang]

Comment on attachment 70925 [details]
cleanup

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

> WebCore/editing/MarkupAccumulator.cpp:129
> +size_t MarkupAccumulator::sumOfStringLengths(const Vector<String>& strings) const

This method doesn't need to be part of MarkupAccumulator.  You can make it a static function (outside the class).

> WebCore/editing/MarkupAccumulator.h:114
> +    void serializeNodes(Node*, Node* nodeToSkip, EChildrenOnly, const Namespaces*);

Shouldn't this return a String like your ChangeLog says?

> WebCore/editing/markup.cpp:734
> +    return accumulator.serializeNodes(const_cast<Node*>(node), deleteButtonContainerElement, childrenOnly);

Doesn't serializeNodes return void and we want to return a string?

[Ryosuke Niwa]

(In reply to comment #3)
> > WebCore/editing/MarkupAccumulator.cpp:129
> > +size_t MarkupAccumulator::sumOfStringLengths(const Vector<String>& strings) const
> 
> This method doesn't need to be part of MarkupAccumulator.  You can make it a static function (outside the class).

It's used in StyledMarkupAccumulator so it needs to be a protected member.

> > WebCore/editing/MarkupAccumulator.h:114
> > +    void serializeNodes(Node*, Node* nodeToSkip, EChildrenOnly, const Namespaces*);
> 
> Shouldn't this return a String like your ChangeLog says?

Not this one. This is a helper for the public version of serializeNodes.  Let me rename that to something else so that there's no confusion.

> > WebCore/editing/markup.cpp:734
> > +    return accumulator.serializeNodes(const_cast<Node*>(node), deleteButtonContainerElement, childrenOnly);
> 
> Doesn't serializeNodes return void and we want to return a string?

There's a version of serializeNodes that returns String in MarkupAccumulator.h.  But I agree that it's quite confusing.

[Tony Chang]

(In reply to comment #4)
> (In reply to comment #3)
> > > WebCore/editing/MarkupAccumulator.cpp:129
> > > +size_t MarkupAccumulator::sumOfStringLengths(const Vector<String>& strings) const
> > 
> > This method doesn't need to be part of MarkupAccumulator.  You can make it a static function (outside the class).
> 
> It's used in StyledMarkupAccumulator so it needs to be a protected member.

I see, can it be a static method on the class?

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > > WebCore/editing/MarkupAccumulator.cpp:129
> > > > +size_t MarkupAccumulator::sumOfStringLengths(const Vector<String>& strings) const
> > > 
> > > This method doesn't need to be part of MarkupAccumulator.  You can make it a static function (outside the class).
> > 
> > It's used in StyledMarkupAccumulator so it needs to be a protected member.
> 
> I see, can it be a static method on the class?

Mn... if I do that then I have to make a bunch of protected methods public and I don't really want to do that.

[Ryosuke Niwa]

Created attachment 70930 [details]
fixed per Adam's & Tony's comments

[Tony Chang]

Comment on attachment 70930 [details]
fixed per Adam's & Tony's comments

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

FWIW, you can call protected static methods on a parent class.

Adam asked the following:
> WebCore/editing/MarkupAccumulator.h:78
> +        out.reserveInitialCapacity(length());
> +        concatenateMarkups(out);
> +        return String::adopt(out);

Why not String::createUninitialized?

> WebCore/ChangeLog:22
> +        (WebCore::MarkupAccumulator::length): Added; calls sumOfStringLengths.

Nit: calls totalLength

> WebCore/ChangeLog:26
> +        (WebCore::StyledMarkupAccumulator::takeResults): Calls length, sumOfStringLengths, and concatenateMarkups.

Nit: totalLength, and concatenateMarkup

[Adam Barth]

(In reply to comment #8)
> (From update of attachment 70930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70930&action=review
> 
> FWIW, you can call protected static methods on a parent class.
> 
> Adam asked the following:
> > WebCore/editing/MarkupAccumulator.h:78
> > +        out.reserveInitialCapacity(length());
> > +        concatenateMarkups(out);
> > +        return String::adopt(out);
> 
> Why not String::createUninitialized?

rniwa and I talked about that.  I think we're going to do that in a subsequent patch.

[Ryosuke Niwa]

Thanks for the review!

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > > WebCore/editing/MarkupAccumulator.cpp:129
> > > > > +size_t MarkupAccumulator::sumOfStringLengths(const Vector<String>& strings) const
> > > > 
> > > > This method doesn't need to be part of MarkupAccumulator.  You can make it a static function (outside the class).
> > > 
> > > It's used in StyledMarkupAccumulator so it needs to be a protected member.
> > 
> > I see, can it be a static method on the class?
> 
> Mn... if I do that then I have to make a bunch of protected methods public and I don't really want to do that.

Oops, this comment is totally bogus.  I thought you meant the old serializeNodes (now serializeNodesWithNamespaces).  Will make it static.

(In reply to comment #8)
> Adam asked the following:
> > WebCore/editing/MarkupAccumulator.h:78
> > +        out.reserveInitialCapacity(length());
> > +        concatenateMarkups(out);
> > +        return String::adopt(out);
> 
> Why not String::createUninitialized?

That's because concatenateMarkups is also used in StyledMarkupAccumulator, and I didn't want to create two strings there.  We need to figure out a cleaner way of doing this after this patch is landed.

> > WebCore/ChangeLog:22
> > +        (WebCore::MarkupAccumulator::length): Added; calls sumOfStringLengths.
> 
> Nit: calls totalLength
> 
> > WebCore/ChangeLog:26
> > +        (WebCore::StyledMarkupAccumulator::takeResults): Calls length, sumOfStringLengths, and concatenateMarkups.
> 
> Nit: totalLength, and concatenateMarkup

Thanks for noticing those.  Fixed.

[Ryosuke Niwa]

Committed r69909: <http://trac.webkit.org/changeset/69909>