[webkit-reviews] review granted: [Bug 43936] Group functions in markup.cpp into MarkupAccumulatorWrapper : [Attachment 64776] fixed per Darin's comment

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 43936: Group functions in markup.cpp into MarkupAccumulatorWrapper
https://bugs.webkit.org/show_bug.cgi?id=43936

Attachment 64776: fixed per Darin's comment
https://bugs.webkit.org/attachment.cgi?id=64776&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Vector<String> m_reversedPreceedingMarkup;

There's a spelling mistake here. It's "preceding" rather than "preceeding".

> +void MarkupAccumulator::appendStartTag(Node* node, const Range* range,
EAnnotateForInterchange annotate, EAbsoluteURLs shouldResolveURLs, bool
convertBlocksToInlines, Namespaces* namespaces, RangeFullySelectsNode
rangeFullySelectsNode)
> +{
> +    Vector<UChar> result;
> +    appendStartMarkup(result, node, range, annotate, shouldResolveURLs,
convertBlocksToInlines, namespaces, rangeFullySelectsNode);
> +    m_succeedingMarkup.append(String::adopt(result));
> +    if (m_nodes)
> +	   m_nodes->append(const_cast<Node*>(node));

There is no need for const_cast here.

> +void MarkupAccumulator::wrapWithNode(Node* node, const Range* range,
EAnnotateForInterchange annotate, EAbsoluteURLs shouldResolveURLs, bool
convertBlocksToInlines, Namespaces* namespaces, RangeFullySelectsNode
rangeFullySelectsNode)
> +{
> +    Vector<UChar> result;
> +    appendStartMarkup(result, node, range, annotate, shouldResolveURLs,
convertBlocksToInlines, namespaces, rangeFullySelectsNode);
> +    m_reversedPreceedingMarkup.append(String::adopt(result));
> +    appendEndTag(node);
> +    if (m_nodes)
> +	   m_nodes->append(const_cast<Node*>(node));

There is no need for const_cast here.

> +    WTF::append(openTag, isBlock ? divStyle : styleSpanOpen);

Do you really need to explicitly qualify WTF::append with the namespace here?
Doesn't just calling append work due to argument-depedendent lookup?

> +    size_t preCount = m_reversedPreceedingMarkup.size();
> +    for (size_t i = 0; i < preCount; ++i)
> +	   length += m_reversedPreceedingMarkup[i].length();
> +    
> +    size_t postCount = m_succeedingMarkup.size();
> +    for (size_t i = 0; i < postCount; ++i)
> +	   length += m_succeedingMarkup[i].length();

If you added a helper function here you could just call it twice.

> +    for (size_t i = preCount; i > 0; --i)
> +	   WTF::append(result, m_reversedPreceedingMarkup[i - 1]);
> +    
> +    for (size_t i = 0; i < postCount; ++i)
> +	   WTF::append(result, m_succeedingMarkup[i]);

Same here. You could make an append function that handled the entire vector.

> +    const UChar* uchars = attribute.characters();

The name "uchars" is not so great. I think "characters" would be better.

> +    unsigned len = attribute.length();

Could you use the name "length" instead of "len"?

I guess those names are in the existing code, so you can fix this later.