[webkit-reviews] review denied: [Bug 43936] Group functions in markup.cpp into MarkupAccumulatorWrapper : [Attachment 64723] Removed unnecessary cleanups

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 64723: Removed unnecessary cleanups
https://bugs.webkit.org/attachment.cgi?id=64723&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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