Remove FetchContext::DidLoadResource

Remove FetchContext::DidLoadResource and move the operations in it
to DispatchDidFinishLoading and DispatchDidFail.

This leads to code duplication but I think this change is overall a
good change because:
 - DidLoadResource is called only twice. Repeating yourself twice is
   not that bad.
 - DidLoadResource is confusing; Whether that precedes or succeeds
   DispatchDidFinishLoading, whether it should be called in
   DispatchDidFail, etc.

Previously FetchContext::DispatchDidFinishLoading and DispatchDidFail
were called before Resource::Finish, but now they are called after
Resource::Finish. That affected a few devtools tests because some tests
finished as soon as resource loading finishes and some error messages
got missing with the new semantics. This CL delays test completion a
bit in order to keep the expectations.

Bug: 914739
Change-Id: I5245dca240ebc7496d1de51909d458a6ed5a61d2
Reviewed-on: https://chromium-review.googlesource.com/c/1474887
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636660}

third_party/blink/renderer/core/loader/document_loader.cc

@@ -533,7 +533,8 @@ void DocumentLoader::BodyLoadingFinished(
   if (!error) {
     fetcher_->Context().DispatchDidFinishLoading(
         main_resource_identifier_, completion_time, total_encoded_data_length,
-        total_decoded_body_length, should_report_corb_blocking);
+        total_decoded_body_length, should_report_corb_blocking,
+        FetchContext::ResourceResponseType::kNotFromMemoryCache);
     if (response_.IsHTTP()) {
       navigation_timing_info_->SetFinalResponse(response_);
       navigation_timing_info_->AddFinalTransferSize(

third_party/blink/renderer/core/loader/frame_fetch_context.cc

@@ -91,6 +91,7 @@
 #include "third_party/blink/renderer/core/timing/dom_window_performance.h"
 #include "third_party/blink/renderer/core/timing/performance.h"
 #include "third_party/blink/renderer/core/timing/window_performance.h"
+#include "third_party/blink/renderer/platform/bindings/script_forbidden_scope.h"
 #include "third_party/blink/renderer/platform/bindings/v8_dom_activity_logger.h"
 #include "third_party/blink/renderer/platform/exported/wrapped_resource_request.h"
 #include "third_party/blink/renderer/platform/histogram.h"
@@ -580,7 +581,8 @@ void FrameFetchContext::DispatchDidFinishLoading(
     TimeTicks finish_time,
     int64_t encoded_data_length,
     int64_t decoded_body_length,
-    bool should_report_corb_blocking) {
+    bool should_report_corb_blocking,
+    ResourceResponseType response_type) {
   if (GetResourceFetcherProperties().IsDetached())
     return;
 
@@ -588,13 +590,25 @@ void FrameFetchContext::DispatchDidFinishLoading(
   probe::didFinishLoading(Probe(), identifier, MasterDocumentLoader(),
                           finish_time, encoded_data_length, decoded_body_length,
                           should_report_corb_blocking);
-  if (frame_or_imported_document_->GetDocument()) {
-    InteractiveDetector* interactive_detector(
-        InteractiveDetector::From(*frame_or_imported_document_->GetDocument()));
-    if (interactive_detector) {
-      interactive_detector->OnResourceLoadEnd(finish_time);
+
+  Document* document = frame_or_imported_document_->GetDocument();
+  if (!document) {
+    return;
+  }
+
+  if (auto* interactive_detector = InteractiveDetector::From(*document)) {
+    interactive_detector->OnResourceLoadEnd(finish_time);
+  }
+
+  if (LocalFrame* frame = document->GetFrame()) {
+    if (IdlenessDetector* idleness_detector = frame->GetIdlenessDetector()) {
+      idleness_detector->OnDidLoadResource();
     }
   }
+
+  if (response_type == ResourceResponseType::kNotFromMemoryCache) {
+    document->CheckCompleted();
+  }
 }
 
 void FrameFetchContext::DispatchDidFail(const KURL& url,
@@ -616,21 +630,28 @@ void FrameFetchContext::DispatchDidFail(const KURL& url,
 
   GetFrame()->Loader().Progress().CompleteProgress(identifier);
   probe::didFailLoading(Probe(), identifier, MasterDocumentLoader(), error);
-  if (frame_or_imported_document_->GetDocument()) {
-    InteractiveDetector* interactive_detector(
-        InteractiveDetector::From(*frame_or_imported_document_->GetDocument()));
-    if (interactive_detector) {
-      // We have not yet recorded load_finish_time. Pass nullopt here; we will
-      // call CurrentTimeTicksInSeconds lazily when we need it.
-      interactive_detector->OnResourceLoadEnd(base::nullopt);
-    }
-  }
   // Notification to FrameConsole should come AFTER InspectorInstrumentation
   // call, DevTools front-end relies on this.
   if (!is_internal_request) {
     GetFrame()->Console().DidFailLoading(MasterDocumentLoader(), identifier,
                                          error);
   }
+  Document* document = frame_or_imported_document_->GetDocument();
+  if (!document) {
+    return;
+  }
+
+  if (auto* interactive_detector = InteractiveDetector::From(*document)) {
+    // We have not yet recorded load_finish_time. Pass nullopt here; we will
+    // call CurrentTimeTicksInSeconds lazily when we need it.
+    interactive_detector->OnResourceLoadEnd(base::nullopt);
+  }
+  if (LocalFrame* frame = document->GetFrame()) {
+    if (IdlenessDetector* idleness_detector = frame->GetIdlenessDetector()) {
+      idleness_detector->OnDidLoadResource();
+    }
+  }
+  document->CheckCompleted();
 }
 
 void FrameFetchContext::RecordLoadingActivity(
@@ -656,21 +677,6 @@ void FrameFetchContext::RecordLoadingActivity(
   }
 }
 
-void FrameFetchContext::DidLoadResource(Resource* resource) {
-  if (GetResourceFetcherProperties().IsDetached() ||
-      !frame_or_imported_document_->GetDocument())
-    return;
-  if (LocalFrame* local_frame =
-          frame_or_imported_document_->GetDocument()->GetFrame()) {
-    if (IdlenessDetector* idleness_detector =
-            local_frame->GetIdlenessDetector()) {
-      idleness_detector->OnDidLoadResource();
-    }
-  }
-
-  frame_or_imported_document_->GetDocument()->CheckCompleted();
-}
-
 void FrameFetchContext::DidObserveLoadingBehavior(
     WebLoadingBehaviorFlag behavior) {
   if (GetDocumentLoader())

third_party/blink/renderer/core/loader/frame_fetch_context.h

@@ -115,7 +115,8 @@ class CORE_EXPORT FrameFetchContext final : public BaseFetchContext {
                                 TimeTicks finish_time,
                                 int64_t encoded_data_length,
                                 int64_t decoded_body_length,
-                                bool should_report_corb_blocking) override;
+                                bool should_report_corb_blocking,
+                                ResourceResponseType) override;
   void DispatchDidFail(const KURL&,
                        unsigned long identifier,
                        const ResourceError&,
@@ -125,7 +126,6 @@ class CORE_EXPORT FrameFetchContext final : public BaseFetchContext {
   void RecordLoadingActivity(const ResourceRequest&,
                              ResourceType,
                              const AtomicString& fetch_initiator_name) override;
-  void DidLoadResource(Resource*) override;
   void DidObserveLoadingBehavior(WebLoadingBehaviorFlag) override;
 
   void AddResourceTiming(const ResourceTimingInfo&) override;

third_party/blink/renderer/core/loader/frame_fetch_context_test.cc

@@ -1305,7 +1305,8 @@ TEST_F(FrameFetchContextTest, DispatchDidFinishLoadingWhenDetached) {
   dummy_page_holder = nullptr;
 
   GetFetchContext()->DispatchDidFinishLoading(
-      4, base::TimeTicks() + base::TimeDelta::FromSecondsD(0.3), 8, 10, false);
+      4, base::TimeTicks() + base::TimeDelta::FromSecondsD(0.3), 8, 10, false,
+      FetchContext::ResourceResponseType::kNotFromMemoryCache);
   // Should not crash.
 }
 
@@ -1339,17 +1340,6 @@ TEST_F(FrameFetchContextTest, RecordLoadingActivityWhenDetached) {
   // Should not crash.
 }
 
-TEST_F(FrameFetchContextTest, DidLoadResourceWhenDetached) {
-  ResourceRequest request(KURL("https://www.example.com/"));
-  request.SetFetchCredentialsMode(network::mojom::FetchCredentialsMode::kOmit);
-  Resource* resource = MockResource::Create(request);
-
-  dummy_page_holder = nullptr;
-
-  GetFetchContext()->DidLoadResource(resource);
-  // Should not crash.
-}
-
 TEST_F(FrameFetchContextTest, AddResourceTimingWhenDetached) {
   scoped_refptr<ResourceTimingInfo> info = ResourceTimingInfo::Create(
       "type", base::TimeTicks() + base::TimeDelta::FromSecondsD(0.3));

third_party/blink/renderer/core/loader/worker_fetch_context.cc

@@ -266,7 +266,8 @@ void WorkerFetchContext::DispatchDidFinishLoading(
     TimeTicks finish_time,
     int64_t encoded_data_length,
     int64_t decoded_body_length,
-    bool should_report_corb_blocking) {
+    bool should_report_corb_blocking,
+    ResourceResponseType) {
   probe::didFinishLoading(Probe(), identifier, nullptr, finish_time,
                           encoded_data_length, decoded_body_length,
                           should_report_corb_blocking);

third_party/blink/renderer/core/loader/worker_fetch_context.h

@@ -97,7 +97,8 @@ class WorkerFetchContext final : public BaseFetchContext {
                                 TimeTicks finish_time,
                                 int64_t encoded_data_length,
                                 int64_t decoded_body_length,
-                                bool should_report_corb_blocking) override;
+                                bool should_report_corb_blocking,
+                                ResourceResponseType) override;
   void DispatchDidFail(const KURL&,
                        unsigned long identifier,
                        const ResourceError&,

third_party/blink/renderer/platform/loader/fetch/fetch_context.cc

@@ -109,7 +109,8 @@ void FetchContext::DispatchDidFinishLoading(unsigned long,
                                             TimeTicks,
                                             int64_t,
                                             int64_t,
-                                            bool) {}
+                                            bool,
+                                            ResourceResponseType) {}
 
 void FetchContext::DispatchDidFail(const KURL&,
                                    unsigned long,
@@ -126,8 +127,6 @@ void FetchContext::RecordLoadingActivity(
     ResourceType,
     const AtomicString& fetch_initiator_name) {}
 
-void FetchContext::DidLoadResource(Resource*) {}
-
 void FetchContext::DidObserveLoadingBehavior(WebLoadingBehaviorFlag) {}
 
 void FetchContext::AddResourceTiming(const ResourceTimingInfo&) {}

third_party/blink/renderer/platform/loader/fetch/fetch_context.h

@@ -156,7 +156,8 @@ class PLATFORM_EXPORT FetchContext
                                         TimeTicks finish_time,
                                         int64_t encoded_data_length,
                                         int64_t decoded_body_length,
-                                        bool should_report_corb_blocking);
+                                        bool should_report_corb_blocking,
+                                        ResourceResponseType);
   virtual void DispatchDidFail(const KURL&,
                                unsigned long identifier,
                                const ResourceError&,
@@ -171,7 +172,6 @@ class PLATFORM_EXPORT FetchContext
                                      ResourceType,
                                      const AtomicString& fetch_initiator_name);
 
-  virtual void DidLoadResource(Resource*);
   virtual void DidObserveLoadingBehavior(WebLoadingBehaviorFlag);
 
   virtual void AddResourceTiming(const ResourceTimingInfo&);

third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc

@@ -607,7 +607,7 @@ void ResourceFetcher::DidLoadResourceFromMemoryCache(
 
   Context().DispatchDidFinishLoading(
       identifier, TimeTicks(), 0, resource->GetResponse().DecodedBodyLength(),
-      false);
+      false, FetchContext::ResourceResponseType::kFromMemoryCache);
 }
 
 Resource* ResourceFetcher::ResourceForStaticData(
@@ -1686,12 +1686,6 @@ ArchiveResource* ResourceFetcher::CreateArchive(
   return archive_->MainResource();
 }
 
-void ResourceFetcher::HandleLoadCompletion(Resource* resource) {
-  Context().DidLoadResource(resource);
-
-  resource->ReloadIfLoFiOrPlaceholderImage(this, Resource::kReloadIfNeeded);
-}
-
 void ResourceFetcher::HandleLoaderFinish(
     Resource* resource,
     TimeTicks finish_time,
@@ -1770,10 +1764,6 @@ void ResourceFetcher::HandleLoaderFinish(
   }
 
   resource->VirtualTimePauser().UnpauseVirtualTime();
-  Context().DispatchDidFinishLoading(
-      resource->Identifier(), finish_time, encoded_data_length,
-      resource->GetResponse().DecodedBodyLength(), should_report_corb_blocking);
-
   if (type == kDidFinishLoading) {
     resource->Finish(finish_time, task_runner_.get());
 
@@ -1804,8 +1794,11 @@ void ResourceFetcher::HandleLoaderFinish(
       context_->DidObserveLoadingBehavior(behavior);
     }
   }
-
-  HandleLoadCompletion(resource);
+  Context().DispatchDidFinishLoading(
+      resource->Identifier(), finish_time, encoded_data_length,
+      resource->GetResponse().DecodedBodyLength(), should_report_corb_blocking,
+      FetchContext::ResourceResponseType::kNotFromMemoryCache);
+  resource->ReloadIfLoFiOrPlaceholderImage(this, Resource::kReloadIfNeeded);
 }
 
 void ResourceFetcher::HandleLoaderError(Resource* resource,
@@ -1824,15 +1817,13 @@ void ResourceFetcher::HandleLoaderError(Resource* resource,
                              fetch_initiator_type_names::kInternal;
 
   resource->VirtualTimePauser().UnpauseVirtualTime();
-  Context().DispatchDidFail(
-      resource->LastResourceRequest().Url(), resource->Identifier(), error,
-      resource->GetResponse().EncodedDataLength(), is_internal_request);
-
   if (error.IsCancellation())
     RemovePreload(resource);
   resource->FinishAsError(error, task_runner_.get());
-
-  HandleLoadCompletion(resource);
+  Context().DispatchDidFail(
+      resource->LastResourceRequest().Url(), resource->Identifier(), error,
+      resource->GetResponse().EncodedDataLength(), is_internal_request);
+  resource->ReloadIfLoFiOrPlaceholderImage(this, Resource::kReloadIfNeeded);
 }
 
 void ResourceFetcher::MoveResourceLoaderToNonBlocking(ResourceLoader* loader) {

third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h

@@ -321,7 +321,6 @@ class PLATFORM_EXPORT ResourceFetcher
                                                 const FetchParameters&);
   void MoveResourceLoaderToNonBlocking(ResourceLoader*);
   void RemoveResourceLoader(ResourceLoader*);
-  void HandleLoadCompletion(Resource*);
 
   void RequestLoadStarted(unsigned long identifier,
                           Resource*,

third_party/blink/web_tests/http/tests/inspector-protocol/resources/interception-test.js

@@ -126,9 +126,13 @@
     });
 
     this._session.protocol.Page.onFrameStoppedLoading(() => {
-      frameStoppedLoading = true;
-      this._log(this._getNextId(), 'Page.frameStoppedLoading');
-      maybeCompleteTest();
+      // We want to see errors that might stop frame loading, so we delay
+      // completion a bit.
+      setTimeout(() => {
+        frameStoppedLoading = true;
+        this._log(this._getNextId(), 'Page.frameStoppedLoading');
+        maybeCompleteTest();
+      }, 0);
     });
 
     this._testRunner.log('Test started');