[webkit-reviews] review granted: [Bug 122403] CTTE: Thread references through markup.h : [Attachment 213502] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 122403: CTTE: Thread references through markup.h
https://bugs.webkit.org/show_bug.cgi?id=122403

Attachment 213502: Patch
https://bugs.webkit.org/attachment.cgi?id=213502&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213502&action=review


Gotta love this patch.

Obviously, must get GTK building before landing (maybe Windows too).

> Source/WebCore/dom/CDATASection.h:69
> +inline CDATASection& toCDATASection(Node& node)
> +{
> +    ASSERT_WITH_SECURITY_IMPLICATION(node.nodeType() ==
Node::CDATA_SECTION_NODE);
> +    return static_cast<CDATASection&>(node);
> +}
> +
> +inline const CDATASection& toCDATASection(const Node& node)
> +{
> +    ASSERT_WITH_SECURITY_IMPLICATION(node.nodeType() ==
Node::CDATA_SECTION_NODE);
> +    return static_cast<const CDATASection&>(node);
> +}
> +
> +inline CDATASection* toCDATASection(Node* node)
> +{
> +    ASSERT_WITH_SECURITY_IMPLICATION(!node || node->nodeType() ==
Node::CDATA_SECTION_NODE);
> +    return static_cast<CDATASection*>(node);
> +}
> +
> +inline const CDATASection* toCDATASection(const Node* node)
> +{
> +    ASSERT_WITH_SECURITY_IMPLICATION(!node || node->nodeType() ==
Node::CDATA_SECTION_NODE);
> +    return static_cast<const CDATASection*>(node);
> +}
> +
> +void toCDATASection(const CDATASection&);
> +void toCDATASection(const CDATASection*);

Sure would be nice to do these with macros.

> Source/WebCore/editing/MarkupAccumulator.cpp:137
>      if (tagNamesToSkip) {
>	   for (size_t i = 0; i < tagNamesToSkip->size(); ++i) {
> -	       if (targetNode->hasTagName(tagNamesToSkip->at(i)))
> +	       if (targetNode.hasTagName(tagNamesToSkip->at(i)))
>		   return;
>	   }
>      }

A waste to do this loop if the node isn’t an element.

> Source/WebCore/editing/MarkupAccumulator.cpp:192
> -void MarkupAccumulator::appendStartTag(Node* node, Namespaces* namespaces)
> +void MarkupAccumulator::appendStartTag(const Node& node, Namespaces*
namespaces)
>  {
>      appendStartMarkup(m_markup, node, namespaces);
>      if (m_nodes)
> -	   m_nodes->append(node);
> +	   m_nodes->append(const_cast<Node*>(&node));
>  }

Seems like const is not so great if you just have to const_cast it away. Is the
const really valuable?

> Source/WebCore/editing/MarkupAccumulator.cpp:340
> +	   parentName = &(text.parentElement())->tagQName();

The parentheses here make the expression confusing. It’s like the & only
applies to the first part of the expression or something?

> Source/WebCore/editing/MarkupAccumulator.h:73
> +    String serializeNodes(Node& targetNode, Node* nodeToSkip,
EChildrenOnly);
> +    String serializeNodes(Node& targetNode, Node* nodeToSkip, EChildrenOnly,
Vector<QualifiedName>* tagNamesToSkip);

I think we should either use a default value of nullptr instead of overloading,
or we should use a & instead of a * for the tag names to skip.

> Source/WebCore/editing/markup.cpp:134
> +    virtual void appendString(const String& string) override
> +    {
> +	   MarkupAccumulator::appendString(string);
> +    }

This override doesn’t look right to me. I think this should instead be:

    using MarkupAccumulator::appendString;

> Source/WebCore/editing/markup.cpp:285
> +    String str = node.nodeValue();

I say we splurge on a few extra letters and call this either "value" or
"string".

> Source/WebCore/editing/markup.cpp:315
> +	      
newInlineStyle->removePropertiesInElementDefaultStyle(const_cast<Element*>(&ele
ment));
> +	      
newInlineStyle->removeStyleConflictingWithStyleOfNode(const_cast<Element*>(&ele
ment));

Hmmpf, const.

> Source/WebCore/editing/markup.cpp:320
> +	   if (element.isStyledElement() && static_cast<const
StyledElement&>(element).inlineStyle())
> +	       newInlineStyle->overrideWithStyle(static_cast<const
StyledElement&>(element).inlineStyle());

toStyledElement?

> Source/WebCore/editing/markup.cpp:829
> +    if (!documentType)
> +	   return emptyString();

I know the old behavior was to return the empty string, but I would be tempted
to return null string instead if I was supra callers would be OK with it.

> Source/WebCore/editing/mac/EditorMac.mm:517
> -    fragment = createFragmentFromMarkup(frame.document(),
stringOmittingMicrosoftPrefix, emptyString(),
DisallowScriptingAndPluginContent);
> +    fragment = createFragmentFromMarkup(*frame.document(),
stringOmittingMicrosoftPrefix, emptyString(),
DisallowScriptingAndPluginContent);

The other functions above seem to check frame.document() for null, but tis one
does not. Should it?

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:594
> +    StringBuilder builder;

I don’t think StringBuilder is superior for concatenating two strings. If we
could get the markup code to build the string in the builder, that would be
another story.