[webkit-changes] [164505] trunk/Source

Fri, 21 Feb 2014 14:42:34 -0800

Title: [164505] trunk/Source
Revision
164505
Author
benja...@webkit.org
Date
2014-02-21 14:47:37 -0800 (Fri, 21 Feb 2014)

Log Message

jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument
https://bugs.webkit.org/show_bug.cgi?id=128893

Reviewed by Darin Adler.

Source/WebCore: 

The declaration of TreeScope::getElementById() was taking an AtomicString as the parameter.
Because of this, all the call sites manipulating String were creating and keeping alive an AtomicString
to make the call.

This had two negative consequences:
-The call sites were ref-ing the ID's atomic string for no reason.
-When there is no ID associated with the input string, an atomic string was created for the sole
 purpose of failing the query. Since IDs are stored as AtomicString, if there is not an existing
 AtomicString for the input, there is no reason to query anything.

* WebCore.exp.in:
* bindings/js/JSDOMBinding.cpp:
(WebCore::findAtomicString): Update this after the rename.

* bindings/scripts/CodeGeneratorObjC.pm:
(GenerateImplementation):
* bindings/scripts/IDLAttributes.txt:
Now that there are two overloads for TreeScope::getElementById(), the conversion from NSString*
is ambiguous. I add the keyword ObjCExplicitAtomicString to force an explicit conversion to AtomicString.

* dom/Document.idl:
* dom/TreeScope.cpp:
(WebCore::TreeScope::getElementById):
When getting an AtomicString, the case of a empty string is not important, use isNull() instead.
When getting a String, get the corresponding AtomicString if any and use that for getting the element.

* dom/TreeScope.h:
* html/FTPDirectoryDocument.cpp:
(WebCore::FTPDirectoryDocumentParser::loadDocumentTemplate):
Solve the ambiguous call.

* svg/SVGAElement.cpp:
(WebCore::SVGAElement::defaultEventHandler):
This is a wonderful candidate for substringSharingImpl. The substring does not survive the call since
the new getElementById never create any AtomicString.

* svg/SVGSVGElement.cpp:
(WebCore::SVGSVGElement::getElementById):
It looks like there are opportunities to get faster here, Ryosuke should have a look.

* svg/SVGSVGElement.h:
* xml/XMLTreeViewer.cpp:
(WebCore::XMLTreeViewer::transformDocumentToTreeView):
Unrelated cleanup: noStyleMessage was useless.

Source/WebKit2: 

* WebProcess/InjectedBundle/InjectedBundle.cpp:
(WebKit::InjectedBundle::pageNumberForElementById): Remove the explicit conversion to use the right overload.

Source/WTF: 

AtomicString::find() is a special case optimized for the _javascript_ bindings. The method can only
be called under specific conditions.
The method is renamed to findStringWithHash().

The new AtomicString::find is generic and does not require any propery on the input.

* wtf/text/AtomicString.cpp:
(WTF::AtomicString::findStringWithHash):
(WTF::AtomicString::findSlowCase):
* wtf/text/AtomicString.h:
(WTF::AtomicString::find):

Modified Paths

Diff

Modified: trunk/Source/WTF/ChangeLog (164504 => 164505)


--- trunk/Source/WTF/ChangeLog	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WTF/ChangeLog	2014-02-21 22:47:37 UTC (rev 164505)
@@ -1,3 +1,22 @@
+2014-02-21  Benjamin Poulain  <benja...@webkit.org>
+
+        jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument
+        https://bugs.webkit.org/show_bug.cgi?id=128893
+
+        Reviewed by Darin Adler.
+
+        AtomicString::find() is a special case optimized for the _javascript_ bindings. The method can only
+        be called under specific conditions.
+        The method is renamed to findStringWithHash().
+
+        The new AtomicString::find is generic and does not require any propery on the input.
+
+        * wtf/text/AtomicString.cpp:
+        (WTF::AtomicString::findStringWithHash):
+        (WTF::AtomicString::findSlowCase):
+        * wtf/text/AtomicString.h:
+        (WTF::AtomicString::find):
+
 2014-02-20  Csaba Osztrogonác  <o...@webkit.org>
 
         Add StackStats sources to cmake and autotools build files

Modified: trunk/Source/WTF/wtf/text/AtomicString.cpp (164504 => 164505)


--- trunk/Source/WTF/wtf/text/AtomicString.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WTF/wtf/text/AtomicString.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -404,20 +404,19 @@
     return stringTable().find<HashAndCharactersTranslator<CharacterType>>(buffer);
 }
 
-AtomicStringImpl* AtomicString::find(const StringImpl* stringImpl)
+AtomicStringImpl* AtomicString::findStringWithHash(const StringImpl& stringImpl)
 {
-    ASSERT(stringImpl);
-    ASSERT(stringImpl->existingHash());
+    ASSERT(stringImpl.existingHash());
 
-    if (!stringImpl->length())
+    if (!stringImpl.length())
         return static_cast<AtomicStringImpl*>(StringImpl::empty());
 
     AtomicStringTableLocker locker;
     HashSet<StringImpl*>::iterator iterator;
-    if (stringImpl->is8Bit())
-        iterator = findString<LChar>(stringImpl);
+    if (stringImpl.is8Bit())
+        iterator = findString<LChar>(&stringImpl);
     else
-        iterator = findString<UChar>(stringImpl);
+        iterator = findString<UChar>(&stringImpl);
     if (iterator == stringTable().end())
         return 0;
     return static_cast<AtomicStringImpl*>(*iterator);
@@ -449,6 +448,18 @@
     return returnValue;
 }
 
+AtomicStringImpl* AtomicString::findSlowCase(StringImpl& string)
+{
+    ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpls should return from the fast case.");
+
+    AtomicStringTableLocker locker;
+    HashSet<StringImpl*>& atomicStringTable = stringTable();
+    auto iterator = atomicStringTable.find(&string);
+    if (iterator != atomicStringTable.end())
+        return static_cast<AtomicStringImpl*>(*iterator);
+    return nullptr;
+}
+
 AtomicString AtomicString::fromUTF8Internal(const char* charactersStart, const char* charactersEnd)
 {
     HashAndUTF8Characters buffer;

Modified: trunk/Source/WTF/wtf/text/AtomicString.h (164504 => 164505)


--- trunk/Source/WTF/wtf/text/AtomicString.h	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WTF/wtf/text/AtomicString.h	2014-02-21 22:47:37 UTC (rev 164505)
@@ -85,7 +85,13 @@
     AtomicString(WTF::HashTableDeletedValueType) : m_string(WTF::HashTableDeletedValue) { }
     bool isHashTableDeletedValue() const { return m_string.isHashTableDeletedValue(); }
 
-    WTF_EXPORT_STRING_API static AtomicStringImpl* find(const StringImpl*);
+    WTF_EXPORT_STRING_API static AtomicStringImpl* findStringWithHash(const StringImpl&);
+    static AtomicStringImpl* find(StringImpl* string)
+    {
+        if (!string || string->isAtomic())
+            return static_cast<AtomicStringImpl*>(string);
+        return findSlowCase(*string);
+    }
 
     operator const String&() const { return m_string; }
     const String& string() const { return m_string; };
@@ -190,6 +196,7 @@
     WTF_EXPORT_STRING_API static PassRefPtr<StringImpl> add(CFStringRef);
 #endif
 
+    WTF_EXPORT_STRING_API static AtomicStringImpl* findSlowCase(StringImpl&);
     WTF_EXPORT_STRING_API static AtomicString fromUTF8Internal(const char*, const char*);
 
 #if !ASSERT_DISABLED

Modified: trunk/Source/WebCore/ChangeLog (164504 => 164505)


--- trunk/Source/WebCore/ChangeLog	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/ChangeLog	2014-02-21 22:47:37 UTC (rev 164505)
@@ -1,3 +1,55 @@
+2014-02-21  Benjamin Poulain  <benja...@webkit.org>
+
+        jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument
+        https://bugs.webkit.org/show_bug.cgi?id=128893
+
+        Reviewed by Darin Adler.
+
+        The declaration of TreeScope::getElementById() was taking an AtomicString as the parameter.
+        Because of this, all the call sites manipulating String were creating and keeping alive an AtomicString
+        to make the call.
+
+        This had two negative consequences:
+        -The call sites were ref-ing the ID's atomic string for no reason.
+        -When there is no ID associated with the input string, an atomic string was created for the sole
+         purpose of failing the query. Since IDs are stored as AtomicString, if there is not an existing
+         AtomicString for the input, there is no reason to query anything.
+
+        * WebCore.exp.in:
+        * bindings/js/JSDOMBinding.cpp:
+        (WebCore::findAtomicString): Update this after the rename.
+
+        * bindings/scripts/CodeGeneratorObjC.pm:
+        (GenerateImplementation):
+        * bindings/scripts/IDLAttributes.txt:
+        Now that there are two overloads for TreeScope::getElementById(), the conversion from NSString*
+        is ambiguous. I add the keyword ObjCExplicitAtomicString to force an explicit conversion to AtomicString.
+
+        * dom/Document.idl:
+        * dom/TreeScope.cpp:
+        (WebCore::TreeScope::getElementById):
+        When getting an AtomicString, the case of a empty string is not important, use isNull() instead.
+        When getting a String, get the corresponding AtomicString if any and use that for getting the element.
+
+        * dom/TreeScope.h:
+        * html/FTPDirectoryDocument.cpp:
+        (WebCore::FTPDirectoryDocumentParser::loadDocumentTemplate):
+        Solve the ambiguous call.
+
+        * svg/SVGAElement.cpp:
+        (WebCore::SVGAElement::defaultEventHandler):
+        This is a wonderful candidate for substringSharingImpl. The substring does not survive the call since
+        the new getElementById never create any AtomicString.
+
+        * svg/SVGSVGElement.cpp:
+        (WebCore::SVGSVGElement::getElementById):
+        It looks like there are opportunities to get faster here, Ryosuke should have a look.
+
+        * svg/SVGSVGElement.h:
+        * xml/XMLTreeViewer.cpp:
+        (WebCore::XMLTreeViewer::transformDocumentToTreeView):
+        Unrelated cleanup: noStyleMessage was useless.
+
 2014-02-21  Daniel Bates  <daba...@apple.com>
 
         COL element in table has 0 for offsetWidth

Modified: trunk/Source/WebCore/WebCore.exp.in (164504 => 164505)


--- trunk/Source/WebCore/WebCore.exp.in	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/WebCore.exp.in	2014-02-21 22:47:37 UTC (rev 164505)
@@ -1876,7 +1876,7 @@
 __ZNK7WebCore9PageCache10frameCountEv
 __ZNK7WebCore9RenderBox11clientWidthEv
 __ZNK7WebCore9RenderBox12clientHeightEv
-__ZNK7WebCore9TreeScope14getElementByIdERKN3WTF12AtomicStringE
+__ZNK7WebCore9TreeScope14getElementByIdERKN3WTF6StringE
 __ZTVN7WebCore12ChromeClientE
 __ZTVN7WebCore14LoaderStrategyE
 __ZTVN7WebCore14StaticNodeListE

Modified: trunk/Source/WebCore/bindings/js/JSDOMBinding.cpp (164504 => 164505)


--- trunk/Source/WebCore/bindings/js/JSDOMBinding.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/bindings/js/JSDOMBinding.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -108,7 +108,7 @@
     if (!impl)
         return 0;
     ASSERT(impl->existingHash());
-    return AtomicString::find(impl);
+    return AtomicString::findStringWithHash(*impl);
 }
 
 String valueToStringWithNullCheck(ExecState* exec, JSValue value)

Modified: trunk/Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm (164504 => 164505)


--- trunk/Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm	2014-02-21 22:47:37 UTC (rev 164505)
@@ -1474,7 +1474,12 @@
                 AddIncludesForType($param->type);
 
                 my $idlType = $param->type;
-                my $implGetter = GetObjCTypeGetter($paramName, $idlType);
+                my $implGetter;
+                if ($param->extendedAttributes->{"ObjCExplicitAtomicString"}) {
+                    $implGetter = "AtomicString($paramName)"
+                } else {
+                    $implGetter = GetObjCTypeGetter($paramName, $idlType);
+                }
 
                 push(@parameterNames, $implGetter);
                 $needsCustom{"XPathNSResolver"} = $paramName if $idlType eq "XPathNSResolver";

Modified: trunk/Source/WebCore/bindings/scripts/IDLAttributes.txt (164504 => 164505)


--- trunk/Source/WebCore/bindings/scripts/IDLAttributes.txt	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/bindings/scripts/IDLAttributes.txt	2014-02-21 22:47:37 UTC (rev 164505)
@@ -88,6 +88,7 @@
 NotEnumerable
 NotDeletable
 ObjCCustomImplementation
+ObjCExplicitAtomicString
 ObjCLegacyUnnamedParameters
 ObjCPolymorphic
 ObjCProtocol

Modified: trunk/Source/WebCore/dom/Document.idl (164504 => 164505)


--- trunk/Source/WebCore/dom/Document.idl	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/dom/Document.idl	2014-02-21 22:47:37 UTC (rev 164505)
@@ -51,7 +51,7 @@
                                                                           [TreatNullAs=NullString,Default=Undefined] optional DOMString qualifiedName);
     [ObjCLegacyUnnamedParameters] NodeList getElementsByTagNameNS([TreatNullAs=NullString,Default=Undefined] optional DOMString namespaceURI,
                                                    [Default=Undefined] optional DOMString localName);
-    Element getElementById([Default=Undefined] optional DOMString elementId);
+    Element getElementById([Default=Undefined,ObjCExplicitAtomicString] optional DOMString elementId);
 
     // DOM Level 3 Core

Modified: trunk/Source/WebCore/dom/TreeScope.cpp (164504 => 164505)


--- trunk/Source/WebCore/dom/TreeScope.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/dom/TreeScope.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -102,13 +102,24 @@
 
 Element* TreeScope::getElementById(const AtomicString& elementId) const
 {
-    if (elementId.isEmpty())
+    if (elementId.isNull())
         return nullptr;
     if (!m_elementsById)
         return nullptr;
     return m_elementsById->getElementById(*elementId.impl(), *this);
 }
 
+Element* TreeScope::getElementById(const String& elementId) const
+{
+    if (!m_elementsById)
+        return nullptr;
+
+    if (AtomicStringImpl* atomicElementId = AtomicString::find(elementId.impl()))
+        return m_elementsById->getElementById(*atomicElementId, *this);
+
+    return nullptr;
+}
+
 const Vector<Element*>* TreeScope::getAllElementsById(const AtomicString& elementId) const
 {
     if (elementId.isEmpty())

Modified: trunk/Source/WebCore/dom/TreeScope.h (164504 => 164505)


--- trunk/Source/WebCore/dom/TreeScope.h	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/dom/TreeScope.h	2014-02-21 22:47:37 UTC (rev 164505)
@@ -55,6 +55,7 @@
 
     Element* focusedElement();
     Element* getElementById(const AtomicString&) const;
+    Element* getElementById(const String&) const;
     const Vector<Element*>* getAllElementsById(const AtomicString&) const;
     bool hasElementWithId(const AtomicStringImpl&) const;
     bool containsMultipleElementsWithId(const AtomicString& id) const;

Modified: trunk/Source/WebCore/html/FTPDirectoryDocument.cpp (164504 => 164505)


--- trunk/Source/WebCore/html/FTPDirectoryDocument.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/html/FTPDirectoryDocument.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -297,7 +297,7 @@
 
     HTMLDocumentParser::insert(String(templateDocumentData->data(), templateDocumentData->size()));
 
-    RefPtr<Element> tableElement = document()->getElementById("ftpDirectoryTable");
+    RefPtr<Element> tableElement = document()->getElementById(String(ASCIILiteral("ftpDirectoryTable")));
     if (!tableElement)
         LOG_ERROR("Unable to find element by id \"ftpDirectoryTable\" in the template document.");
     else if (!isHTMLTableElement(tableElement.get()))

Modified: trunk/Source/WebCore/svg/SVGAElement.cpp (164504 => 164505)


--- trunk/Source/WebCore/svg/SVGAElement.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/svg/SVGAElement.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -158,7 +158,7 @@
             String url = ""
 
             if (url[0] == '#') {
-                Element* targetElement = treeScope().getElementById(url.substring(1));
+                Element* targetElement = treeScope().getElementById(url.substringSharingImpl(1));
                 if (targetElement && isSVGSMILElement(*targetElement)) {
                     toSVGSMILElement(*targetElement).beginByLinkActivation();
                     event->setDefaultHandled();

Modified: trunk/Source/WebCore/svg/SVGSVGElement.cpp (164504 => 164505)


--- trunk/Source/WebCore/svg/SVGSVGElement.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/svg/SVGSVGElement.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -765,12 +765,13 @@
 
 // getElementById on SVGSVGElement is restricted to only the child subtree defined by the <svg> element.
 // See http://www.w3.org/TR/SVG11/struct.html#InterfaceSVGSVGElement
-Element* SVGSVGElement::getElementById(const AtomicString& id)
+Element* SVGSVGElement::getElementById(const String& id)
 {
     Element* element = treeScope().getElementById(id);
     if (element && element->isDescendantOf(this))
         return element;
 
+    // FIXME: This should use treeScope().getAllElementsById.
     // Fall back to traversing our subtree. Duplicate ids are allowed, the first found will
     // be returned.
     for (auto& element : descendantsOfType<Element>(*this)) {

Modified: trunk/Source/WebCore/svg/SVGSVGElement.h (164504 => 164505)


--- trunk/Source/WebCore/svg/SVGSVGElement.h	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/svg/SVGSVGElement.h	2014-02-21 22:47:37 UTC (rev 164505)
@@ -122,7 +122,7 @@
 
     void setupInitialView(const String& fragmentIdentifier, Element* anchorNode);
 
-    Element* getElementById(const AtomicString&);
+    Element* getElementById(const String&);
 
     bool widthAttributeEstablishesViewport() const;
     bool heightAttributeEstablishesViewport() const;

Modified: trunk/Source/WebCore/xml/XMLTreeViewer.cpp (164504 => 164505)


--- trunk/Source/WebCore/xml/XMLTreeViewer.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebCore/xml/XMLTreeViewer.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -57,12 +57,11 @@
 
     String scriptString = StringImpl::createWithoutCopying(XMLViewer_js, sizeof(XMLViewer_js));
     m_document.frame()->script().evaluate(ScriptSourceCode(scriptString));
-    String noStyleMessage("This XML file does not appear to have any style information associated with it. The document tree is shown below.");
-    m_document.frame()->script().evaluate(ScriptSourceCode("prepareWebKitXMLViewer('" + noStyleMessage + "');"));
+    m_document.frame()->script().evaluate(ScriptSourceCode(AtomicString("prepareWebKitXMLViewer('This XML file does not appear to have any style information associated with it. The document tree is shown below.');")));
 
     String cssString = StringImpl::createWithoutCopying(XMLViewer_css, sizeof(XMLViewer_css));
     RefPtr<Text> text = m_document.createTextNode(cssString);
-    m_document.getElementById("xml-viewer-style")->appendChild(text, IGNORE_EXCEPTION);
+    m_document.getElementById(String(ASCIILiteral("xml-viewer-style")))->appendChild(text, IGNORE_EXCEPTION);
     m_document.styleResolverChanged(RecalcStyleImmediately);
 }

Modified: trunk/Source/WebKit2/ChangeLog (164504 => 164505)


--- trunk/Source/WebKit2/ChangeLog	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebKit2/ChangeLog	2014-02-21 22:47:37 UTC (rev 164505)
@@ -1,3 +1,13 @@
+2014-02-21  Benjamin Poulain  <benja...@webkit.org>
+
+        jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument
+        https://bugs.webkit.org/show_bug.cgi?id=128893
+
+        Reviewed by Darin Adler.
+
+        * WebProcess/InjectedBundle/InjectedBundle.cpp:
+        (WebKit::InjectedBundle::pageNumberForElementById): Remove the explicit conversion to use the right overload.
+
 2014-02-21  Enrica Casucci  <enr...@apple.com>
 
         Support WebSelections in WK2 on iOS.

Modified: trunk/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp (164504 => 164505)


--- trunk/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp	2014-02-21 22:38:12 UTC (rev 164504)
+++ trunk/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp	2014-02-21 22:47:37 UTC (rev 164505)
@@ -446,7 +446,7 @@
     if (!coreFrame)
         return -1;
 
-    Element* element = coreFrame->document()->getElementById(AtomicString(id));
+    Element* element = coreFrame->document()->getElementById(id);
     if (!element)
         return -1;
 

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to