JSHTMLFormElement::canGetItemsForName needlessly allocates a Vector
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Aug 2013 06:05:05 +0000 (06:05 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Aug 2013 06:05:05 +0000 (06:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=120277

Reviewed by Sam Weinig.

Added HTMLFormElement::hasNamedElement and used it in JSHTMLFormElement::canGetItemsForName.

This required fixing a bug in HTMLFormElement::getNamedElements that the first call to getNamedElements
after replacing an element A with another element B of the same name caused it to erroneously append A
to namedItems via the aliases mapping. Because getNamedElements used to be always called in pairs, this
wrong behavior was never visible to the Web. Fixed the bug by not adding the old element to namedItem
when namedItem's size is 1.

Also renamed m_elementAliases to m_pastNamesMap along with related member functions.

No new tests are added since there should be no Web exposed behavioral change.

* bindings/js/JSHTMLFormElementCustom.cpp:
(WebCore::JSHTMLFormElement::canGetItemsForName):
* html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::elementFromPastNamesMap):
(WebCore::HTMLFormElement::addElementToPastNamesMap):
(WebCore::HTMLFormElement::hasNamedElement):
(WebCore::HTMLFormElement::getNamedElements):
* html/HTMLFormElement.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@154586 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSHTMLFormElementCustom.cpp
Source/WebCore/html/HTMLFormElement.cpp
Source/WebCore/html/HTMLFormElement.h

index 71909dd27083bf5c8f6b267674d55e9ba0aaec36..d777a47ff7c779563ab14dcb650ce0dda3c30afb 100644 (file)
@@ -1,3 +1,31 @@
+2013-08-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        JSHTMLFormElement::canGetItemsForName needlessly allocates a Vector
+        https://bugs.webkit.org/show_bug.cgi?id=120277
+
+        Reviewed by Sam Weinig.
+
+        Added HTMLFormElement::hasNamedElement and used it in JSHTMLFormElement::canGetItemsForName.
+
+        This required fixing a bug in HTMLFormElement::getNamedElements that the first call to getNamedElements
+        after replacing an element A with another element B of the same name caused it to erroneously append A
+        to namedItems via the aliases mapping. Because getNamedElements used to be always called in pairs, this
+        wrong behavior was never visible to the Web. Fixed the bug by not adding the old element to namedItem
+        when namedItem's size is 1.
+
+        Also renamed m_elementAliases to m_pastNamesMap along with related member functions.
+
+        No new tests are added since there should be no Web exposed behavioral change.
+
+        * bindings/js/JSHTMLFormElementCustom.cpp:
+        (WebCore::JSHTMLFormElement::canGetItemsForName):
+        * html/HTMLFormElement.cpp:
+        (WebCore::HTMLFormElement::elementFromPastNamesMap):
+        (WebCore::HTMLFormElement::addElementToPastNamesMap):
+        (WebCore::HTMLFormElement::hasNamedElement):
+        (WebCore::HTMLFormElement::getNamedElements):
+        * html/HTMLFormElement.h:
+
 2013-08-25  Andreas Kling  <akling@apple.com>
 
         RenderLayerBacking::renderer() should return a reference.
index d565e5c1fb40438e9589f260a4ff7f52383018d7..18662cf6f401a7ad240bf2ec795cf09fb097ff5c 100644 (file)
@@ -39,9 +39,7 @@ namespace WebCore {
 
 bool JSHTMLFormElement::canGetItemsForName(ExecState*, HTMLFormElement* form, PropertyName propertyName)
 {
-    Vector<RefPtr<Node> > namedItems;
-    form->getNamedElements(propertyNameToAtomicString(propertyName), namedItems);
-    return namedItems.size();
+    return form->hasNamedElement(propertyNameToAtomicString(propertyName));
 }
 
 JSValue JSHTMLFormElement::nameGetter(ExecState* exec, JSValue slotBase, PropertyName propertyName)
index b664d709e14afa56c2221ca2c331a73118c1c959..538a26649ccf1734a740e3165a5a30fa1813448e 100644 (file)
@@ -611,36 +611,38 @@ bool HTMLFormElement::checkInvalidControlsAndCollectUnhandled(Vector<RefPtr<Form
     return hasInvalidControls;
 }
 
-HTMLFormControlElement* HTMLFormElement::elementForAlias(const AtomicString& alias)
+HTMLFormControlElement* HTMLFormElement::elementFromPastNamesMap(const AtomicString& pastName) const
 {
-    if (alias.isEmpty() || !m_elementAliases)
+    if (pastName.isEmpty() || !m_pastNamesMap)
         return 0;
-    return m_elementAliases->get(alias.impl());
+    return m_pastNamesMap->get(pastName.impl());
 }
 
-void HTMLFormElement::addElementAlias(HTMLFormControlElement* element, const AtomicString& alias)
+void HTMLFormElement::addElementToPastNamesMap(HTMLFormControlElement* element, const AtomicString& pastName)
 {
-    if (alias.isEmpty())
+    if (pastName.isEmpty())
         return;
-    if (!m_elementAliases)
-        m_elementAliases = adoptPtr(new AliasMap);
-    m_elementAliases->set(alias.impl(), element);
+    if (!m_pastNamesMap)
+        m_pastNamesMap = adoptPtr(new PastNamesMap);
+    m_pastNamesMap->set(pastName.impl(), element);
+}
+
+bool HTMLFormElement::hasNamedElement(const AtomicString& name)
+{
+    return elements()->hasNamedItem(name) || elementFromPastNamesMap(name);
 }
 
 void HTMLFormElement::getNamedElements(const AtomicString& name, Vector<RefPtr<Node> >& namedItems)
 {
+    // http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-form-nameditem
     elements()->namedItems(name, namedItems);
 
-    HTMLFormControlElement* aliasElement = elementForAlias(name);
-    if (aliasElement) {
-        if (namedItems.find(aliasElement) == notFound) {
-            // We have seen it before but it is gone now. Still, we need to return it.
-            // FIXME: The above comment is not clear enough; it does not say why we need to do this.
-            namedItems.append(aliasElement);
-        }
-    }
-    if (namedItems.size() && namedItems.first() != aliasElement)
-        addElementAlias(static_cast<HTMLFormControlElement*>(namedItems.first().get()), name);
+    // FIXME: The specification says we should not add the element from the past when names map when namedItems is not empty.
+    HTMLFormControlElement* elementFromPast = elementFromPastNamesMap(name);
+    if (namedItems.size() && namedItems.first() != elementFromPast)
+        addElementToPastNamesMap(static_cast<HTMLFormControlElement*>(namedItems.first().get()), name);
+    else if (elementFromPast && namedItems.find(elementFromPast) == notFound)
+        namedItems.append(elementFromPast);
 }
 
 void HTMLFormElement::documentDidResumeFromPageCache()
index 3f21dee3aaaf0c4502150aeb096c2e72ffcc31b5..468302886977926954c8487976d2c7fac29ba761 100644 (file)
@@ -47,6 +47,7 @@ public:
     virtual ~HTMLFormElement();
 
     PassRefPtr<HTMLCollection> elements();
+    bool hasNamedElement(const AtomicString&);
     void getNamedElements(const AtomicString&, Vector<RefPtr<Node> >&);
 
     unsigned length() const;
@@ -98,9 +99,6 @@ public:
 
     bool checkValidity();
 
-    HTMLFormControlElement* elementForAlias(const AtomicString&);
-    void addElementAlias(HTMLFormControlElement*, const AtomicString& alias);
-
     CheckedRadioButtons& checkedRadioButtons() { return m_checkedRadioButtons; }
 
     const Vector<FormAssociatedElement*>& associatedElements() const { return m_associatedElements; }
@@ -140,10 +138,14 @@ private:
     // are any invalid controls in this form.
     bool checkInvalidControlsAndCollectUnhandled(Vector<RefPtr<FormAssociatedElement> >&);
 
-    typedef HashMap<RefPtr<AtomicStringImpl>, RefPtr<HTMLFormControlElement> > AliasMap;
+    HTMLFormControlElement* elementFromPastNamesMap(const AtomicString&) const;
+    void addElementToPastNamesMap(HTMLFormControlElement*, const AtomicString& pastName);
+
+    // FIXME: This can leak HTMLFormControlElements.
+    typedef HashMap<RefPtr<AtomicStringImpl>, RefPtr<HTMLFormControlElement> > PastNamesMap;
 
     FormSubmission::Attributes m_attributes;
-    OwnPtr<AliasMap> m_elementAliases;
+    OwnPtr<PastNamesMap> m_pastNamesMap;
 
     CheckedRadioButtons m_checkedRadioButtons;