Get rid of firstItem and nextItem from HTMLCollection

https://bugs.webkit.org/show_bug.cgi?id=89923

Reviewed by Andreas Kling.

Source/WebCore:

Removed HTMLCollection::firstItem and HTMLCollection::nextItem.
Also added hasAnyItem() and hasExactlyOneItem() to HTMLCollection so that named getter on Document
doesn't need to compute the full length before returning a HTMLCollection.

* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::getDocumentLinks):
* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::namedItemGetter):
* bindings/js/JSHTMLDocumentCustom.cpp:
(WebCore::JSHTMLDocument::nameGetter):
* bindings/v8/custom/V8DOMWindowCustom.cpp:
(WebCore::V8DOMWindow::namedPropertyGetter):
* bindings/v8/custom/V8HTMLDocumentCustom.cpp:
(WebCore::V8HTMLDocument::GetNamedProperty):
* dom/Document.cpp:
(WebCore::Document::openSearchDescriptionURL):
* html/HTMLCollection.cpp:
(WebCore::shouldIncludeChildren):
(WebCore::HTMLCollection::HTMLCollection):
(WebCore):
(WebCore::HTMLCollection::item):
* html/HTMLCollection.h:
(HTMLCollection):
(WebCore::HTMLCollection::hasAnyItem):
(WebCore::HTMLCollection::hasExactlyOneItem):
* html/HTMLMapElement.cpp:
(WebCore::HTMLMapElement::imageElement):

Source/WebKit/chromium:

Re-implement WebNodeCollection::firstItem() and WebNodeCollection::nextItem() in WebKit code
because we got rid of it from WebCore implementation.

This is an extremely poor API and we shouldn't be exposing it in the future.

* public/WebNodeCollection.h:
(WebKit::WebNodeCollection::WebNodeCollection):
(WebNodeCollection):
* src/WebNodeCollection.cpp:
(WebKit::WebNodeCollection::nextItem):
(WebKit::WebNodeCollection::firstItem):
* src/WebPageSerializerImpl.cpp:
(WebKit::WebPageSerializerImpl::collectTargetFrames):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 7a14020..9de02fd 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,38 @@
+2012-06-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Get rid of firstItem and nextItem from HTMLCollection
+        https://bugs.webkit.org/show_bug.cgi?id=89923
+
+        Reviewed by Andreas Kling.
+
+        Removed HTMLCollection::firstItem and HTMLCollection::nextItem.
+        Also added hasAnyItem() and hasExactlyOneItem() to HTMLCollection so that named getter on Document
+        doesn't need to compute the full length before returning a HTMLCollection.
+
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::getDocumentLinks):
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::namedItemGetter):
+        * bindings/js/JSHTMLDocumentCustom.cpp:
+        (WebCore::JSHTMLDocument::nameGetter):
+        * bindings/v8/custom/V8DOMWindowCustom.cpp:
+        (WebCore::V8DOMWindow::namedPropertyGetter):
+        * bindings/v8/custom/V8HTMLDocumentCustom.cpp:
+        (WebCore::V8HTMLDocument::GetNamedProperty):
+        * dom/Document.cpp:
+        (WebCore::Document::openSearchDescriptionURL):
+        * html/HTMLCollection.cpp:
+        (WebCore::shouldIncludeChildren):
+        (WebCore::HTMLCollection::HTMLCollection):
+        (WebCore):
+        (WebCore::HTMLCollection::item):
+        * html/HTMLCollection.h:
+        (HTMLCollection):
+        (WebCore::HTMLCollection::hasAnyItem):
+        (WebCore::HTMLCollection::hasExactlyOneItem):
+        * html/HTMLMapElement.cpp:
+        (WebCore::HTMLMapElement::imageElement):
+
 2012-06-25  Pratik Solanki  <psolanki@apple.com>
 
         JavaScript resources have low priority when SVG is enabled

diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
index ba41d21..fdb8d02 100644 (file)
--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -2424,9 +2424,8 @@ AccessibilityObject* AccessibilityRenderObject::accessibilityParentForImageMap(H
 void AccessibilityRenderObject::getDocumentLinks(AccessibilityChildrenVector& result)
 {
     Document* document = m_renderer->document();
-    HTMLCollection* coll = document->links();
-    Node* curr = coll->firstItem();
-    while (curr) {
+    HTMLCollection* links = document->links();
+    for (unsigned i = 0; Node* curr = links->item(i); i++) {
         RenderObject* obj = curr->renderer();
         if (obj) {
             RefPtr<AccessibilityObject> axobj = document->axObjectCache()->getOrCreate(obj);
@@ -2444,7 +2443,6 @@ void AccessibilityRenderObject::getDocumentLinks(AccessibilityChildrenVector& re
                 result.append(areaObject);
             }
         }
-        curr = coll->nextItem();
     }
 }
 

diff --git a/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp b/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
index 3796847..2d17465 100644 (file)
--- a/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
+++ b/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
@@ -117,8 +117,8 @@ static JSValue namedItemGetter(ExecState* exec, JSValue slotBase, PropertyName p
     ASSERT(document->isHTMLDocument());
 
     HTMLCollection* collection = document->windowNamedItems(propertyNameToAtomicString(propertyName));
-    if (collection->length() == 1)
-        return toJS(exec, thisObj, collection->firstItem());
+    if (collection->hasExactlyOneItem())
+        return toJS(exec, thisObj, collection->item(0));
     return toJS(exec, thisObj, collection);
 }
 

diff --git a/Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp b/Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp
index 14bc0f0..fddc5a5 100644 (file)
--- a/Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp
+++ b/Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp
@@ -64,12 +64,11 @@ JSValue JSHTMLDocument::nameGetter(ExecState* exec, JSValue slotBase, PropertyNa
 
     HTMLCollection* collection = document->documentNamedItems(propertyNameToAtomicString(propertyName));
 
-    unsigned length = collection->length();
-    if (!length)
+    if (!collection->hasAnyItem())
         return jsUndefined();
 
-    if (length == 1) {
-        Node* node = collection->firstItem();
+    if (collection->hasExactlyOneItem()) {
+        Node* node = collection->item(0);
 
         Frame* frame;
         if (node->hasTagName(iframeTag) && (frame = static_cast<HTMLIFrameElement*>(node)->contentFrame()))

diff --git a/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp b/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
index 0ea813e..c0964ad 100644 (file)
--- a/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
@@ -521,9 +521,9 @@ v8::Handle<v8::Value> V8DOMWindow::namedPropertyGetter(v8::Local<v8::String> nam
     if (doc && doc->isHTMLDocument()) {
         if (static_cast<HTMLDocument*>(doc)->hasNamedItem(propName.impl()) || doc->hasElementWithId(propName.impl())) {
             HTMLCollection* items = doc->windowNamedItems(propName);
-            if (items->length() >= 1) {
-                if (items->length() == 1)
-                    return toV8(items->firstItem(), info.GetIsolate());
+            if (items->hasAnyItem()) {
+                if (items->hasExactlyOneItem())
+                    return toV8(items->item(0), info.GetIsolate());
                 return toV8(items, info.GetIsolate());
             }
         }

diff --git a/Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp b/Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
index 6b9d0b3..df53317 100644 (file)
--- a/Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
@@ -85,11 +85,11 @@ v8::Handle<v8::Value> V8HTMLDocument::GetNamedProperty(HTMLDocument* htmlDocumen
         return v8::Handle<v8::Value>();
 
     HTMLCollection* items = htmlDocument->documentNamedItems(key);
-    if (!items->length())
+    if (!items->hasAnyItem())
         return v8::Handle<v8::Value>();
 
-    if (items->length() == 1) {
-        Node* node = items->firstItem();
+    if (items->hasExactlyOneItem()) {
+        Node* node = items->item(0);
         Frame* frame = 0;
         if (node->hasTagName(HTMLNames::iframeTag) && (frame = static_cast<HTMLIFrameElement*>(node)->contentFrame()))
             return toV8(frame->domWindow(), isolate);

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index 00452eb..4dca64b 100644 (file)
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -4581,7 +4581,7 @@ KURL Document::openSearchDescriptionURL()
         return KURL();
 
     HTMLCollection* children = head()->children();
-    for (Node* child = children->firstItem(); child; child = children->nextItem()) {
+    for (unsigned i = 0; Node* child = children->item(i); i++) {
         if (!child->hasTagName(linkTag))
             continue;
         HTMLLinkElement* linkElement = static_cast<HTMLLinkElement*>(child);

diff --git a/Source/WebCore/html/HTMLCollection.cpp b/Source/WebCore/html/HTMLCollection.cpp
index c20c464..1129cab 100644 (file)
--- a/Source/WebCore/html/HTMLCollection.cpp
+++ b/Source/WebCore/html/HTMLCollection.cpp
@@ -36,16 +36,7 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
-HTMLCollection::HTMLCollection(Node* base, CollectionType type)
-    : m_includeChildren(shouldIncludeChildren(type))
-    , m_type(type)
-    , m_base(base)
-{
-    ASSERT(m_base);
-    m_cache.clear();
-}
-
-bool HTMLCollection::shouldIncludeChildren(CollectionType type)
+static bool shouldIncludeChildren(CollectionType type)
 {
     switch (type) {
     case DocAll:
@@ -78,6 +69,15 @@ bool HTMLCollection::shouldIncludeChildren(CollectionType type)
     return false;
 }
 
+HTMLCollection::HTMLCollection(Node* base, CollectionType type)
+    : m_includeChildren(shouldIncludeChildren(type))
+    , m_type(type)
+    , m_base(base)
+{
+    ASSERT(m_base);
+    m_cache.clear();
+}
+
 PassOwnPtr<HTMLCollection> HTMLCollection::create(Node* base, CollectionType type)
 {
     return adoptPtr(new HTMLCollection(base, type));
@@ -199,39 +199,23 @@ unsigned HTMLCollection::length() const
 
 Node* HTMLCollection::item(unsigned index) const
 {
-     invalidateCacheIfNeeded();
-     if (m_cache.current && m_cache.position == index)
-         return m_cache.current;
-     if (m_cache.hasLength && m_cache.length <= index)
-         return 0;
-     if (!m_cache.current || m_cache.position > index) {
-         m_cache.current = itemAfter(0);
-         m_cache.position = 0;
-         if (!m_cache.current)
-             return 0;
-     }
-     Element* e = m_cache.current;
-     for (unsigned pos = m_cache.position; e && pos < index; pos++)
-         e = itemAfter(e);
-     m_cache.current = e;
-     m_cache.position = index;
-     return m_cache.current;
-}
-
-Node* HTMLCollection::firstItem() const
-{
-     return item(0);
-}
-
-Node* HTMLCollection::nextItem() const
-{
-     invalidateCacheIfNeeded();
-
-     // Look for the 'second' item. The first one is currentItem, already given back.
-     Element* retval = itemAfter(m_cache.current);
-     m_cache.current = retval;
-     m_cache.position++;
-     return retval;
+    invalidateCacheIfNeeded();
+    if (m_cache.current && m_cache.position == index)
+        return m_cache.current;
+    if (m_cache.hasLength && m_cache.length <= index)
+        return 0;
+    if (!m_cache.current || m_cache.position > index) {
+        m_cache.current = itemAfter(0);
+        m_cache.position = 0;
+        if (!m_cache.current)
+            return 0;
+    }
+    Element* e = m_cache.current;
+    for (unsigned pos = m_cache.position; e && pos < index; pos++)
+        e = itemAfter(e);
+    m_cache.current = e;
+    m_cache.position = index;
+    return m_cache.current;
 }
 
 static inline bool nameShouldBeVisibleInDocumentAll(HTMLElement* element)

diff --git a/Source/WebCore/html/HTMLCollection.h b/Source/WebCore/html/HTMLCollection.h
index fa3ff1e..ede81c0 100644 (file)
--- a/Source/WebCore/html/HTMLCollection.h
+++ b/Source/WebCore/html/HTMLCollection.h
@@ -44,19 +44,25 @@ public:
     void ref() { m_base->ref(); }
     void deref() { m_base->deref(); }
 
+    // DOM API
     unsigned length() const;
-
     virtual Node* item(unsigned index) const;
-    virtual Node* nextItem() const;
-
     virtual Node* namedItem(const AtomicString& name) const;
+    PassRefPtr<NodeList> tags(const String&);
 
-    Node* firstItem() const;
-
+    // Non-DOM API
     bool hasNamedItem(const AtomicString& name) const;
     void namedItems(const AtomicString& name, Vector<RefPtr<Node> >&) const;
-
-    PassRefPtr<NodeList> tags(const String&);
+    bool hasAnyItem() const
+    {
+        invalidateCacheIfNeeded();
+        return (m_cache.hasLength && m_cache.length) || m_cache.current || item(0);
+    }
+    bool hasExactlyOneItem() const
+    {
+        invalidateCacheIfNeeded();
+        return (m_cache.hasLength && m_cache.length == 1) || (m_cache.current && !itemAfter(m_cache.current)) || (item(0) && !item(1));
+    }
 
     Node* base() const { return m_base; }
     CollectionType type() const { return static_cast<CollectionType>(m_type); }
@@ -98,7 +104,6 @@ protected:
     } m_cache;
 
 private:
-    static bool shouldIncludeChildren(CollectionType);
     bool checkForNameMatch(Element*, bool checkName, const AtomicString& name) const;
 
     virtual unsigned calcLength() const;

diff --git a/Source/WebCore/html/HTMLMapElement.cpp b/Source/WebCore/html/HTMLMapElement.cpp
index 8f6d6f6..293eee3 100644 (file)
--- a/Source/WebCore/html/HTMLMapElement.cpp
+++ b/Source/WebCore/html/HTMLMapElement.cpp
@@ -82,8 +82,8 @@ bool HTMLMapElement::mapMouseEvent(LayoutPoint location, const LayoutSize& size,
 
 HTMLImageElement* HTMLMapElement::imageElement()
 {
-    HTMLCollection* coll = document()->images();
-    for (Node* curr = coll->firstItem(); curr; curr = coll->nextItem()) {
+    HTMLCollection* images = document()->images();
+    for (unsigned i = 0; Node* curr = images->item(i); i++) {
         if (!curr->hasTagName(imgTag))
             continue;
         

diff --git a/Source/WebKit/chromium/ChangeLog b/Source/WebKit/chromium/ChangeLog
index b653ebd..59efb7b 100644 (file)
--- a/Source/WebKit/chromium/ChangeLog
+++ b/Source/WebKit/chromium/ChangeLog
@@ -1,3 +1,24 @@
+2012-06-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Get rid of firstItem and nextItem from HTMLCollection
+        https://bugs.webkit.org/show_bug.cgi?id=89923
+
+        Reviewed by Andreas Kling.
+
+        Re-implement WebNodeCollection::firstItem() and WebNodeCollection::nextItem() in WebKit code
+        because we got rid of it from WebCore implementation.
+
+        This is an extremely poor API and we shouldn't be exposing it in the future.
+
+        * public/WebNodeCollection.h:
+        (WebKit::WebNodeCollection::WebNodeCollection):
+        (WebNodeCollection):
+        * src/WebNodeCollection.cpp:
+        (WebKit::WebNodeCollection::nextItem):
+        (WebKit::WebNodeCollection::firstItem):
+        * src/WebPageSerializerImpl.cpp:
+        (WebKit::WebPageSerializerImpl::collectTargetFrames):
+
 2012-06-25  Luke Macpherson  <macpherson@chromium.org>
 
         Add runtime flag to enable/disable CSS variables (in addition to existing compile-time flag).

diff --git a/Source/WebKit/chromium/public/WebNodeCollection.h b/Source/WebKit/chromium/public/WebNodeCollection.h
index b168b6d..a896f5d 100644 (file)
--- a/Source/WebKit/chromium/public/WebNodeCollection.h
+++ b/Source/WebKit/chromium/public/WebNodeCollection.h
@@ -46,7 +46,7 @@ class WebNodeCollection {
 public:
     ~WebNodeCollection() { reset(); }
 
-    WebNodeCollection() : m_private(0) { }
+    WebNodeCollection() : m_private(0), m_current(0) { }
     WebNodeCollection(const WebNodeCollection& n) : m_private(0) { assign(n); }
     WebNodeCollection& operator=(const WebNodeCollection& n)
     {
@@ -70,6 +70,7 @@ public:
 private:
     void assign(WebCore::HTMLCollection*);
     WebCore::HTMLCollection* m_private;
+    mutable unsigned m_current;
 };
 
 } // namespace WebKit

diff --git a/Source/WebKit/chromium/src/WebNodeCollection.cpp b/Source/WebKit/chromium/src/WebNodeCollection.cpp
index a8356d9..617bf56 100644 (file)
--- a/Source/WebKit/chromium/src/WebNodeCollection.cpp
+++ b/Source/WebKit/chromium/src/WebNodeCollection.cpp
@@ -74,12 +74,16 @@ unsigned WebNodeCollection::length() const
 
 WebNode WebNodeCollection::nextItem() const
 {
-    return WebNode(m_private->nextItem());
+    Node* node = m_private->item(m_current);
+    if (node)
+        m_current++;
+    return WebNode(node);
 }
 
 WebNode WebNodeCollection::firstItem() const
 {
-    return WebNode(m_private->firstItem());
+    m_current = 0;
+    return nextItem();
 }
 
 } // namespace WebKit

diff --git a/Source/WebKit/chromium/src/WebPageSerializerImpl.cpp b/Source/WebKit/chromium/src/WebPageSerializerImpl.cpp
index 072a990..ac200f1 100644 (file)
--- a/Source/WebKit/chromium/src/WebPageSerializerImpl.cpp
+++ b/Source/WebKit/chromium/src/WebPageSerializerImpl.cpp
@@ -477,7 +477,8 @@ void WebPageSerializerImpl::collectTargetFrames()
         Document* currentDoc = currentFrame->frame()->document();
         // Go through sub-frames.
         RefPtr<HTMLAllCollection> all = currentDoc->all();
-        for (Node* node = all->firstItem(); node; node = all->nextItem()) {
+
+        for (unsigned i = 0; Node* node = all->item(i); i++) {
             if (!node->isHTMLElement())
                 continue;
             Element* element = static_cast<Element*>(node);