WebKit Bugzilla
serializeNodesWithNamespaces should be a member function of MarkupAccumulator, and serializeNodes should be a member function of StyledMarkupAccumulator.
Created attachment 70925 [details] cleanup
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?
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?
(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.
(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?
(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.
Created attachment 70930 [details] fixed per Adam's & Tony's comments
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
(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.
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.
Committed r69909: <http://trac.webkit.org/changeset/69909>