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