From d50a665fd92da96ac24164ccbca70aa5a4d3b22b Mon Sep 17 00:00:00 2001 From: "rniwa@webkit.org" Date: Tue, 26 Jun 2012 06:29:58 +0000 Subject: [PATCH] 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 --- Source/WebCore/ChangeLog | 35 +++++++++++ .../accessibility/AccessibilityRenderObject.cpp | 6 +- Source/WebCore/bindings/js/JSDOMWindowCustom.cpp | 4 +- .../WebCore/bindings/js/JSHTMLDocumentCustom.cpp | 7 +-- .../bindings/v8/custom/V8DOMWindowCustom.cpp | 6 +- .../bindings/v8/custom/V8HTMLDocumentCustom.cpp | 6 +- Source/WebCore/dom/Document.cpp | 2 +- Source/WebCore/html/HTMLCollection.cpp | 70 +++++++++------------- Source/WebCore/html/HTMLCollection.h | 21 ++++--- Source/WebCore/html/HTMLMapElement.cpp | 4 +- Source/WebKit/chromium/ChangeLog | 21 +++++++ Source/WebKit/chromium/public/WebNodeCollection.h | 3 +- Source/WebKit/chromium/src/WebNodeCollection.cpp | 8 ++- .../WebKit/chromium/src/WebPageSerializerImpl.cpp | 3 +- 14 files changed, 122 insertions(+), 74 deletions(-) diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 7a14020..9de02fd 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,38 @@ +2012-06-25 Ryosuke Niwa + + 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 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 --- 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 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 --- 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 --- 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(node)->contentFrame())) diff --git a/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp b/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp index 0ea813e..c0964ad 100644 --- a/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp @@ -521,9 +521,9 @@ v8::Handle V8DOMWindow::namedPropertyGetter(v8::Local nam if (doc && doc->isHTMLDocument()) { if (static_cast(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 --- a/Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp @@ -85,11 +85,11 @@ v8::Handle V8HTMLDocument::GetNamedProperty(HTMLDocument* htmlDocumen return v8::Handle(); HTMLCollection* items = htmlDocument->documentNamedItems(key); - if (!items->length()) + if (!items->hasAnyItem()) return v8::Handle(); - 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(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 --- 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(child); diff --git a/Source/WebCore/html/HTMLCollection.cpp b/Source/WebCore/html/HTMLCollection.cpp index c20c464..1129cab 100644 --- 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::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 --- 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 tags(const String&); - Node* firstItem() const; - + // Non-DOM API bool hasNamedItem(const AtomicString& name) const; void namedItems(const AtomicString& name, Vector >&) const; - - PassRefPtr 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(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 --- 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 --- a/Source/WebKit/chromium/ChangeLog +++ b/Source/WebKit/chromium/ChangeLog @@ -1,3 +1,24 @@ +2012-06-25 Ryosuke Niwa + + 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 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 --- 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 --- 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 --- 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 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(node); -- 2.7.4