Summary: | serializeNodesWithNamespaces should be in MarkupAccumulator | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Enhancement | CC: | abarth, eric, tony | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 47748 | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2010-10-15 16:04:42 PDT
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> |