[webkit-reviews] review granted: [Bug 122403] CTTE: Thread references through markup.h : [Attachment 213502] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Oct 6 09:58:29 PDT 2013
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.
More information about the webkit-reviews
mailing list