Bug 47749 - serializeNodesWithNamespaces should be in MarkupAccumulator
: serializeNodesWithNamespaces should be in MarkupAccumulator
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
: 47748
  Show dependency treegraph
 
Reported: 2010-10-15 16:04 PST by
Modified: 2010-10-15 20:16 PST (History)


Attachments
cleanup (14.15 KB, patch)
2010-10-15 17:04 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
fixed per Adam's & Tony's comments (14.23 KB, patch)
2010-10-15 17:33 PST, Ryosuke Niwa
tony: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-10-15 16:04:42 PST
serializeNodesWithNamespaces should be a member function of MarkupAccumulator, and serializeNodes should be a member function of StyledMarkupAccumulator.
------- Comment #1 From 2010-10-15 17:04:54 PST -------
Created an attachment (id=70925) [details]
cleanup
------- Comment #2 From 2010-10-15 17:11:07 PST -------
(From update of attachment 70925 [details])
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?
------- Comment #3 From 2010-10-15 17:20:20 PST -------
(From update of attachment 70925 [details])
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?
------- Comment #4 From 2010-10-15 17:27:25 PST -------
(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.
------- Comment #5 From 2010-10-15 17:31:18 PST -------
(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?
------- Comment #6 From 2010-10-15 17:32:34 PST -------
(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.
------- Comment #7 From 2010-10-15 17:33:07 PST -------
Created an attachment (id=70930) [details]
fixed per Adam's & Tony's comments
------- Comment #8 From 2010-10-15 17:49:21 PST -------
(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?

> 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
------- Comment #9 From 2010-10-15 18:04:12 PST -------
(In reply to comment #8)
> (From update of attachment 70930 [details] [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.
------- Comment #10 From 2010-10-15 20:13:39 PST -------
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.
------- Comment #11 From 2010-10-15 20:16:31 PST -------
Committed r69909: <http://trac.webkit.org/changeset/69909>