Bug 112734 – Refactored script content removal in the fragment parser for clarity and speed

• Component: New Bugs
• Status: RESOLVED FIXED

[Geoffrey Garen]

Refactored script content removal in the fragment parser for clarity and speed

[Geoffrey Garen]

Created attachment 193913 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/pasteboard/paste-noscript-expected.txt', u'LayoutTests/editing/pasteboard/paste-noscript-svg-expected.txt', u'LayoutTests/editing/pasteboard/paste-noscript-xhtml-expected.txt', u'LayoutTests/editing/pasteboard/paste-noscript.html', u'LayoutTests/editing/pasteboard/paste-visible-script-expected.txt', u'LayoutTests/editing/pasteboard/resources/paste-noscript-content.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/dom/DocumentFragment.cpp', u'Source/WebCore/dom/DocumentFragment.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/Element.h', u'Source/WebCore/dom/FragmentScriptingPermission.h', u'Source/WebCore/dom/ScriptableDocumentParser.cpp', u'Source/WebCore/dom/ScriptableDocumentParser.h', u'Source/WebCore/editing/markup.cpp', u'Source/WebCore/editing/markup.h', u'Source/WebCore/html/HTMLAnchorElement.cpp', u'Source/WebCore/html/HTMLBaseElement.cpp', u'Source/WebCore/html/HTMLElement.cpp', u'Source/WebCore/html/HTMLLinkElement.cpp', u'Source/WebCore/html/parser/HTMLConstructionSite.cpp', u'Source/WebCore/html/parser/HTMLConstructionSite.h', u'Source/WebCore/html/parser/HTMLDocumentParser.cpp', u'Source/WebCore/html/parser/HTMLDocumentParser.h', u'Source/WebCore/html/parser/HTMLTreeBuilder.cpp', u'Source/WebCore/html/parser/HTMLTreeBuilder.h', u'Source/WebCore/platform/blackberry/PasteboardBlackBerry.cpp', u'Source/WebCore/platform/chromium/DragDataChromium.cpp', u'Source/WebCore/platform/chromium/PasteboardChromium.cpp', u'Source/WebCore/platform/gtk/PasteboardGtk.cpp', u'Source/WebCore/platform/mac/PasteboardMac.mm', u'Source/WebCore/platform/qt/DragDataQt.cpp', u'Source/WebCore/platform/qt/PasteboardQt.cpp', u'Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp', u'Source/WebCore/platform/wx/PasteboardWx.cpp', u'Source/WebCore/svg/SVGAElement.cpp', u'Source/WebCore/svg/SVGAElement.h', u'Source/WebCore/xml/XMLErrors.cpp', u'Source/WebCore/xml/parser/XMLDocumentParser.cpp', u'Source/WebCore/xml/parser/XMLDocumentParser.h', u'Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp', u'Source/WebCore/xml/parser/XMLDocumentParserQt.cpp']" exit_code: 1
Source/WebCore/dom/FragmentScriptingPermission.h:28:  #ifndef header guard has wrong style, please use: FragmentScriptingPermission_h  [build/header_guard] [5]
Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:708:  nb_namespaces is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:734:  nb_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 44 files


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

[Enrica Casucci]

Comment on attachment 193913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193913&action=review

Looks good to me. This is a great improvement!

> Source/WebCore/dom/Element.cpp:-1034
> -void Element::parserSetAttributes(const Vector<Attribute>& attributeVector, FragmentScriptingPermission scriptingPermission)

This is very clever!

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:-703
> -static inline void handleNamespaceAttributes(Vector<Attribute, 8>& prefixedAttributes, const xmlChar** libxmlNamespaces, int nb_namespaces, ExceptionCode& ec)

Why did you remove the 8?

[Ryosuke Niwa]

Comment on attachment 193913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193913&action=review

> Source/WebCore/dom/Element.cpp:1047
> +    size_t dst = 0;

Please don't use abbreviations like dst.

> Source/WebCore/dom/Element.cpp:1048
> +    for (size_t src = 0; src < attributeVector.size(); ++src) {

or src.

> Source/WebCore/dom/ScriptableDocumentParser.cpp:42
> +    if (!pluginContentIsAllowed(m_parserContentPolicy) && (!document->settings() || document->settings()->unsafePluginPastingEnabled()))
> +        m_parserContentPolicy = allowPluginContent(m_parserContentPolicy);

Why are we not doing this in the caller?

[Geoffrey Garen]

> Looks good to me. This is a great improvement!

Thanks! :)

> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:-703
> > -static inline void handleNamespaceAttributes(Vector<Attribute, 8>& prefixedAttributes, const xmlChar** libxmlNamespaces, int nb_namespaces, ExceptionCode& ec)
> 
> Why did you remove the 8?

I wanted the HTML parser and the XML parser to use the same helper functions, and the HTML parser didn't have the 8. I could have change the HTML parser to include the 8, but I guessed that the HTML parser was the better thing to copy, since it gets more performance testing.

[Geoffrey Garen]

> Please don't use abbreviations like dst.
> or src.

Will fix.

> > +    if (!pluginContentIsAllowed(m_parserContentPolicy) && (!document->settings() || document->settings()->unsafePluginPastingEnabled()))
> > +        m_parserContentPolicy = allowPluginContent(m_parserContentPolicy);
> 
> Why are we not doing this in the caller?

We used to do this in the caller, but I worried it was error prone to make each caller do this check. I thought about this because I'm looking at adding another settings check, and I was especially worried about having to add that check to all the right call sites.

[Build Bot]

Comment on attachment 193913 [details]
Patch

Attachment 193913 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17135444

[Geoffrey Garen]

Committed r146264: <http://trac.webkit.org/changeset/146264>

[Ryosuke Niwa]

Windows build fix landed in http://trac.webkit.org/changeset/146270.