Bug 43936 – Group functions in markup.cpp into MarkupAccumulatorWrapper

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

This is a cleanup bug to make functions in markup.cpp private member functions of MarkupAccumulator.  We should also rename MarkupAccumulatorWrapper to MarkupAccumulator since the original accumulator has been removed as of http://trac.webkit.org/changeset/65265.

[Ryosuke Niwa]

Created attachment 64273 [details]
Patch

[Ryosuke Niwa]

Created attachment 64723 [details]
Removed unnecessary cleanups

[Darin Adler]

Comment on attachment 64723 [details]
Removed unnecessary cleanups

> +    MarkupAccumulator(Vector<Node*>* nodes)
> +    : m_nodes(nodes)
> +    {
> +    }

This does not have the correct formatting. The ":" line should be indented one tab stop.

> +    void insertString(const String& s);

You should omit the letter "s" here. It adds no value.

> +    void appendAttributeValue(Vector<UChar>& result, const String& attr, bool escapeNBSP);

Please consider using the word "attribute" rather than "attr" for the name here.

> +    String escapeContentText(const String& in, bool escapeNBSP);

I don't think the word "in" here as an attribute value adds any clarity.

> +    pair<const UChar*, size_t> ucharRange(const Node*, const Range *);

The name ucharRange is quite strange. It's also not our normal style to leave a space after Range before "*".

> +    void appendUCharRange(Vector<UChar>& result, const pair<const UChar*, size_t> range);

Naming the vector "result" is a little strange since it's an in/out argument. The pair argument should probably be a const pair& rather than just a const pair.

It might be better to have a typedef for the pair.

> +    bool shouldAddNamespaceElem(const Element*);

Please don't abbreviate "element" as "elem" in the name of this. Also, why take a const Element* rather than an Element*? Since these classes are in a tree, it's rarely useful to use a const* rather than just a *.

> +    bool shouldAddNamespaceAttr(const Attribute*, HashMap<AtomicStringImpl*, AtomicStringImpl*>& namespaces);

Please don't abbreviate "attribute" as "attr" in the name of this. Same comment about const Element*.

It might be better to have a typedef for the map type.

> +    void appendNamespace(Vector<UChar>& result, const AtomicString& prefix, const AtomicString& ns, HashMap<AtomicStringImpl*, AtomicStringImpl*>& namespaces);

Please don't abbreviate "namespace" to "ns". But since you can't use "namespace" because it's a reserved word you probably need to say namespaceURI or something like that. Whatever QualifiedName.h uses.

> +    void appendDocumentType(Vector<UChar>& result, const DocumentType*);

Same comment about "result".

> +    void appendStartMarkup(Vector<UChar>& result, const Node*, const Range*, EAnnotateForInterchange, EAbsoluteURLs, bool convertBlocksToInlines, HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces, RangeFullySelectsNode);

Same comment abouve "result".

> +    bool shouldSelfClose(const Node *node);

Should just be Node*. No need to include the name "node", and the * is in the wrong place, and const doesn't help here.

> +    void appendEndMarkup(Vector<UChar>& result, const Node*);

Same comments about "result" and "const".

> +    Vector<Node*>* m_nodes;
> +    Vector<String> preMarkups;
> +    Vector<String> postMarkups;

In a new class it doesn't make sense to mix naming styles. If members need an "m_" prefix, then all three should have it. Also, the names "pre markups" and "post markups" aren't really clear data member names. We should choose names that are more normal language that you might use when talking to someone. Even "prefix strings" and "suffix strings" would be better, I think.

> +void MarkupAccumulator::insertString(const String& s)
> +{
> +    postMarkups.append(s);
> +}

I don't understand why we use "insert" to mean "add to the end". Normally we use the word "append" for that. Or if we want to be vague it could be "add". But "insert" almost always means to add something somewhere other than the end.

Also, please use the word "string" rather than the letter "s".

> +void MarkupAccumulator::insertOpenTag(const Node* node, const Range* range, EAnnotateForInterchange annotate, EAbsoluteURLs absoluteURLs, bool convertBlocksToInlines, HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces, RangeFullySelectsNode rangeFullySelectsNode)

Calling the enum value "absolute URLs" isn't great. The argument indicates how URLs should be handled, and is not actually a set of URLs. So the name could be "absoluteURLPolicy" or something along those lines.

I think "add" or "append" would be better than "insert".

> +    if (m_nodes)
> +        m_nodes->append(const_cast<Node*>(node));

Use of const_cast like this indicates a design mistake. We should not be using const pointers at all. If there is an existing mistake of using them, we should put the const_cast right at the entry point to the outer functions, and then fix the entry point later.

> +void MarkupAccumulator::insertEndTag(const Node* node)

I think "add" or "append" would be better than "insert".

> +static bool propertyMissingOrEqualToNone(CSSStyleDeclaration* style, int propertyID);

This declaration should be at the top of the file, not in the middle. The name "style" should be omitted.

I don't have time to review the rest of this patch right now, although I would like to.

review- because I want you to fix some of the things I mentioned

[Ryosuke Niwa]

Created attachment 64776 [details]
fixed per Darin's comment

[Ryosuke Niwa]

Thanks for the detailed review.  I wonder why check-webkit-style didn't catch any of that.

(In reply to comment #3)
> (From update of attachment 64723 [details])
> > +    MarkupAccumulator(Vector<Node*>* nodes)
> > +    : m_nodes(nodes)
> > +    {
> > +    }
> 
> This does not have the correct formatting. The ":" line should be indented one tab stop.
Fixed.

> > +    void insertString(const String& s);
> 
> You should omit the letter "s" here. It adds no value.

Done.

> > +    void appendAttributeValue(Vector<UChar>& result, const String& attr, bool escapeNBSP);
> 
> Please consider using the word "attribute" rather than "attr" for the name here.

Done.

> > +    String escapeContentText(const String& in, bool escapeNBSP);
> 
> I don't think the word "in" here as an attribute value adds any clarity.

Removed.

> > +    pair<const UChar*, size_t> ucharRange(const Node*, const Range *);
> 
> The name ucharRange is quite strange. It's also not our normal style to leave a space after Range before "*".
> 
> > +    void appendUCharRange(Vector<UChar>& result, const pair<const UChar*, size_t> range);
> 
> Naming the vector "result" is a little strange since it's an in/out argument. The pair argument should probably be a const pair& rather than just a const pair.
> 
> It might be better to have a typedef for the pair.

Right, all of this needs to be fixed but I'd like to postpone that for the sake of keeping the patch focused on grouping functions.  I'm planning to to do more cleanup afterwards.  Namely, ucharRange is called only in appendStartMarkup where it calls appendUCharRange and appendEscapedContent for Node::TEXT_Node.  I should be able to come up with a better way to pass information from ucharRange to appendEscapedContent and cleanup the whole mess there.

> > +    bool shouldAddNamespaceElem(const Element*);
> 
> Please don't abbreviate "element" as "elem" in the name of this. Also, why take a const Element* rather than an Element*? Since these classes are in a tree, it's rarely useful to use a const* rather than just a *.

Fixed.  Removing const requires modifying a bunch of places so I'll leave it as const for now.

> > +    bool shouldAddNamespaceAttr(const Attribute*, HashMap<AtomicStringImpl*, AtomicStringImpl*>& namespaces);
> 
> Please don't abbreviate "attribute" as "attr" in the name of this. Same comment about const Element*.

Fixed.

> It might be better to have a typedef for the map type.

Good idea.

> > +    void appendNamespace(Vector<UChar>& result, const AtomicString& prefix, const AtomicString& ns, HashMap<AtomicStringImpl*, AtomicStringImpl*>& namespaces);
> 
> Please don't abbreviate "namespace" to "ns". But since you can't use "namespace" because it's a reserved word you probably need to say namespaceURI or something like that. Whatever QualifiedName.h uses.

Fixed.

> > +    void appendDocumentType(Vector<UChar>& result, const DocumentType*);
> 
> Same comment about "result".

Renaming result requires a lot of modification in the code, and I'd like to postpone it until when I clean up each method.  But if you're taking about just the class declaration, then I'm more than happy to change it to whatever you'd like to call it.

> > +    bool shouldSelfClose(const Node *node);
> 
> Should just be Node*. No need to include the name "node", and the * is in the wrong place, and const doesn't help here.

Fixed.

> > +    void appendEndMarkup(Vector<UChar>& result, const Node*);
> 
> Same comments about "result" and "const".

Removing const here requires a lot of code change.  I'd like to postpone this as well.

> > +    Vector<Node*>* m_nodes;
> > +    Vector<String> preMarkups;
> > +    Vector<String> postMarkups;
> 
> In a new class it doesn't make sense to mix naming styles. If members need an "m_" prefix, then all three should have it. Also, the names "pre markups" and "post markups" aren't really clear data member names. We should choose names that are more normal language that you might use when talking to someone. Even "prefix strings" and "suffix strings" would be better, I think.

I agree it's very confusing because preMarkups has markup in the reversed order.  It has the outermost element at the end and we're reversing the order in takeResults. Renamed them to m_reversedPreceedingMarkup and m_succeedingMarkup.

> > +void MarkupAccumulator::insertString(const String& s)
> > +{
> > +    postMarkups.append(s);
> > +}
> 
> I don't understand why we use "insert" to mean "add to the end". Normally we use the word "append" for that. Or if we want to be vague it could be "add". But "insert" almost always means to add something somewhere other than the end.

Good point, fixed.

> Also, please use the word "string" rather than the letter "s".

Right, fixed.

> > +void MarkupAccumulator::insertOpenTag(const Node* node, const Range* range, EAnnotateForInterchange annotate, EAbsoluteURLs absoluteURLs, bool convertBlocksToInlines, HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces, RangeFullySelectsNode rangeFullySelectsNode)
> 
> Calling the enum value "absolute URLs" isn't great. The argument indicates how URLs should be handled, and is not actually a set of URLs. So the name could be "absoluteURLPolicy" or something along those lines.

Renamed to shouldResolveURLs.

> > +    if (m_nodes)
> > +        m_nodes->append(const_cast<Node*>(node));
> 
> Use of const_cast like this indicates a design mistake. We should not be using const pointers at all. If there is an existing mistake of using them, we should put the const_cast right at the entry point to the outer functions, and then fix the entry point later.

Fixed.  createMarkup (node version) takes in const Node* so added a const_cast where it calls serializeNodesWithNamespaces

> > +void MarkupAccumulator::insertEndTag(const Node* node)
> 
> I think "add" or "append" would be better than "insert".

Fixed.

> > +static bool propertyMissingOrEqualToNone(CSSStyleDeclaration* style, int propertyID);
> 
> This declaration should be at the top of the file, not in the middle. The name "style" should be omitted.

Moved.

[Darin Adler]

Comment on attachment 64776 [details]
fixed per Darin's comment

> +    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.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 64776 [details])
> > +    Vector<String> m_reversedPreceedingMarkup;
> 
> There's a spelling mistake here. It's "preceding" rather than "preceeding".

Will fix.

> > +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.

Will fix.

> > +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.

Will fix.

> > +    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?

No. It's a left over from old days.  Will fix.

> > +    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.

Right but as the comment above indicates, I'm intending to rewrite the whole method, or the way we accumulate markups.
So I'll postpone creating helper functions until then.

> > +    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.

Will do when I cleanup those functions.

[Ryosuke Niwa]

Committed r65647: <http://trac.webkit.org/changeset/65647>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65647 might have broken SnowLeopard Intel Release (Tests)