Cut down on double hashing and code needlessly using hash table iterators
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Sep 2013 06:00:45 +0000 (06:00 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Sep 2013 06:00:45 +0000 (06:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=120611

Reviewed by Andreas Kling.

Source/WebCore:

Some of these changes are primarily code cleanup, but others could provide
a small code size and speed improvement by avoiding extra hashing.

* Modules/geolocation/Geolocation.cpp:
(WebCore::Geolocation::Watchers::find): Use get instead of find.
(WebCore::Geolocation::Watchers::remove): Use take instead of find.
(WebCore::Geolocation::makeCachedPositionCallbacks): Use the return
value from remove to avoid hashing twice.

* Modules/webaudio/AudioContext.cpp:
(WebCore::AudioContext::addAutomaticPullNode): Use the return value from
add to avoid hashing twice.
(WebCore::AudioContext::removeAutomaticPullNode): Use the return value
from remove to avoid hashing twice.

* Modules/webaudio/AudioNodeInput.cpp:
(WebCore::AudioNodeInput::connect): Use the return value from add to avoid
hashing twice.
(WebCore::AudioNodeInput::disconnect): Use the return value from remove
to avoid hashing twice.

* Modules/webaudio/AudioParam.cpp:
(WebCore::AudioParam::connect): Use the return value from add to avoid
hashing twice.
(WebCore::AudioParam::disconnect): Use the return value from remove to
avoid hashing twice.

* bridge/NP_jsobject.cpp:
(ObjectMap::remove): Use remove instead of find/remove.

* dom/Node.cpp:
(WebCore::Node::~Node): Use the return value from remove instead of
find/remove.

* inspector/InspectorProfilerAgent.cpp:
(WebCore::InspectorProfilerAgent::removeProfile): Remove needless
calls to contains.

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::removeSubresourceLoader): Use the return
value from remove instead of find/remove.

* loader/ResourceLoadScheduler.cpp:
(WebCore::ResourceLoadScheduler::HostInformation::remove): Use the
return value from remove to avoid hashing twice.

* loader/appcache/ApplicationCacheGroup.cpp:
(WebCore::ApplicationCacheGroup::disassociateDocumentLoader): Use
remove instead of find/remove.
(WebCore::ApplicationCacheGroup::cacheDestroyed): Removed a needless
call to contains to avoid hashing twice. It's fine to do the check
for an empty hash table unconditionally.

* page/DOMWindow.cpp:
(WebCore::addUnloadEventListener): Eliminated a local variable for clarity.
(WebCore::removeUnloadEventListener): Ditto. Also use remove instead
of find/remove.
(WebCore::removeAllUnloadEventListeners): Ditto. Also use removeAll instead
of find/removeAll.
(WebCore::addBeforeUnloadEventListener): Ditto.
(WebCore::removeBeforeUnloadEventListener): Ditto.
(WebCore::removeAllBeforeUnloadEventListeners): Ditto.

* page/FrameView.cpp:
(WebCore::FrameView::removeViewportConstrainedObject): Use the return
value from remove to avoid hashing twice.
(WebCore::FrameView::removeScrollableArea): Use the return value from
remove instead of find/remove.
(WebCore::FrameView::containsScrollableArea): Use && instead of an if
statement in a way that is idiomatic for this kind of function.

* page/Page.cpp:
(WebCore::Page::addRelevantRepaintedObject): Use the return value from
remove instead of find/remove.

* page/PageGroup.cpp:
(WebCore::PageGroup::removeUserScriptsFromWorld): Use remove instead
of find/remove.
(WebCore::PageGroup::removeUserStyleSheetsFromWorld): Use the return
value from remove instead of find/remove.

* page/PerformanceUserTiming.cpp:
(WebCore::clearPeformanceEntries): Removed a needless call to contains.

* platform/graphics/DisplayRefreshMonitor.cpp:
(WebCore::DisplayRefreshMonitor::removeClient): Use the return value
from remove instead of find/remove.
(WebCore::DisplayRefreshMonitorManager::displayDidRefresh): Use remove
instead of find/remove.

* platform/graphics/blackberry/LayerRenderer.cpp:
(WebCore::LayerRenderer::removeLayer): Use the return value from remove
instead of find/remove.

* platform/win/WindowMessageBroadcaster.cpp:
(WebCore::WindowMessageBroadcaster::removeListener): Use remove instead
of find/remove. It's fine to do the check for an empty hash table unconditionally.

* plugins/PluginDatabase.cpp:
(WebCore::PluginDatabase::removeDisabledPluginFile): Use the return value
from remove instead of find/remove.

* rendering/style/StyleCustomFilterProgramCache.cpp:
(WebCore::StyleCustomFilterProgramCache::lookup): Use get instead of find.
(WebCore::StyleCustomFilterProgramCache::add): Use contains instead of find
in an assertion.
(WebCore::StyleCustomFilterProgramCache::remove): Use remove instead of
find/remove.

* svg/SVGCursorElement.cpp:
(WebCore::SVGCursorElement::removeClient): Use the return value from remove
instead of find/remove.

* svg/SVGDocumentExtensions.cpp:
(WebCore::SVGDocumentExtensions::removeResource): Removed an unneeded call
to contains.
(WebCore::SVGDocumentExtensions::removeAllTargetReferencesForElement): Use
remove instead of find/remove. It's fine to do the check for an empty hash
table unconditionally.
(WebCore::SVGDocumentExtensions::removeAllElementReferencesForTarget): Use
remove instead of find/remove. Also removed unhelpful assertions. One is
already done by HashMap, and the other is just checking a basic invariant
of every HashMap that doesn't need to be checked.

* svg/graphics/SVGImageCache.cpp:
(WebCore::SVGImageCache::removeClientFromCache): Removed an unneeded call
to contains.

* svg/properties/SVGAnimatedProperty.cpp:
(WebCore::SVGAnimatedProperty::~SVGAnimatedProperty): Use the version of
remove that takes an iterator rather than the one that takes a key, so
we don't need to redo the hashing.

Source/WebKit2:

* Platform/CoreIPC/Connection.cpp:
(CoreIPC::Connection::waitForMessage): Use take instead of find/remove.

* UIProcess/WebPreferences.cpp:
(WebKit::WebPreferences::removePageGroup): Use the return value from remove
instead of find/remove.

* WebProcess/Geolocation/GeolocationPermissionRequestManager.cpp:
(WebKit::GeolocationPermissionRequestManager::cancelRequestForGeolocation):
(WebKit::GeolocationPermissionRequestManager::didReceiveGeolocationPermissionDecision):
Use take instead of find/remove.

* WebProcess/Plugins/Netscape/NetscapePlugin.cpp:
(WebKit::NetscapePlugin::frameDidFinishLoading): Use take instead of find/remove.
(WebKit::NetscapePlugin::frameDidFail): Use take instead of find/remove.

* WebProcess/WebPage/WebBackForwardListProxy.cpp:
(WebKit::WebBackForwardListProxy::removeItem): Use take instead of find/remove.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::didFinishCheckingText): Use take instead of get/remove so we
hash only once.
(WebKit::WebPage::didCancelCheckingText): Ditto.
(WebKit::WebPage::stopExtendingIncrementalRenderingSuppression): Use the return
value from remove instead of contains/remove so we hash only once.

Source/WTF:

Double hashing is common in code that needs to combine a remove with some
action to only be done if the code is removed. The only way to avoid it is
to write code using find and a hash table iterator. To help with this, add
a boolean return value to remove functions to indicate if anything was removed.

Double hashing also happens in code that does a get followed by a remove.
The take function is helpful in this case. To help with this, add a takeFirst
funciton to ListHashSet.

* wtf/HashCountedSet.h:
(WTF::HashCountedSet::removeAll): Added a boolean return value, analogous to the one
that the HashCountedSet::remove function already has.

* wtf/HashMap.h:
(WTF::HashMap::remove): Added a boolean return value, true if something was removed.
* wtf/HashSet.h:
(WTF::HashSet::remove): Ditto.
* wtf/RefPtrHashMap.h:
(WTF::RefPtrHashMap::remove): Ditto.

* wtf/ListHashSet.h:
(WTF::ListHashSet::takeFirst): Added.
(WTF::ListHashSet::takeLast): Added.
(WTF::ListHashSet::remove): Added a boolean return value, true if something was removed.

* wtf/WTFThreadData.h:
(JSC::IdentifierTable::remove): Use the new remove return value to get rid of most of
the code in this function.

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

39 files changed:
Source/WTF/ChangeLog
Source/WTF/wtf/HashCountedSet.h
Source/WTF/wtf/HashMap.h
Source/WTF/wtf/HashSet.h
Source/WTF/wtf/ListHashSet.h
Source/WTF/wtf/RefPtrHashMap.h
Source/WTF/wtf/WTFThreadData.h
Source/WebCore/ChangeLog
Source/WebCore/Modules/geolocation/Geolocation.cpp
Source/WebCore/Modules/webaudio/AudioContext.cpp
Source/WebCore/Modules/webaudio/AudioNodeInput.cpp
Source/WebCore/Modules/webaudio/AudioParam.cpp
Source/WebCore/bridge/NP_jsobject.cpp
Source/WebCore/dom/Node.cpp
Source/WebCore/inspector/InspectorProfilerAgent.cpp
Source/WebCore/loader/DocumentLoader.cpp
Source/WebCore/loader/ResourceLoadScheduler.cpp
Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp
Source/WebCore/page/DOMWindow.cpp
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/Page.cpp
Source/WebCore/page/PageGroup.cpp
Source/WebCore/page/PerformanceUserTiming.cpp
Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp
Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp
Source/WebCore/platform/win/WindowMessageBroadcaster.cpp
Source/WebCore/plugins/PluginDatabase.cpp
Source/WebCore/rendering/style/StyleCustomFilterProgramCache.cpp
Source/WebCore/svg/SVGCursorElement.cpp
Source/WebCore/svg/SVGDocumentExtensions.cpp
Source/WebCore/svg/graphics/SVGImageCache.cpp
Source/WebCore/svg/properties/SVGAnimatedProperty.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/Platform/CoreIPC/Connection.cpp
Source/WebKit2/UIProcess/WebPreferences.cpp
Source/WebKit2/WebProcess/Geolocation/GeolocationPermissionRequestManager.cpp
Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp
Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp
Source/WebKit2/WebProcess/WebPage/WebPage.cpp

index 708fd6f..56743b5 100644 (file)
@@ -1,3 +1,39 @@
+2013-09-02  Darin Adler  <darin@apple.com>
+
+        Cut down on double hashing and code needlessly using hash table iterators
+        https://bugs.webkit.org/show_bug.cgi?id=120611
+
+        Reviewed by Andreas Kling.
+
+        Double hashing is common in code that needs to combine a remove with some
+        action to only be done if the code is removed. The only way to avoid it is
+        to write code using find and a hash table iterator. To help with this, add
+        a boolean return value to remove functions to indicate if anything was removed.
+
+        Double hashing also happens in code that does a get followed by a remove.
+        The take function is helpful in this case. To help with this, add a takeFirst
+        funciton to ListHashSet.
+
+        * wtf/HashCountedSet.h:
+        (WTF::HashCountedSet::removeAll): Added a boolean return value, analogous to the one
+        that the HashCountedSet::remove function already has.
+
+        * wtf/HashMap.h:
+        (WTF::HashMap::remove): Added a boolean return value, true if something was removed.
+        * wtf/HashSet.h:
+        (WTF::HashSet::remove): Ditto.
+        * wtf/RefPtrHashMap.h:
+        (WTF::RefPtrHashMap::remove): Ditto.
+
+        * wtf/ListHashSet.h:
+        (WTF::ListHashSet::takeFirst): Added.
+        (WTF::ListHashSet::takeLast): Added.
+        (WTF::ListHashSet::remove): Added a boolean return value, true if something was removed.
+
+        * wtf/WTFThreadData.h:
+        (JSC::IdentifierTable::remove): Use the new remove return value to get rid of most of
+        the code in this function.
+
 2013-09-02  David Kilzer  <ddkilzer@apple.com>
 
         Remove duplicate entries found by Xcode in WTF project
index eff4a29..3673688 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005, 2006, 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2005, 2006, 2008, 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -68,8 +68,8 @@ namespace WTF {
         bool remove(iterator);
  
         // Removes the value, regardless of its count.
-        void removeAll(iterator);
-        void removeAll(const ValueType&);
+        bool removeAll(iterator);
+        bool removeAll(const ValueType&);
 
         // Clears the whole set.
         void clear();
@@ -183,18 +183,19 @@ namespace WTF {
     }
     
     template<typename Value, typename HashFunctions, typename Traits>
-    inline void HashCountedSet<Value, HashFunctions, Traits>::removeAll(const ValueType& value)
+    inline bool HashCountedSet<Value, HashFunctions, Traits>::removeAll(const ValueType& value)
     {
-        removeAll(find(value));
+        return removeAll(find(value));
     }
     
     template<typename Value, typename HashFunctions, typename Traits>
-    inline void HashCountedSet<Value, HashFunctions, Traits>::removeAll(iterator it)
+    inline bool HashCountedSet<Value, HashFunctions, Traits>::removeAll(iterator it)
     {
         if (it == end())
-            return;
+            return false;
 
         m_impl.remove(it);
+        return true;
     }
     
     template<typename Value, typename HashFunctions, typename Traits>
index 708e8d8..037baf1 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005, 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2005, 2006, 2007, 2008, 2011, 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -106,8 +106,8 @@ namespace WTF {
         // and a boolean that's true if a new value was actually added
         AddResult add(const KeyType&, MappedPassInType);
 
-        void remove(const KeyType&);
-        void remove(iterator);
+        bool remove(const KeyType&);
+        bool remove(iterator);
         void clear();
 
         MappedPassOutType take(const KeyType&); // efficient combination of get with remove
@@ -380,18 +380,19 @@ namespace WTF {
     }
 
     template<typename T, typename U, typename V, typename W, typename X>
-    inline void HashMap<T, U, V, W, X>::remove(iterator it)
+    inline bool HashMap<T, U, V, W, X>::remove(iterator it)
     {
         if (it.m_impl == m_impl.end())
-            return;
+            return false;
         m_impl.internalCheckTableConsistency();
         m_impl.removeWithoutEntryConsistencyCheck(it.m_impl);
+        return true;
     }
 
     template<typename T, typename U, typename V, typename W, typename X>
-    inline void HashMap<T, U, V, W, X>::remove(const KeyType& key)
+    inline bool HashMap<T, U, V, W, X>::remove(const KeyType& key)
     {
-        remove(find(key));
+        return remove(find(key));
     }
 
     template<typename T, typename U, typename V, typename W, typename X>
index 208552d..607b6bc 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005, 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2005, 2006, 2007, 2008, 2011, 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -88,8 +88,8 @@ namespace WTF {
         template<typename IteratorType>
         bool add(IteratorType begin, IteratorType end);
 
-        void remove(const ValueType&);
-        void remove(iterator);
+        bool remove(const ValueType&);
+        bool remove(iterator);
         void clear();
 
         static bool isValidValue(const ValueType&);
@@ -204,18 +204,19 @@ namespace WTF {
     }
 
     template<typename T, typename U, typename V>
-    inline void HashSet<T, U, V>::remove(iterator it)
+    inline bool HashSet<T, U, V>::remove(iterator it)
     {
         if (it.m_impl == m_impl.end())
-            return;
+            return false;
         m_impl.internalCheckTableConsistency();
         m_impl.removeWithoutEntryConsistencyCheck(it.m_impl);
+        return true;
     }
 
     template<typename T, typename U, typename V>
-    inline void HashSet<T, U, V>::remove(const ValueType& value)
+    inline bool HashSet<T, U, V>::remove(const ValueType& value)
     {
-        remove(find(value));
+        return remove(find(value));
     }
 
     template<typename T, typename U, typename V>
index 78639b4..0732160 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005, 2006, 2007, 2008, 2011, 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2005, 2006, 2007, 2008, 2011, 2012, 2013 Apple Inc. All rights reserved.
  * Copyright (C) 2011, Benjamin Poulain <ikipou@gmail.com>
  *
  * This library is free software; you can redistribute it and/or
@@ -107,10 +107,12 @@ namespace WTF {
         ValueType& first();
         const ValueType& first() const;
         void removeFirst();
+        ValueType takeFirst();
 
         ValueType& last();
         const ValueType& last() const;
         void removeLast();
+        ValueType takeLast();
 
         iterator find(const ValueType&);
         const_iterator find(const ValueType&) const;
@@ -140,8 +142,8 @@ namespace WTF {
         AddResult insertBefore(const ValueType& beforeValue, const ValueType& newValue);
         AddResult insertBefore(iterator, const ValueType&);
 
-        void remove(const ValueType&);
-        void remove(iterator);
+        bool remove(const ValueType&);
+        bool remove(iterator);
         void clear();
 
     private:
@@ -629,6 +631,14 @@ namespace WTF {
     }
 
     template<typename T, size_t inlineCapacity, typename U>
+    inline T ListHashSet<T, inlineCapacity, U>::takeFirst()
+    {
+        T result = first();
+        removeFirst();
+        return result;
+    }
+
+    template<typename T, size_t inlineCapacity, typename U>
     inline const T& ListHashSet<T, inlineCapacity, U>::first() const
     {
         ASSERT(!isEmpty());
@@ -658,6 +668,14 @@ namespace WTF {
     }
 
     template<typename T, size_t inlineCapacity, typename U>
+    inline T ListHashSet<T, inlineCapacity, U>::takeLast()
+    {
+        T result = last();
+        removeLast();
+        return result;
+    }
+
+    template<typename T, size_t inlineCapacity, typename U>
     inline typename ListHashSet<T, inlineCapacity, U>::iterator ListHashSet<T, inlineCapacity, U>::find(const ValueType& value)
     {
         ImplTypeIterator it = m_impl.template find<BaseTranslator>(value);
@@ -761,18 +779,19 @@ namespace WTF {
     }
 
     template<typename T, size_t inlineCapacity, typename U>
-    inline void ListHashSet<T, inlineCapacity, U>::remove(iterator it)
+    inline bool ListHashSet<T, inlineCapacity, U>::remove(iterator it)
     {
         if (it == end())
-            return;
+            return false;
         m_impl.remove(it.node());
         unlinkAndDelete(it.node());
+        return true;
     }
 
     template<typename T, size_t inlineCapacity, typename U>
-    inline void ListHashSet<T, inlineCapacity, U>::remove(const ValueType& value)
+    inline bool ListHashSet<T, inlineCapacity, U>::remove(const ValueType& value)
     {
-        remove(find(value));
+        return remove(find(value));
     }
 
     template<typename T, size_t inlineCapacity, typename U>
index 9541837..01cce6e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005, 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2005, 2006, 2007, 2008, 2011, 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -96,9 +96,9 @@ namespace WTF {
         AddResult add(const KeyType&, MappedPassInType);
         AddResult add(RawKeyType, MappedPassInType);
 
-        void remove(const KeyType&);
-        void remove(RawKeyType);
-        void remove(iterator);
+        bool remove(const KeyType&);
+        bool remove(RawKeyType);
+        bool remove(iterator);
         void clear();
 
         MappedPassOutType take(const KeyType&); // efficient combination of get with remove
@@ -275,24 +275,25 @@ namespace WTF {
     }
 
     template<typename T, typename U, typename V, typename W, typename X>
-    inline void HashMap<RefPtr<T>, U, V, W, X>::remove(iterator it)
+    inline bool HashMap<RefPtr<T>, U, V, W, X>::remove(iterator it)
     {
         if (it.m_impl == m_impl.end())
-            return;
+            return false;
         m_impl.internalCheckTableConsistency();
         m_impl.removeWithoutEntryConsistencyCheck(it.m_impl);
+        return true;
     }
 
     template<typename T, typename U, typename V, typename W, typename X>
-    inline void HashMap<RefPtr<T>, U, V, W, X>::remove(const KeyType& key)
+    inline bool HashMap<RefPtr<T>, U, V, W, X>::remove(const KeyType& key)
     {
-        remove(find(key));
+        return remove(find(key));
     }
 
     template<typename T, typename U, typename V, typename W, typename X>
-    inline void HashMap<RefPtr<T>, U, V, W, X>::remove(RawKeyType key)
+    inline bool HashMap<RefPtr<T>, U, V, W, X>::remove(RawKeyType key)
     {
-        remove(find(key));
+        return remove(find(key));
     }
 
     template<typename T, typename U, typename V, typename W, typename X>
index fa157e9..0c6a54d 100644 (file)
@@ -36,7 +36,7 @@
 #include <wtf/ThreadSpecific.h>
 #include <wtf/Threading.h>
 
-// FIXME: This is a temporary layering violation while we move more string code to WTF.
+// FIXME: This is a temporary layering violation until we move more of the string code from JavaScriptCore to WTF.
 namespace JSC {
 
 class IdentifierTable {
@@ -44,18 +44,10 @@ class IdentifierTable {
 public:
     WTF_EXPORT_PRIVATE ~IdentifierTable();
 
-    WTF_EXPORT_PRIVATE HashSet<StringImpl*>::AddResult add(StringImpl* value);
-    template<typename U, typename V>
-    HashSet<StringImpl*>::AddResult add(U value);
+    WTF_EXPORT_PRIVATE HashSet<StringImpl*>::AddResult add(StringImpl*);
+    template<typename U, typename V> HashSet<StringImpl*>::AddResult add(U);
 
-    bool remove(StringImpl* r)
-    {
-        HashSet<StringImpl*>::iterator iter = m_table.find(r);
-        if (iter == m_table.end())
-            return false;
-        m_table.remove(iter);
-        return true;
-    }
+    bool remove(StringImpl* identifier) { return m_table.remove(identifier); }
 
 private:
     HashSet<StringImpl*> m_table;
index 8d1bb9a..2267407 100644 (file)
@@ -1,3 +1,143 @@
+2013-09-02  Darin Adler  <darin@apple.com>
+
+        Cut down on double hashing and code needlessly using hash table iterators
+        https://bugs.webkit.org/show_bug.cgi?id=120611
+
+        Reviewed by Andreas Kling.
+
+        Some of these changes are primarily code cleanup, but others could provide
+        a small code size and speed improvement by avoiding extra hashing.
+
+        * Modules/geolocation/Geolocation.cpp:
+        (WebCore::Geolocation::Watchers::find): Use get instead of find.
+        (WebCore::Geolocation::Watchers::remove): Use take instead of find.
+        (WebCore::Geolocation::makeCachedPositionCallbacks): Use the return
+        value from remove to avoid hashing twice.
+
+        * Modules/webaudio/AudioContext.cpp:
+        (WebCore::AudioContext::addAutomaticPullNode): Use the return value from
+        add to avoid hashing twice.
+        (WebCore::AudioContext::removeAutomaticPullNode): Use the return value
+        from remove to avoid hashing twice.
+
+        * Modules/webaudio/AudioNodeInput.cpp:
+        (WebCore::AudioNodeInput::connect): Use the return value from add to avoid
+        hashing twice.
+        (WebCore::AudioNodeInput::disconnect): Use the return value from remove
+        to avoid hashing twice.
+
+        * Modules/webaudio/AudioParam.cpp:
+        (WebCore::AudioParam::connect): Use the return value from add to avoid
+        hashing twice.
+        (WebCore::AudioParam::disconnect): Use the return value from remove to
+        avoid hashing twice.
+
+        * bridge/NP_jsobject.cpp:
+        (ObjectMap::remove): Use remove instead of find/remove.
+
+        * dom/Node.cpp:
+        (WebCore::Node::~Node): Use the return value from remove instead of
+        find/remove.
+
+        * inspector/InspectorProfilerAgent.cpp:
+        (WebCore::InspectorProfilerAgent::removeProfile): Remove needless
+        calls to contains.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::removeSubresourceLoader): Use the return
+        value from remove instead of find/remove.
+
+        * loader/ResourceLoadScheduler.cpp:
+        (WebCore::ResourceLoadScheduler::HostInformation::remove): Use the
+        return value from remove to avoid hashing twice.
+
+        * loader/appcache/ApplicationCacheGroup.cpp:
+        (WebCore::ApplicationCacheGroup::disassociateDocumentLoader): Use
+        remove instead of find/remove.
+        (WebCore::ApplicationCacheGroup::cacheDestroyed): Removed a needless
+        call to contains to avoid hashing twice. It's fine to do the check
+        for an empty hash table unconditionally.
+
+        * page/DOMWindow.cpp:
+        (WebCore::addUnloadEventListener): Eliminated a local variable for clarity.
+        (WebCore::removeUnloadEventListener): Ditto. Also use remove instead
+        of find/remove.
+        (WebCore::removeAllUnloadEventListeners): Ditto. Also use removeAll instead
+        of find/removeAll.
+        (WebCore::addBeforeUnloadEventListener): Ditto.
+        (WebCore::removeBeforeUnloadEventListener): Ditto.
+        (WebCore::removeAllBeforeUnloadEventListeners): Ditto.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::removeViewportConstrainedObject): Use the return
+        value from remove to avoid hashing twice.
+        (WebCore::FrameView::removeScrollableArea): Use the return value from
+        remove instead of find/remove.
+        (WebCore::FrameView::containsScrollableArea): Use && instead of an if
+        statement in a way that is idiomatic for this kind of function.
+
+        * page/Page.cpp:
+        (WebCore::Page::addRelevantRepaintedObject): Use the return value from
+        remove instead of find/remove.
+
+        * page/PageGroup.cpp:
+        (WebCore::PageGroup::removeUserScriptsFromWorld): Use remove instead
+        of find/remove.
+        (WebCore::PageGroup::removeUserStyleSheetsFromWorld): Use the return
+        value from remove instead of find/remove.
+
+        * page/PerformanceUserTiming.cpp:
+        (WebCore::clearPeformanceEntries): Removed a needless call to contains.
+
+        * platform/graphics/DisplayRefreshMonitor.cpp:
+        (WebCore::DisplayRefreshMonitor::removeClient): Use the return value
+        from remove instead of find/remove.
+        (WebCore::DisplayRefreshMonitorManager::displayDidRefresh): Use remove
+        instead of find/remove.
+
+        * platform/graphics/blackberry/LayerRenderer.cpp:
+        (WebCore::LayerRenderer::removeLayer): Use the return value from remove
+        instead of find/remove.
+
+        * platform/win/WindowMessageBroadcaster.cpp:
+        (WebCore::WindowMessageBroadcaster::removeListener): Use remove instead
+        of find/remove. It's fine to do the check for an empty hash table unconditionally.
+
+        * plugins/PluginDatabase.cpp:
+        (WebCore::PluginDatabase::removeDisabledPluginFile): Use the return value
+        from remove instead of find/remove.
+
+        * rendering/style/StyleCustomFilterProgramCache.cpp:
+        (WebCore::StyleCustomFilterProgramCache::lookup): Use get instead of find.
+        (WebCore::StyleCustomFilterProgramCache::add): Use contains instead of find
+        in an assertion.
+        (WebCore::StyleCustomFilterProgramCache::remove): Use remove instead of
+        find/remove.
+
+        * svg/SVGCursorElement.cpp:
+        (WebCore::SVGCursorElement::removeClient): Use the return value from remove
+        instead of find/remove.
+
+        * svg/SVGDocumentExtensions.cpp:
+        (WebCore::SVGDocumentExtensions::removeResource): Removed an unneeded call
+        to contains.
+        (WebCore::SVGDocumentExtensions::removeAllTargetReferencesForElement): Use
+        remove instead of find/remove. It's fine to do the check for an empty hash
+        table unconditionally.
+        (WebCore::SVGDocumentExtensions::removeAllElementReferencesForTarget): Use
+        remove instead of find/remove. Also removed unhelpful assertions. One is
+        already done by HashMap, and the other is just checking a basic invariant
+        of every HashMap that doesn't need to be checked.
+
+        * svg/graphics/SVGImageCache.cpp:
+        (WebCore::SVGImageCache::removeClientFromCache): Removed an unneeded call
+        to contains.
+
+        * svg/properties/SVGAnimatedProperty.cpp:
+        (WebCore::SVGAnimatedProperty::~SVGAnimatedProperty): Use the version of
+        remove that takes an iterator rather than the one that takes a key, so
+        we don't need to redo the hashing.
+
 2013-09-02  Andreas Kling  <akling@apple.com>
 
         Generate isFooElement() functions from tagname data.
index c596866..0735996 100644 (file)
@@ -188,29 +188,20 @@ bool Geolocation::Watchers::add(int id, PassRefPtr<GeoNotifier> prpNotifier)
 Geolocation::GeoNotifier* Geolocation::Watchers::find(int id)
 {
     ASSERT(id > 0);
-    IdToNotifierMap::iterator iter = m_idToNotifierMap.find(id);
-    if (iter == m_idToNotifierMap.end())
-        return 0;
-    return iter->value.get();
+    return m_idToNotifierMap.get(id);
 }
 
 void Geolocation::Watchers::remove(int id)
 {
     ASSERT(id > 0);
-    IdToNotifierMap::iterator iter = m_idToNotifierMap.find(id);
-    if (iter == m_idToNotifierMap.end())
-        return;
-    m_notifierToIdMap.remove(iter->value);
-    m_idToNotifierMap.remove(iter);
+    if (auto notifier = m_idToNotifierMap.take(id))
+        m_notifierToIdMap.remove(notifier);
 }
 
 void Geolocation::Watchers::remove(GeoNotifier* notifier)
 {
-    NotifierToIdMap::iterator iter = m_notifierToIdMap.find(notifier);
-    if (iter == m_notifierToIdMap.end())
-        return;
-    m_idToNotifierMap.remove(iter->value);
-    m_notifierToIdMap.remove(iter);
+    if (auto identifier = m_notifierToIdMap.take(notifier))
+        m_idToNotifierMap.remove(identifier);
 }
 
 bool Geolocation::Watchers::contains(GeoNotifier* notifier) const
@@ -380,9 +371,7 @@ void Geolocation::makeCachedPositionCallbacks()
 
         // If this is a one-shot request, stop it. Otherwise, if the watch still
         // exists, start the service to get updates.
-        if (m_oneShots.contains(notifier))
-            m_oneShots.remove(notifier);
-        else if (m_watchers.contains(notifier)) {
+        if (!m_oneShots.remove(notifier) && m_watchers.contains(notifier)) {
             if (notifier->hasZeroTimeout() || startUpdating(notifier))
                 notifier->startTimerIfNeeded();
             else
index 3979fa5..ccdb305 100644 (file)
@@ -912,20 +912,16 @@ void AudioContext::addAutomaticPullNode(AudioNode* node)
 {
     ASSERT(isGraphOwner());
 
-    if (!m_automaticPullNodes.contains(node)) {
-        m_automaticPullNodes.add(node);
+    if (m_automaticPullNodes.add(node).isNewEntry)
         m_automaticPullNodesNeedUpdating = true;
-    }
 }
 
 void AudioContext::removeAutomaticPullNode(AudioNode* node)
 {
     ASSERT(isGraphOwner());
 
-    if (m_automaticPullNodes.contains(node)) {
-        m_automaticPullNodes.remove(node);
+    if (m_automaticPullNodes.remove(node))
         m_automaticPullNodesNeedUpdating = true;
-    }
 }
 
 void AudioContext::updateAutomaticPullNodes()
index 5213650..6a2e5dd 100644 (file)
@@ -54,11 +54,10 @@ void AudioNodeInput::connect(AudioNodeOutput* output)
         return;
 
     // Check if we're already connected to this output.
-    if (m_outputs.contains(output))
+    if (!m_outputs.add(output).isNewEntry)
         return;
         
     output->addInput(this);
-    m_outputs.add(output);
     changedOutputs();
 
     // Sombody has just connected to us, so count it as a reference.
@@ -74,8 +73,7 @@ void AudioNodeInput::disconnect(AudioNodeOutput* output)
         return;
 
     // First try to disconnect from "active" connections.
-    if (m_outputs.contains(output)) {
-        m_outputs.remove(output);
+    if (m_outputs.remove(output)) {
         changedOutputs();
         output->removeInput(this);
         node()->deref(AudioNode::RefTypeConnection); // Note: it's important to return immediately after all deref() calls since the node may be deleted.
@@ -83,8 +81,7 @@ void AudioNodeInput::disconnect(AudioNodeOutput* output)
     }
     
     // Otherwise, try to disconnect from disabled connections.
-    if (m_disabledOutputs.contains(output)) {
-        m_disabledOutputs.remove(output);
+    if (m_disabledOutputs.remove(output)) {
         output->removeInput(this);
         node()->deref(AudioNode::RefTypeConnection); // Note: it's important to return immediately after all deref() calls since the node may be deleted.
         return;
index 2be5039..3e4899f 100644 (file)
@@ -172,11 +172,10 @@ void AudioParam::connect(AudioNodeOutput* output)
     if (!output)
         return;
 
-    if (m_outputs.contains(output))
+    if (!m_outputs.add(output).isNewEntry)
         return;
 
     output->addParam(this);
-    m_outputs.add(output);
     changedOutputs();
 }
 
@@ -188,8 +187,7 @@ void AudioParam::disconnect(AudioNodeOutput* output)
     if (!output)
         return;
 
-    if (m_outputs.contains(output)) {
-        m_outputs.remove(output);
+    if (m_outputs.remove(output)) {
         changedOutputs();
         output->removeParam(this);
     }
index 7eadd9d..9e6f684 100644 (file)
@@ -70,9 +70,8 @@ public:
 
     void remove(RootObject* rootObject)
     {
-        HashMap<RootObject*, JSToNPObjectMap>::iterator iter = m_map.find(rootObject);
-        ASSERT(iter != m_map.end());
-        m_map.remove(iter);
+        ASSERT(m_map.contains(rootObject));
+        m_map.remove(rootObject);
     }
 
     void remove(RootObject* rootObject, JSObject* jsObject)
index 8ff1cff..067abc3 100644 (file)
@@ -336,10 +336,7 @@ void Node::trackForDebugging()
 Node::~Node()
 {
 #ifndef NDEBUG
-    HashSet<Node*>::iterator it = ignoreSet.find(this);
-    if (it != ignoreSet.end())
-        ignoreSet.remove(it);
-    else
+    if (ignoreSet.remove(this))
         nodeCounter.decrement();
 #endif
 
index 322eb99..f34485e 100644 (file)
@@ -312,11 +312,9 @@ void InspectorProfilerAgent::removeProfile(ErrorString*, const String& type, int
 {
     unsigned uid = static_cast<unsigned>(rawUid);
     if (type == CPUProfileType) {
-        if (m_profiles.contains(uid))
-            m_profiles.remove(uid);
+        m_profiles.remove(uid);
     } else if (type == HeapProfileType) {
-        if (m_snapshots.contains(uid))
-            m_snapshots.remove(uid);
+        m_snapshots.remove(uid);
     }
 }
 
index a6d9a1a..2d0b4d4 100644 (file)
@@ -1311,10 +1311,8 @@ void DocumentLoader::addSubresourceLoader(ResourceLoader* loader)
 
 void DocumentLoader::removeSubresourceLoader(ResourceLoader* loader)
 {
-    ResourceLoaderSet::iterator it = m_subresourceLoaders.find(loader);
-    if (it == m_subresourceLoaders.end())
+    if (!m_subresourceLoaders.remove(loader))
         return;
-    m_subresourceLoaders.remove(it);
     checkLoadComplete();
     if (Frame* frame = m_frame)
         frame->loader().checkLoadComplete();
index 1ed6bf7..1d243b5 100644 (file)
@@ -278,10 +278,8 @@ void ResourceLoadScheduler::HostInformation::addLoadInProgress(ResourceLoader* r
     
 void ResourceLoadScheduler::HostInformation::remove(ResourceLoader* resourceLoader)
 {
-    if (m_requestsLoading.contains(resourceLoader)) {
-        m_requestsLoading.remove(resourceLoader);
+    if (m_requestsLoading.remove(resourceLoader))
         return;
-    }
     
     for (int priority = ResourceLoadPriorityHighest; priority >= ResourceLoadPriorityLowest; --priority) {  
         RequestQueue::iterator end = m_requestsPending[priority].end();
index 83df60b..3cbe132 100644 (file)
@@ -362,10 +362,7 @@ void ApplicationCacheGroup::stopLoading()
 
 void ApplicationCacheGroup::disassociateDocumentLoader(DocumentLoader* loader)
 {
-    HashSet<DocumentLoader*>::iterator it = m_associatedDocumentLoaders.find(loader);
-    if (it != m_associatedDocumentLoaders.end())
-        m_associatedDocumentLoaders.remove(it);
-
+    m_associatedDocumentLoaders.remove(loader);
     m_pendingMasterResourceLoaders.remove(loader);
 
     loader->applicationCacheHost()->setApplicationCache(0); // Will set candidate to 0, too.
@@ -390,11 +387,7 @@ void ApplicationCacheGroup::disassociateDocumentLoader(DocumentLoader* loader)
 
 void ApplicationCacheGroup::cacheDestroyed(ApplicationCache* cache)
 {
-    if (!m_caches.contains(cache))
-        return;
-    
     m_caches.remove(cache);
-
     if (m_caches.isEmpty()) {
         ASSERT(m_associatedDocumentLoaders.isEmpty());
         ASSERT(m_pendingMasterResourceLoaders.isEmpty());
index 1e4cc49..af2bd55 100644 (file)
@@ -175,52 +175,38 @@ static DOMWindowSet& windowsWithBeforeUnloadEventListeners()
 
 static void addUnloadEventListener(DOMWindow* domWindow)
 {
-    DOMWindowSet& set = windowsWithUnloadEventListeners();
-    if (set.add(domWindow).isNewEntry)
+    if (windowsWithUnloadEventListeners().add(domWindow).isNewEntry)
         domWindow->disableSuddenTermination();
 }
 
 static void removeUnloadEventListener(DOMWindow* domWindow)
 {
-    DOMWindowSet& set = windowsWithUnloadEventListeners();
-    DOMWindowSet::iterator it = set.find(domWindow);
-    if (set.remove(it))
+    if (windowsWithUnloadEventListeners().remove(domWindow))
         domWindow->enableSuddenTermination();
 }
 
 static void removeAllUnloadEventListeners(DOMWindow* domWindow)
 {
-    DOMWindowSet& set = windowsWithUnloadEventListeners();
-    DOMWindowSet::iterator it = set.find(domWindow);
-    if (it == set.end())
-        return;
-    set.removeAll(it);
-    domWindow->enableSuddenTermination();
+    if (windowsWithUnloadEventListeners().removeAll(domWindow))
+        domWindow->enableSuddenTermination();
 }
 
 static void addBeforeUnloadEventListener(DOMWindow* domWindow)
 {
-    DOMWindowSet& set = windowsWithBeforeUnloadEventListeners();
-    if (set.add(domWindow).isNewEntry)
+    if (windowsWithBeforeUnloadEventListeners().add(domWindow).isNewEntry)
         domWindow->disableSuddenTermination();
 }
 
 static void removeBeforeUnloadEventListener(DOMWindow* domWindow)
 {
-    DOMWindowSet& set = windowsWithBeforeUnloadEventListeners();
-    DOMWindowSet::iterator it = set.find(domWindow);
-    if (set.remove(it))
+    if (windowsWithBeforeUnloadEventListeners().remove(domWindow))
         domWindow->enableSuddenTermination();
 }
 
 static void removeAllBeforeUnloadEventListeners(DOMWindow* domWindow)
 {
-    DOMWindowSet& set = windowsWithBeforeUnloadEventListeners();
-    DOMWindowSet::iterator it = set.find(domWindow);
-    if (it == set.end())
-        return;
-    set.removeAll(it);
-    domWindow->enableSuddenTermination();
+    if (windowsWithBeforeUnloadEventListeners().removeAll(domWindow))
+        domWindow->enableSuddenTermination();
 }
 
 static bool allowsBeforeUnloadListeners(DOMWindow* window)
index 8f2b261..589d265 100644 (file)
@@ -1562,8 +1562,7 @@ void FrameView::addViewportConstrainedObject(RenderObject* object)
 
 void FrameView::removeViewportConstrainedObject(RenderObject* object)
 {
-    if (m_viewportConstrainedObjects && m_viewportConstrainedObjects->contains(object)) {
-        m_viewportConstrainedObjects->remove(object);
+    if (m_viewportConstrainedObjects && m_viewportConstrainedObjects->remove(object)) {
         if (Page* page = frame().page()) {
             if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
                 scrollingCoordinator->frameViewFixedObjectsDidChange(this);
@@ -4060,22 +4059,12 @@ bool FrameView::addScrollableArea(ScrollableArea* scrollableArea)
 
 bool FrameView::removeScrollableArea(ScrollableArea* scrollableArea)
 {
-    if (!m_scrollableAreas)
-        return false;
-
-    ScrollableAreaSet::iterator it = m_scrollableAreas->find(scrollableArea);
-    if (it == m_scrollableAreas->end())
-        return false;
-
-    m_scrollableAreas->remove(it);
-    return true;
+    return m_scrollableAreas && m_scrollableAreas->remove(scrollableArea);
 }
 
 bool FrameView::containsScrollableArea(ScrollableArea* scrollableArea) const
 {
-    if (!m_scrollableAreas)
-        return false;
-    return m_scrollableAreas->contains(scrollableArea);
+    return m_scrollableAreas && m_scrollableAreas->contains(scrollableArea);
 }
 
 void FrameView::removeChild(Widget* widget)
index 8908e06..dd95d65 100644 (file)
@@ -1410,11 +1410,8 @@ void Page::addRelevantRepaintedObject(RenderObject* object, const LayoutRect& ob
 
     // If this object was previously counted as an unpainted object, remove it from that HashSet
     // and corresponding Region. FIXME: This doesn't do the right thing if the objects overlap.
-    HashSet<RenderObject*>::iterator it = m_relevantUnpaintedRenderObjects.find(object);
-    if (it != m_relevantUnpaintedRenderObjects.end()) {
-        m_relevantUnpaintedRenderObjects.remove(it);
+    if (m_relevantUnpaintedRenderObjects.remove(object))
         m_relevantUnpaintedRegion.subtract(snappedPaintRect);
-    }
 
     // Split the relevantRect into a top half and a bottom half. Making sure we have coverage in
     // both halves helps to prevent cases where we have a fully loaded menu bar or masthead with
index 228c839..4820a81 100644 (file)
@@ -365,11 +365,7 @@ void PageGroup::removeUserScriptsFromWorld(DOMWrapperWorld* world)
     if (!m_userScripts)
         return;
 
-    UserScriptMap::iterator it = m_userScripts->find(world);
-    if (it == m_userScripts->end())
-        return;
-       
-    m_userScripts->remove(it);
+    m_userScripts->remove(world);
 }
 
 void PageGroup::removeUserStyleSheetsFromWorld(DOMWrapperWorld* world)
@@ -378,12 +374,9 @@ void PageGroup::removeUserStyleSheetsFromWorld(DOMWrapperWorld* world)
 
     if (!m_userStyleSheets)
         return;
-    
-    UserStyleSheetMap::iterator it = m_userStyleSheets->find(world);
-    if (it == m_userStyleSheets->end())
+
+    if (!m_userStyleSheets->remove(world))
         return;
-    
-    m_userStyleSheets->remove(it);
 
     invalidateInjectedStyleSheetCacheInAllFrames();
 }
index f5516fa..e2af43e 100644 (file)
@@ -95,8 +95,7 @@ static void clearPeformanceEntries(PerformanceEntryMap& performanceEntryMap, con
         return;
     }
 
-    if (performanceEntryMap.contains(name))
-        performanceEntryMap.remove(name);
+    performanceEntryMap.remove(name);
 }
 
 void UserTiming::mark(const String& markName, ExceptionCode& ec)
index 912d07f..baa4740 100644 (file)
@@ -82,12 +82,7 @@ void DisplayRefreshMonitor::addClient(DisplayRefreshMonitorClient* client)
 
 bool DisplayRefreshMonitor::removeClient(DisplayRefreshMonitorClient* client)
 {
-    DisplayRefreshMonitorClientSet::iterator it = m_clients.find(client);
-    if (it != m_clients.end()) {
-        m_clients.remove(it);
-        return true;
-    }
-    return false;
+    return m_clients.remove(client);
 }
 
 void DisplayRefreshMonitor::displayDidRefresh()
@@ -179,9 +174,8 @@ bool DisplayRefreshMonitorManager::scheduleAnimation(DisplayRefreshMonitorClient
 void DisplayRefreshMonitorManager::displayDidRefresh(DisplayRefreshMonitor* monitor)
 {
     if (monitor->shouldBeTerminated()) {
-        DisplayRefreshMonitorMap::iterator it = m_monitors.find(monitor->displayID());
-        ASSERT(it != m_monitors.end());
-        m_monitors.remove(it);
+        ASSERT(m_monitors.contains(monitor->displayID()));
+        m_monitors.remove(monitor->displayID());
     }
 }
 
index 24a806f..a20d44a 100644 (file)
@@ -510,11 +510,7 @@ void LayerRenderer::addLayer(LayerCompositingThread* layer)
 
 bool LayerRenderer::removeLayer(LayerCompositingThread* layer)
 {
-    LayerSet::iterator iter = m_layers.find(layer);
-    if (iter == m_layers.end())
-        return false;
-    m_layers.remove(layer);
-    return true;
+    return m_layers.remove(layer);
 }
 
 void LayerRenderer::addLayerToReleaseTextureResourcesList(LayerCompositingThread* layer)
index 7088995..d2e1d47 100644 (file)
@@ -85,12 +85,7 @@ void WindowMessageBroadcaster::addListener(WindowMessageListener* listener)
 
 void WindowMessageBroadcaster::removeListener(WindowMessageListener* listener)
 {
-    ListenerSet::iterator found = m_listeners.find(listener);
-    if (found == m_listeners.end())
-        return;
-
-    m_listeners.remove(found);
-
+    m_listeners.remove(listener);
     if (m_listeners.isEmpty())
         destroy();
 }
index 2d17abc..f8cc013 100644 (file)
@@ -371,11 +371,7 @@ void PluginDatabase::clear()
 
 bool PluginDatabase::removeDisabledPluginFile(const String& fileName)
 {
-    if (!m_disabledPluginFiles.contains(fileName))
-        return false;
-
-    m_disabledPluginFiles.remove(fileName);
-    return true;
+    return m_disabledPluginFiles.remove(fileName);
 }
 
 bool PluginDatabase::addDisabledPluginFile(const String& fileName)
index 16b579f..86a724d 100644 (file)
@@ -58,8 +58,7 @@ StyleCustomFilterProgramCache::~StyleCustomFilterProgramCache()
 
 StyleCustomFilterProgram* StyleCustomFilterProgramCache::lookup(const CustomFilterProgramInfo& programInfo) const
 {
-    CacheMap::const_iterator iter = m_cache.find(programInfo);
-    return iter != m_cache.end() ? iter->value : 0;
+    return m_cache.get(programInfo);
 }
 
 StyleCustomFilterProgram* StyleCustomFilterProgramCache::lookup(StyleCustomFilterProgram* program) const
@@ -69,21 +68,17 @@ StyleCustomFilterProgram* StyleCustomFilterProgramCache::lookup(StyleCustomFilte
 
 void StyleCustomFilterProgramCache::add(StyleCustomFilterProgram* program)
 {
-    CustomFilterProgramInfo key = programCacheKey(program);
-    ASSERT(m_cache.find(key) == m_cache.end());
-    m_cache.set(key, program);
+    ASSERT(!m_cache.contains(programCacheKey(program)));
+    m_cache.set(programCacheKey(program), program);
     program->setCache(this);
 }
 
 void StyleCustomFilterProgramCache::remove(StyleCustomFilterProgram* program)
 {
-    CacheMap::iterator iter = m_cache.find(programCacheKey(program));
-    ASSERT(iter != m_cache.end());
-    m_cache.remove(iter);
+    ASSERT(m_cache.contains(programCacheKey(program)));
+    m_cache.remove(programCacheKey(program));
 }
 
-
 } // namespace WebCore
 
 #endif // ENABLE(CSS_SHADERS)
-
index acc940a..fca1429 100644 (file)
@@ -106,11 +106,8 @@ void SVGCursorElement::addClient(SVGElement* element)
 
 void SVGCursorElement::removeClient(SVGElement* element)
 {
-    HashSet<SVGElement*>::iterator it = m_clients.find(element);
-    if (it != m_clients.end()) {
-        m_clients.remove(it);
+    if (m_clients.remove(element))
         element->cursorElementRemoved();
-    }
 }
 
 void SVGCursorElement::removeReferencedElement(SVGElement* element)
index 6b7eca6..9ee6250 100644 (file)
@@ -75,7 +75,7 @@ void SVGDocumentExtensions::addResource(const AtomicString& id, RenderSVGResourc
 
 void SVGDocumentExtensions::removeResource(const AtomicString& id)
 {
-    if (id.isEmpty() || !m_resources.contains(id))
+    if (id.isEmpty())
         return;
 
     m_resources.remove(id);
@@ -322,16 +322,12 @@ void SVGDocumentExtensions::removeAllTargetReferencesForElement(SVGElement* refe
 {
     Vector<SVGElement*> toBeRemoved;
 
-    HashMap<SVGElement*, OwnPtr<HashSet<SVGElement*> > >::iterator end = m_elementDependencies.end();
-    for (HashMap<SVGElement*, OwnPtr<HashSet<SVGElement*> > >::iterator it = m_elementDependencies.begin(); it != end; ++it) {
+    auto end = m_elementDependencies.end();
+    for (auto it = m_elementDependencies.begin(); it != end; ++it) {
         SVGElement* referencedElement = it->key;
-        HashSet<SVGElement*>* referencingElements = it->value.get();
-        HashSet<SVGElement*>::iterator setIt = referencingElements->find(referencingElement);
-        if (setIt == referencingElements->end())
-            continue;
-
-        referencingElements->remove(setIt);
-        if (referencingElements->isEmpty())
+        HashSet<SVGElement*>& referencingElements = *it->value;
+        referencingElements.remove(referencingElement);
+        if (referencingElements.isEmpty())
             toBeRemoved.append(referencedElement);
     }
 
@@ -367,13 +363,7 @@ void SVGDocumentExtensions::rebuildAllElementReferencesForTarget(SVGElement* ref
 
 void SVGDocumentExtensions::removeAllElementReferencesForTarget(SVGElement* referencedElement)
 {
-    ASSERT(referencedElement);
-    HashMap<SVGElement*, OwnPtr<HashSet<SVGElement*> > >::iterator it = m_elementDependencies.find(referencedElement);
-    if (it == m_elementDependencies.end())
-        return;
-    ASSERT(it->key == referencedElement);
-
-    m_elementDependencies.remove(it);
+    m_elementDependencies.remove(referencedElement);
 }
 
 #if ENABLE(SVG_FONTS)
index 7c98e1b..e3a89fd 100644 (file)
@@ -47,8 +47,7 @@ void SVGImageCache::removeClientFromCache(const CachedImageClient* client)
 {
     ASSERT(client);
 
-    if (m_imageForContainerMap.contains(client))
-        m_imageForContainerMap.remove(client);
+    m_imageForContainerMap.remove(client);
 }
 
 void SVGImageCache::setContainerSizeForRenderer(const CachedImageClient* client, const IntSize& containerSize, float containerZoom)
index e5fee5a..60fae5b 100644 (file)
@@ -39,11 +39,10 @@ SVGAnimatedProperty::SVGAnimatedProperty(SVGElement* contextElement, const Quali
 SVGAnimatedProperty::~SVGAnimatedProperty()
 {
     // Remove wrapper from cache.
-    Cache* cache = animatedPropertyCache();
-    const Cache::const_iterator end = cache->end();
-    for (Cache::const_iterator it = cache->begin(); it != end; ++it) {
+    Cache& cache = *animatedPropertyCache();
+    for (auto it = cache.begin(), end = cache.end(); it != end; ++it) {
         if (it->value == this) {
-            cache->remove(it->key);
+            cache.remove(it);
             break;
         }
     }
index 2736e4d..a0ff01d 100644 (file)
@@ -1,5 +1,38 @@
 2013-09-02  Darin Adler  <darin@apple.com>
 
+        Cut down on double hashing and code needlessly using hash table iterators
+        https://bugs.webkit.org/show_bug.cgi?id=120611
+
+        Reviewed by Andreas Kling.
+
+        * Platform/CoreIPC/Connection.cpp:
+        (CoreIPC::Connection::waitForMessage): Use take instead of find/remove.
+
+        * UIProcess/WebPreferences.cpp:
+        (WebKit::WebPreferences::removePageGroup): Use the return value from remove
+        instead of find/remove.
+
+        * WebProcess/Geolocation/GeolocationPermissionRequestManager.cpp:
+        (WebKit::GeolocationPermissionRequestManager::cancelRequestForGeolocation):
+        (WebKit::GeolocationPermissionRequestManager::didReceiveGeolocationPermissionDecision):
+        Use take instead of find/remove.
+
+        * WebProcess/Plugins/Netscape/NetscapePlugin.cpp:
+        (WebKit::NetscapePlugin::frameDidFinishLoading): Use take instead of find/remove.
+        (WebKit::NetscapePlugin::frameDidFail): Use take instead of find/remove.
+
+        * WebProcess/WebPage/WebBackForwardListProxy.cpp:
+        (WebKit::WebBackForwardListProxy::removeItem): Use take instead of find/remove.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::didFinishCheckingText): Use take instead of get/remove so we
+        hash only once.
+        (WebKit::WebPage::didCancelCheckingText): Ditto.
+        (WebKit::WebPage::stopExtendingIncrementalRenderingSuppression): Use the return
+        value from remove instead of contains/remove so we hash only once.
+
+2013-09-02  Darin Adler  <darin@apple.com>
+
         [Mac] No need for HardAutorelease, which is same as CFBridgingRelease
         https://bugs.webkit.org/show_bug.cgi?id=120569
 
index 170ec32..89f06b5 100644 (file)
@@ -417,13 +417,8 @@ PassOwnPtr<MessageDecoder> Connection::waitForMessage(StringReference messageRec
     while (true) {
         MutexLocker locker(m_waitForMessageMutex);
 
-        HashMap<std::pair<std::pair<StringReference, StringReference>, uint64_t>, OwnPtr<MessageDecoder>>::iterator it = m_waitForMessageMap.find(messageAndDestination);
-        if (it->value) {
-            OwnPtr<MessageDecoder> decoder = it->value.release();
-            m_waitForMessageMap.remove(it);
-
+        if (OwnPtr<MessageDecoder> decoder = m_waitForMessageMap.take(messageAndDestination))
             return decoder.release();
-        }
         
         // Now we wait.
         if (!m_waitForMessageCondition.timedWait(m_waitForMessageMutex, absoluteTime)) {
index 4768f28..a4c4b79 100644 (file)
@@ -70,13 +70,8 @@ void WebPreferences::addPageGroup(WebPageGroup* pageGroup)
 
 void WebPreferences::removePageGroup(WebPageGroup* pageGroup)
 {
-    HashSet<WebPageGroup*>::iterator iter = m_pageGroups.find(pageGroup);
-    if (iter == m_pageGroups.end())
-        return;
-
-    m_pageGroups.remove(iter);
-
-    if (privateBrowsingEnabled()) {
+    bool didRemovePageGroup = m_pageGroups.remove(pageGroup);
+    if (didRemovePageGroup && privateBrowsingEnabled()) {
         --privateBrowsingPageGroupCount;
         if (!privateBrowsingPageGroupCount)
             WebContext::willStopUsingPrivateBrowsing();
index 6b05f7f..c40cf66 100644 (file)
@@ -59,7 +59,6 @@ void GeolocationPermissionRequestManager::startRequestForGeolocation(Geolocation
     m_geolocationToIDMap.set(geolocation, geolocationID);
     m_idToGeolocationMap.set(geolocationID, geolocation);
 
-
     Frame* frame = geolocation->frame();
 
     WebFrameLoaderClient* webFrameLoaderClient = toWebFrameLoaderClient(frame->loader().client());
@@ -73,26 +72,20 @@ void GeolocationPermissionRequestManager::startRequestForGeolocation(Geolocation
 
 void GeolocationPermissionRequestManager::cancelRequestForGeolocation(Geolocation* geolocation)
 {
-    GeolocationToIDMap::iterator it = m_geolocationToIDMap.find(geolocation);
-    if (it == m_geolocationToIDMap.end())
+    uint64_t geolocationID = m_geolocationToIDMap.take(geolocation);
+    if (!geolocationID)
         return;
-
-    uint64_t geolocationID = it->value;
-    m_geolocationToIDMap.remove(it);
     m_idToGeolocationMap.remove(geolocationID);
 }
 
 void GeolocationPermissionRequestManager::didReceiveGeolocationPermissionDecision(uint64_t geolocationID, bool allowed)
 {
-    IDToGeolocationMap::iterator it = m_idToGeolocationMap.find(geolocationID);
-    if (it == m_idToGeolocationMap.end())
+    Geolocation* geolocation = m_idToGeolocationMap.take(geolocationID);
+    if (!geolocation)
         return;
+    m_geolocationToIDMap.remove(geolocation);
 
-    Geolocation* geolocation = it->value;
     geolocation->setIsAllowed(allowed);
-
-    m_idToGeolocationMap.remove(it);
-    m_geolocationToIDMap.remove(geolocation);
 }
 
 } // namespace WebKit
index efcf193..2054df9 100644 (file)
@@ -764,32 +764,22 @@ void NetscapePlugin::frameDidFinishLoading(uint64_t requestID)
 {
     ASSERT(m_isStarted);
     
-    PendingURLNotifyMap::iterator it = m_pendingURLNotifications.find(requestID);
-    if (it == m_pendingURLNotifications.end())
+    auto notification = m_pendingURLNotifications.take(requestID);
+    if (notification.first.isEmpty())
         return;
 
-    String url = it->value.first;
-    void* notificationData = it->value.second;
-
-    m_pendingURLNotifications.remove(it);
-    
-    NPP_URLNotify(url.utf8().data(), NPRES_DONE, notificationData);
+    NPP_URLNotify(notification.first.utf8().data(), NPRES_DONE, notification.second);
 }
 
 void NetscapePlugin::frameDidFail(uint64_t requestID, bool wasCancelled)
 {
     ASSERT(m_isStarted);
     
-    PendingURLNotifyMap::iterator it = m_pendingURLNotifications.find(requestID);
-    if (it == m_pendingURLNotifications.end())
+    auto notification = m_pendingURLNotifications.take(requestID);
+    if (notification.first.isNull())
         return;
-
-    String url = it->value.first;
-    void* notificationData = it->value.second;
-
-    m_pendingURLNotifications.remove(it);
     
-    NPP_URLNotify(url.utf8().data(), wasCancelled ? NPRES_USER_BREAK : NPRES_NETWORK_ERR, notificationData);
+    NPP_URLNotify(notification.first.utf8().data(), wasCancelled ? NPRES_USER_BREAK : NPRES_NETWORK_ERR, notification.second);
 }
 
 void NetscapePlugin::didEvaluateJavaScript(uint64_t requestID, const String& result)
index c971e0d..3f91ce1 100644 (file)
@@ -126,14 +126,12 @@ uint64_t WebBackForwardListProxy::idForItem(HistoryItem* item)
 
 void WebBackForwardListProxy::removeItem(uint64_t itemID)
 {
-    IDToHistoryItemMap::iterator it = idToHistoryItemMap().find(itemID);
-    if (it == idToHistoryItemMap().end())
+    RefPtr<HistoryItem> item = idToHistoryItemMap().take(itemID);
+    if (!item)
         return;
         
-    WebCore::pageCache()->remove(it->value.get());
-
-    historyItemToIDMap().remove(it->value);
-    idToHistoryItemMap().remove(it);
+    pageCache()->remove(item.get());
+    historyItemToIDMap().remove(item);
 }
 
 WebBackForwardListProxy::WebBackForwardListProxy(WebPage* page)
index aca18f3..3ad4c40 100644 (file)
@@ -3957,22 +3957,20 @@ void WebPage::addTextCheckingRequest(uint64_t requestID, PassRefPtr<TextChecking
 
 void WebPage::didFinishCheckingText(uint64_t requestID, const Vector<TextCheckingResult>& result)
 {
-    TextCheckingRequest* request = m_pendingTextCheckingRequestMap.get(requestID);
+    RefPtr<TextCheckingRequest> request = m_pendingTextCheckingRequestMap.take(requestID);
     if (!request)
         return;
 
     request->didSucceed(result);
-    m_pendingTextCheckingRequestMap.remove(requestID);
 }
 
 void WebPage::didCancelCheckingText(uint64_t requestID)
 {
-    TextCheckingRequest* request = m_pendingTextCheckingRequestMap.get(requestID);
+    RefPtr<TextCheckingRequest> request = m_pendingTextCheckingRequestMap.take(requestID);
     if (!request)
         return;
 
     request->didCancel();
-    m_pendingTextCheckingRequestMap.remove(requestID);
 }
 
 void WebPage::didCommitLoad(WebFrame* frame)
@@ -4182,10 +4180,9 @@ unsigned WebPage::extendIncrementalRenderingSuppression()
 
 void WebPage::stopExtendingIncrementalRenderingSuppression(unsigned token)
 {
-    if (!m_activeRenderingSuppressionTokens.contains(token))
+    if (!m_activeRenderingSuppressionTokens.remove(token))
         return;
 
-    m_activeRenderingSuppressionTokens.remove(token);
     m_page->mainFrame().view()->setVisualUpdatesAllowedByClient(!shouldExtendIncrementalRenderingSuppression());
 }