Bug 122403

Summary: CTTE: Thread references through markup.h
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eflews.bot, gtk-ews, gyuyoung.kim, kling, philn, rego+ews, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
eflews.bot: commit-queue-
Patch
none
Patch
none
Patch none

Description Sam Weinig 2013-10-05 21:16:47 PDT 
CTTE: Thread references through markup.h
Comment 1 Sam Weinig 2013-10-05 21:19:21 PDT 
Created attachment 213491 [details]
Patch
Comment 2 EFL EWS Bot 2013-10-05 21:27:24 PDT 
Comment on attachment 213491 [details]
Patch

Attachment 213491 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3469037
Comment 3 Sam Weinig 2013-10-05 21:36:00 PDT 
Created attachment 213492 [details]
Patch
Comment 4 Andreas Kling 2013-10-05 21:36:18 PDT 
> 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.
Comment 5 EFL EWS Bot 2013-10-05 21:44:18 PDT 
Comment on attachment 213492 [details]
Patch

Attachment 213492 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3486002
Comment 6 EFL EWS Bot 2013-10-05 21:46:53 PDT 
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
Comment 7 Sam Weinig 2013-10-05 21:51:01 PDT 
Created attachment 213493 [details]
Patch
Comment 8 WebKit Commit Bot 2013-10-05 21:52:19 PDT 
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.
Comment 9 Andreas Kling 2013-10-05 21:52:49 PDT 
Comment on attachment 213493 [details]
Patch

r=me
Comment 10 Sam Weinig 2013-10-05 22:33:57 PDT 
Created attachment 213495 [details]
Patch
Comment 11 WebKit Commit Bot 2013-10-05 22:36:32 PDT 
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.
Comment 12 Sam Weinig 2013-10-06 00:35:48 PDT 
Created attachment 213502 [details]
Patch
Comment 13 WebKit Commit Bot 2013-10-06 00:37:16 PDT 
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.
Comment 14 kov's GTK+ EWS bot 2013-10-06 01:30:18 PDT 
Comment on attachment 213502 [details]
Patch

Attachment 213502 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3515021
Comment 15 Darin Adler 2013-10-06 09:43:01 PDT 
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&)'
Comment 16 Darin Adler 2013-10-06 09:57:22 PDT 
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.
Comment 17 Sam Weinig 2013-10-06 12:17:39 PDT 
Landed in r157002.
Comment 18 Sam Weinig 2013-10-06 12:20:31 PDT 
(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.