Refactoring starting a resource load

third_party/WebKit/Source/core/fetch/Resource.cpp

Index: third_party/WebKit/Source/core/fetch/Resource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/Resource.cpp b/third_party/WebKit/Source/core/fetch/Resource.cpp
index 50c6fed7a8020ca05270af05b5652ff6fdb0f03d..c36db8784820e0bba410d96b4ea16087944a66c7 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -29,7 +29,6 @@
 #include "core/fetch/MemoryCache.h"
 #include "core/fetch/ResourceClient.h"
 #include "core/fetch/ResourceClientOrObserverWalker.h"
-#include "core/fetch/ResourceFetcher.h"
 #include "core/fetch/ResourceLoader.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #include "core/inspector/InstanceCounters.h"
@@ -251,27 +250,14 @@ DEFINE_TRACE(Resource)
     visitor->trace(m_cacheHandler);
 }
 
-void Resource::load(ResourceFetcher* fetcher)
+ResourceRequest& Resource::prepareRequestForLoadStart()

[hiroshige, 2016/04/21 08:27:40]

I'm not sure whether it is good to split load() into
prepareRequestForLoadStart(), setLoader() and a part of
ResourceFetcher::startLoad():
- All of them should be called only when start loading and interact with each
other.
- prepareRequestForLoadStart() exposes a mutable reference to
|m_resourceRequest|.

How about keeping the original Resource::load() and adding |loadingTaskRunner|
and |defersLoading| parameters to Resource::load()?

(In this case, it would be better if we can make Resource::load() only callable
from ResourceFetcher, but I feel we can just add comments and/or rename it to
startLoadInternal() or something that doesn't look callable from everywhere)

[Nate Chapin, 2016/05/18 23:56:04]

I refactored a bit more to enable this to be just requestForLoadStart() and be
const. It adds some complexity to ResourceFetcher::requestResource(), but I
think it's probably(?) cleaner on the whole.

 {
-    // TOOD(japhet): Temporary, out of place hack to stop a top crasher.
-    // Make this more organic.
-    if (!fetcher->loadingTaskRunner())

[hiroshige, 2016/04/21 08:27:40]

Is this check removed?
(Or loadingTaskRunner() is guaranteed to be non-null when
shouldLoadNewResource() is true?)

[Nate Chapin, 2016/05/18 23:56:04]

Correct, loadingTaskRunner() being null and shouldLoadNewResource() returning
false should be the same case.

-        return;
-
     RELEASE_ASSERT(!m_loader);
-    ASSERT(stillNeedsLoad());
     m_status = Pending;
 
     ResourceRequest& request(m_revalidatingRequest.isNull() ? m_resourceRequest : m_revalidatingRequest);
-    KURL url = request.url();
     request.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
-
-    m_loader = ResourceLoader::create(fetcher, this);
-    m_loader->start(request);
-    // If the request reference is null (i.e., a synchronous revalidation will
-    // null the request), don't make the request non-null by setting the url.
-    if (!request.isNull())
-        request.setURL(url);
+    return request;
 }
 
 void Resource::checkNotify()