Bug 122403 – CTTE: Thread references through markup.h

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

CTTE: Thread references through markup.h

[Sam Weinig]

Created attachment 213491 [details]
Patch

[EFL EWS Bot]

Comment on attachment 213491 [details]
Patch

Attachment 213491 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3469037

[Sam Weinig]

Created attachment 213492 [details]
Patch

[Andreas Kling]

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:595
>      RefPtr<Range> selectionRange = frame->selection().toNormalizedRange();
> +    if (!selectionRange)
> +        return nullptr;

Looks like we would previously continue on if this failed, creating doctype-only markup below.

> Source/WebCore/page/PageSerializer.cpp:110
> +    virtual void appendText(StringBuilder&, const Text&) OVERRIDE;
> +    virtual void appendElement(StringBuilder&, const Element&, Namespaces*) OVERRIDE;
> +    virtual void appendCustomAttributes(StringBuilder&, const Element&, Namespaces*) OVERRIDE;
> +    virtual void appendEndTag(const Node&) OVERRIDE;

Anders says we can do override without SHOUTING now!

> Source/WebKit/mac/WebView/WebFrame.mm:754
> +    size_t size = nodesVector.size();
> +    for (size_t i = 0; i < size; ++i) {

I'd put the size in the for scope together with i.

[EFL EWS Bot]

Comment on attachment 213492 [details]
Patch

Attachment 213492 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3486002

[EFL EWS Bot]

Comment on attachment 213492 [details]
Patch

Attachment 213492 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3483006

[Sam Weinig]

Created attachment 213493 [details]
Patch

[WebKit Commit Bot]

Attachment 213493 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/objc/DOMHTML.mm', u'Source/WebCore/dom/CDATASection.h', u'Source/WebCore/dom/Comment.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/DocumentType.h', u'Source/WebCore/dom/ProcessingInstruction.h', u'Source/WebCore/dom/Range.cpp', u'Source/WebCore/dom/ShadowRoot.cpp', u'Source/WebCore/dom/Text.h', u'Source/WebCore/editing/CompositeEditCommand.cpp', u'Source/WebCore/editing/EditorCommand.cpp', u'Source/WebCore/editing/MarkupAccumulator.cpp', u'Source/WebCore/editing/MarkupAccumulator.h', u'Source/WebCore/editing/ios/EditorIOS.mm', u'Source/WebCore/editing/mac/EditorMac.mm', u'Source/WebCore/editing/markup.cpp', u'Source/WebCore/editing/markup.h', u'Source/WebCore/html/HTMLElement.cpp', u'Source/WebCore/html/HTMLFrameOwnerElement.h', u'Source/WebCore/html/HTMLTemplateElement.cpp', u'Source/WebCore/html/HTMLTemplateElement.h', u'Source/WebCore/inspector/DOMEditor.cpp', u'Source/WebCore/inspector/DOMEditor.h', u'Source/WebCore/inspector/DOMPatchSupport.cpp', u'Source/WebCore/inspector/DOMPatchSupport.h', u'Source/WebCore/inspector/InspectorDOMAgent.cpp', u'Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/PageSerializer.cpp', u'Source/WebCore/platform/ios/PasteboardIOS.mm', u'Source/WebCore/xml/XMLHttpRequest.cpp', u'Source/WebCore/xml/XMLSerializer.cpp', u'Source/WebCore/xml/XSLTProcessor.cpp', u'Source/WebCore/xml/XSLTProcessor.h', u'Source/WebCore/xml/XSLTProcessorLibxslt.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/DOM/WebDOMOperations.mm', u'Source/WebKit/mac/WebView/WebFrame.mm']" exit_code: 1
Source/WebCore/editing/markup.cpp:132:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/editing/markup.cpp:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 213493 [details]
Patch

r=me

[Sam Weinig]

Created attachment 213495 [details]
Patch

[WebKit Commit Bot]

Attachment 213495 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/objc/DOMHTML.mm', u'Source/WebCore/dom/CDATASection.h', u'Source/WebCore/dom/Comment.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/DocumentType.h', u'Source/WebCore/dom/ProcessingInstruction.h', u'Source/WebCore/dom/Range.cpp', u'Source/WebCore/dom/ShadowRoot.cpp', u'Source/WebCore/dom/Text.h', u'Source/WebCore/editing/CompositeEditCommand.cpp', u'Source/WebCore/editing/EditorCommand.cpp', u'Source/WebCore/editing/MarkupAccumulator.cpp', u'Source/WebCore/editing/MarkupAccumulator.h', u'Source/WebCore/editing/ios/EditorIOS.mm', u'Source/WebCore/editing/mac/EditorMac.mm', u'Source/WebCore/editing/markup.cpp', u'Source/WebCore/editing/markup.h', u'Source/WebCore/html/HTMLElement.cpp', u'Source/WebCore/html/HTMLFrameOwnerElement.h', u'Source/WebCore/html/HTMLTemplateElement.cpp', u'Source/WebCore/html/HTMLTemplateElement.h', u'Source/WebCore/inspector/DOMEditor.cpp', u'Source/WebCore/inspector/DOMEditor.h', u'Source/WebCore/inspector/DOMPatchSupport.cpp', u'Source/WebCore/inspector/DOMPatchSupport.h', u'Source/WebCore/inspector/InspectorDOMAgent.cpp', u'Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/PageSerializer.cpp', u'Source/WebCore/platform/gtk/PasteboardGtk.cpp', u'Source/WebCore/platform/ios/PasteboardIOS.mm', u'Source/WebCore/xml/XMLHttpRequest.cpp', u'Source/WebCore/xml/XMLSerializer.cpp', u'Source/WebCore/xml/XSLTProcessor.cpp', u'Source/WebCore/xml/XSLTProcessor.h', u'Source/WebCore/xml/XSLTProcessorLibxslt.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/DOM/WebDOMOperations.mm', u'Source/WebKit/mac/WebView/WebFrame.mm']" exit_code: 1
Source/WebCore/editing/markup.cpp:132:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/editing/markup.cpp:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 213502 [details]
Patch

[WebKit Commit Bot]

Attachment 213502 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/objc/DOMHTML.mm', u'Source/WebCore/dom/CDATASection.h', u'Source/WebCore/dom/Comment.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/DocumentType.h', u'Source/WebCore/dom/ProcessingInstruction.h', u'Source/WebCore/dom/Range.cpp', u'Source/WebCore/dom/ShadowRoot.cpp', u'Source/WebCore/dom/Text.h', u'Source/WebCore/editing/CompositeEditCommand.cpp', u'Source/WebCore/editing/EditorCommand.cpp', u'Source/WebCore/editing/MarkupAccumulator.cpp', u'Source/WebCore/editing/MarkupAccumulator.h', u'Source/WebCore/editing/ios/EditorIOS.mm', u'Source/WebCore/editing/mac/EditorMac.mm', u'Source/WebCore/editing/markup.cpp', u'Source/WebCore/editing/markup.h', u'Source/WebCore/html/HTMLElement.cpp', u'Source/WebCore/html/HTMLFrameOwnerElement.h', u'Source/WebCore/html/HTMLTemplateElement.cpp', u'Source/WebCore/html/HTMLTemplateElement.h', u'Source/WebCore/inspector/DOMEditor.cpp', u'Source/WebCore/inspector/DOMEditor.h', u'Source/WebCore/inspector/DOMPatchSupport.cpp', u'Source/WebCore/inspector/DOMPatchSupport.h', u'Source/WebCore/inspector/InspectorDOMAgent.cpp', u'Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/PageSerializer.cpp', u'Source/WebCore/platform/gtk/DataObjectGtk.cpp', u'Source/WebCore/platform/gtk/PasteboardGtk.cpp', u'Source/WebCore/platform/ios/PasteboardIOS.mm', u'Source/WebCore/xml/XMLHttpRequest.cpp', u'Source/WebCore/xml/XMLSerializer.cpp', u'Source/WebCore/xml/XSLTProcessor.cpp', u'Source/WebCore/xml/XSLTProcessor.h', u'Source/WebCore/xml/XSLTProcessorLibxslt.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/DOM/WebDOMOperations.mm', u'Source/WebKit/mac/WebView/WebFrame.mm']" exit_code: 1
Source/WebCore/editing/markup.cpp:132:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/editing/markup.cpp:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 213502 [details]
Patch

Attachment 213502 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3515021

[Darin Adler]

Comment on attachment 213502 [details]
Patch

../../Source/WebCore/platform/gtk/DragDataGtk.cpp:100:88: error: invalid initialization of non-const reference of type 'WebCore::Document&' from an rvalue of type 'WebCore::Document*'
../../Source/WebCore/platform/gtk/PasteboardGtk.cpp:324:104: error: cannot convert 'WebCore::Range' to 'WebCore::Range*' for argument '1' to 'WTF::PassRefPtr<WebCore::DocumentFragment> WebCore::createFragmentFromText(WebCore::Range*, const WTF::String&)'

[Darin Adler]

Comment on attachment 213502 [details]
Patch

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*>(&element));
> +            newInlineStyle->removeStyleConflictingWithStyleOfNode(const_cast<Element*>(&element));

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.

[Sam Weinig]

Landed in r157002.

[Sam Weinig]

(In reply to comment #16)
> (From update of attachment 213502 [details])
> 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.

Done.

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

Done.

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

The consts all suck.  I think I am going to rip them all out on a second pass.

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

Done.

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

Converted to one function with a default value.

> 
> > 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;

Yup. Done.

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

Done.

> 
> > Source/WebCore/editing/markup.cpp:315
> > +            newInlineStyle->removePropertiesInElementDefaultStyle(const_cast<Element*>(&element));
> > +            newInlineStyle->removeStyleConflictingWithStyleOfNode(const_cast<Element*>(&element));
> 
> 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?

Done.

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

Didn't do.  Too scared.

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

Yes. Done.

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

I'm trying to avoid operator+ for strings.  This should probably use createFullMarkup if it was extended to support the other arguments.