Changeset 164947 in webkit

Timestamp:

Mar 2, 2014 12:34:51 PM (10 years ago)

Author:

BJ Burg

Message:

DocumentLoader should keep maps of ResourceLoaders instead of sets
​https://bugs.webkit.org/show_bug.cgi?id=129388

Reviewed by Darin Adler.

For web replay, we need to be able to pull a ResourceLoader instance by
identifier from the DocumentLoader. This is easy to do if we convert
ResourceLoaderSet to ResourceLoaderMap, keyed by the loader's identifier.

Added assertions whenever adding or removing from the map to ensure
that we don't try to add duplicates or resources with zero identifiers.

No new tests required. No functionality was added.

• loader/DocumentLoader.cpp:

(WebCore::cancelAll):
(WebCore::setAllDefersLoading):
(WebCore::areAllLoadersPageCacheAcceptable):
(WebCore::DocumentLoader::addSubresourceLoader):
(WebCore::DocumentLoader::removeSubresourceLoader):
(WebCore::DocumentLoader::addPlugInStreamLoader):
(WebCore::DocumentLoader::removePlugInStreamLoader):
(WebCore::DocumentLoader::subresourceLoaderFinishedLoadingOnePart):

• loader/DocumentLoader.h:
• loader/NetscapePlugInStreamLoader.cpp:

(WebCore::NetscapePlugInStreamLoader::create): Only add the loader
to the document loader's map if it initialized successfully.
The old code was probably leaking resource loaders that failed to
initialize.

• loader/mac/DocumentLoaderMac.cpp:

(WebCore::scheduleAll):
(WebCore::unscheduleAll):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/DocumentLoader.cpp (modified) (4 diffs)
• loader/DocumentLoader.h (modified) (2 diffs)
• loader/NetscapePlugInStreamLoader.cpp (modified) (1 diff)
• loader/mac/DocumentLoaderMac.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-03-02  Brian Burg  <bburg@apple.com>
+
+        DocumentLoader should keep maps of ResourceLoaders instead of sets
+        https://bugs.webkit.org/show_bug.cgi?id=129388
+
+        Reviewed by Darin Adler.
+
+        For web replay, we need to be able to pull a ResourceLoader instance by
+        identifier from the DocumentLoader. This is easy to do if we convert
+        ResourceLoaderSet to ResourceLoaderMap, keyed by the loader's identifier.
+
+        Added assertions whenever adding or removing from the map to ensure
+        that we don't try to add duplicates or resources with zero identifiers.
+
+        No new tests required. No functionality was added.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::cancelAll):
+        (WebCore::setAllDefersLoading):
+        (WebCore::areAllLoadersPageCacheAcceptable):
+        (WebCore::DocumentLoader::addSubresourceLoader):
+        (WebCore::DocumentLoader::removeSubresourceLoader):
+        (WebCore::DocumentLoader::addPlugInStreamLoader):
+        (WebCore::DocumentLoader::removePlugInStreamLoader):
+        (WebCore::DocumentLoader::subresourceLoaderFinishedLoadingOnePart):
+        * loader/DocumentLoader.h:
+        * loader/NetscapePlugInStreamLoader.cpp:
+        (WebCore::NetscapePlugInStreamLoader::create): Only add the loader
+        to the document loader's map if it initialized successfully.
+        The old code was probably leaking resource loaders that failed to
+        initialize.
+
+        * loader/mac/DocumentLoaderMac.cpp:
+        (WebCore::scheduleAll):
+        (WebCore::unscheduleAll):
+
 2014-03-02  Dirkjan Ochtman  <d.ochtman@activevideo.com>
 

trunk/Source/WebCore/loader/DocumentLoader.cpp

@@ -79 +79 @@
 namespace WebCore {
 
-static void cancelAll(const ResourceLoaderSet& loaders)
+static void cancelAll(const ResourceLoaderMap& loaders)
 {
     Vector<RefPtr<ResourceLoader>> loadersCopy;
-    copyToVector(loaders, loadersCopy);
-    size_t size = loadersCopy.size();
-    for (size_t i = 0; i < size; ++i)
-        loadersCopy[i]->cancel();
-}
-
-static void setAllDefersLoading(const ResourceLoaderSet& loaders, bool defers)
+    copyValuesToVector(loaders, loadersCopy);
+    for (auto& loader : loadersCopy)
+        loader->cancel();
+}
+
+static void setAllDefersLoading(const ResourceLoaderMap& loaders, bool defers)
 {
     Vector<RefPtr<ResourceLoader>> loadersCopy;
-    copyToVector(loaders, loadersCopy);
-    size_t size = loadersCopy.size();
-    for (size_t i = 0; i < size; ++i)
-        loadersCopy[i]->setDefersLoading(defers);
-}
-
-static bool areAllLoadersPageCacheAcceptable(const ResourceLoaderSet& loaders)
+    copyValuesToVector(loaders, loadersCopy);
+    for (auto& loader : loadersCopy)
+        loader->setDefersLoading(defers);
+}
+
+static bool areAllLoadersPageCacheAcceptable(const ResourceLoaderMap& loaders)
 {
     Vector<RefPtr<ResourceLoader>> loadersCopy;
-    copyToVector(loaders, loadersCopy);
+    copyValuesToVector(loaders, loadersCopy);
     for (auto& loader : loadersCopy) {
         ResourceHandle* handle = loader->handle();
@@ -1339 +1337 @@
     if (!m_gotFirstByte)
         return;
-    ASSERT(!m_subresourceLoaders.contains(loader));
+    ASSERT(loader->identifier());
+    ASSERT(!m_subresourceLoaders.contains(loader->identifier()));
     ASSERT(!mainResourceLoader() || mainResourceLoader() != loader);
-    m_subresourceLoaders.add(loader);
+
+    m_subresourceLoaders.add(loader->identifier(), loader);
 }
 
 void DocumentLoader::removeSubresourceLoader(ResourceLoader* loader)
 {
-    if (!m_subresourceLoaders.remove(loader))
+    ASSERT(loader->identifier());
+
+    if (!m_subresourceLoaders.remove(loader->identifier()))
         return;
     checkLoadComplete();
@@ -1355 +1357 @@
 void DocumentLoader::addPlugInStreamLoader(ResourceLoader* loader)
 {
-    m_plugInStreamLoaders.add(loader);
+    ASSERT(loader->identifier());
+    ASSERT(!m_plugInStreamLoaders.contains(loader->identifier()));
+
+    m_plugInStreamLoaders.add(loader->identifier(), loader);
 }
 
 void DocumentLoader::removePlugInStreamLoader(ResourceLoader* loader)
 {
-    m_plugInStreamLoaders.remove(loader);
+    ASSERT(loader->identifier());
+    ASSERT(loader == m_plugInStreamLoaders.get(loader->identifier()));
+
+    m_plugInStreamLoaders.remove(loader->identifier());
     checkLoadComplete();
 }
@@ -1485 +1493 @@
 void DocumentLoader::subresourceLoaderFinishedLoadingOnePart(ResourceLoader* loader)
 {
-    m_multipartSubresourceLoaders.add(loader);
-    m_subresourceLoaders.remove(loader);
+    ASSERT(loader->identifier());
+    ASSERT(!m_multipartSubresourceLoaders.contains(loader->identifier()));
+    ASSERT(m_subresourceLoaders.contains(loader->identifier()));
+
+    m_multipartSubresourceLoaders.add(loader->identifier(), loader);
+    m_subresourceLoaders.remove(loader->identifier());
     checkLoadComplete();
     if (Frame* frame = m_frame)

trunk/Source/WebCore/loader/DocumentLoader.h

@@ -75 +75 @@
     class SubstituteResource;
 
-    typedef HashSet<RefPtr<ResourceLoader>> ResourceLoaderSet;
+    typedef HashMap<unsigned long, RefPtr<ResourceLoader>> ResourceLoaderMap;
     typedef Vector<ResourceResponse> ResponseVector;
 
@@ -332 +332 @@
 
         CachedResourceHandle<CachedRawResource> m_mainResource;
-        ResourceLoaderSet m_subresourceLoaders;
-        ResourceLoaderSet m_multipartSubresourceLoaders;
-        ResourceLoaderSet m_plugInStreamLoaders;
+        ResourceLoaderMap m_subresourceLoaders;
+        ResourceLoaderMap m_multipartSubresourceLoaders;
+        ResourceLoaderMap m_plugInStreamLoaders;
         
         mutable DocumentWriter m_writer;

trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.cpp

@@ -50 +50 @@
 {
     RefPtr<NetscapePlugInStreamLoader> loader(adoptRef(new NetscapePlugInStreamLoader(frame, client)));
-    loader->documentLoader()->addPlugInStreamLoader(loader.get());
     if (!loader->init(request))
         return 0;
 
+    loader->documentLoader()->addPlugInStreamLoader(loader.get());
     return loader.release();
 }

trunk/Source/WebCore/loader/mac/DocumentLoaderMac.cpp

@@ -36 +36 @@
 
 #if !PLATFORM(IOS)
-static void scheduleAll(const ResourceLoaderSet& loaders, SchedulePair* pair)
+static void scheduleAll(const ResourceLoaderMap& loaders, SchedulePair* pair)
 {
-    const ResourceLoaderSet copy = loaders;
-    ResourceLoaderSet::const_iterator end = copy.end();
-    for (ResourceLoaderSet::const_iterator it = copy.begin(); it != end; ++it)
-        if (ResourceHandle* handle = (*it)->handle())
+    Vector<RefPtr<ResourceLoader>> loadersCopy;
+    copyValuesToVector(loaders, loadersCopy);
+    for (auto& loader : loadersCopy) {
+        if (ResourceHandle* handle = loader->handle())
             handle->schedule(pair);
+    }
 }
 
-static void unscheduleAll(const ResourceLoaderSet& loaders, SchedulePair* pair)
+static void unscheduleAll(const ResourceLoaderMap& loaders, SchedulePair* pair)
 {
-    const ResourceLoaderSet copy = loaders;
-    ResourceLoaderSet::const_iterator end = copy.end();
-    for (ResourceLoaderSet::const_iterator it = copy.begin(); it != end; ++it)
-        if (ResourceHandle* handle = (*it)->handle())
+    Vector<RefPtr<ResourceLoader>> loadersCopy;
+    copyValuesToVector(loaders, loadersCopy);
+    for (auto& loader : loadersCopy) {
+        if (ResourceHandle* handle = loader->handle())
             handle->unschedule(pair);
+    }
 }
 #endif