Refactoring starting a resource load

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

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 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
     License as published by the Free Software Foundation; either
@@ skipping 11 matching lines @@
 */
 
 #include "core/fetch/Resource.h"
 
 #include "core/fetch/CachedMetadata.h"
 #include "core/fetch/CrossOriginAccessControl.h"
 #include "core/fetch/FetchInitiatorTypeNames.h"
 #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"
 #include "platform/Logging.h"
 #include "platform/SharedBuffer.h"
 #include "platform/TraceEvent.h"
 #include "platform/network/HTTPParsers.h"
 #include "platform/weborigin/KURL.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebProcessMemoryDump.h"
@@ skipping 201 matching lines @@
 {
     InspectorInstrumentation::removedResourceFromMemoryCache(this);
 }
 
 DEFINE_TRACE(Resource)
 {
     visitor->trace(m_loader);
     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()
 {
     if (isLoading())
         return;
 
     ResourceClientWalker<ResourceClient> w(m_clients);
     while (ResourceClient* c = w.next())
         c->notifyFinished(this);
@@ skipping 790 matching lines @@
     case Resource::Media:
         return "Media";
     case Resource::Manifest:
         return "Manifest";
     }
     ASSERT_NOT_REACHED();
     return "Unknown";
 }
 
 } // namespace blink