Implement SetDefersLoading on content::URLLoaderClientImpl

content/child/url_loader_client_impl.cc

Index: content/child/url_loader_client_impl.cc
diff --git a/content/child/url_loader_client_impl.cc b/content/child/url_loader_client_impl.cc
index b9305043fae0a586cf2372a13f4f6751469a63b0..912c7cc2cece17e568ffc7207726d0c1a8d8e57e 100644
--- a/content/child/url_loader_client_impl.cc
+++ b/content/child/url_loader_client_impl.cc
@@ -21,11 +21,13 @@ URLLoaderClientImpl::URLLoaderClientImpl(
     : binding_(this),
       request_id_(request_id),
       resource_dispatcher_(resource_dispatcher),
+      is_destroyed_(new base::RefCountedData<bool>(false)),
       task_runner_(std::move(task_runner)) {}
 
 URLLoaderClientImpl::~URLLoaderClientImpl() {
   if (body_consumer_)
     body_consumer_->Cancel();
+  is_destroyed_->data = true;
 }
 
 void URLLoaderClientImpl::Bind(
@@ -34,6 +36,81 @@ void URLLoaderClientImpl::Bind(
   binding_.Bind(client_ptr_info, associated_group);
 }
 
+void URLLoaderClientImpl::SetDefersLoading() {
+  is_deferred_ = true;
+  if (body_consumer_)
+    body_consumer_->SetDefersLoading();
+}
+
+void URLLoaderClientImpl::UnsetDefersLoading() {
+  is_deferred_ = false;
+}
+
+void URLLoaderClientImpl::FlushDeferredMessages() {
+  DCHECK(!is_deferred_);
+  std::vector<IPC::Message> messages;
+  messages.swap(deferred_messages_);
+  bool has_completion_message = false;
+  scoped_refptr<base::RefCountedData<bool>> is_destroyed = is_destroyed_;
+  size_t index = 0;
+  // First, dispatch all messages excluding the followings:
+  //  - response body (dispatched by |body_consumer_|)
+  //  - transfer size change (dispatched later)
+  //  - completion (dispatched by |body_consumer_| or dispatched later)
+  for (index = 0; index < messages.size(); ++index) {
+    if (messages[index].type() == ResourceMsg_RequestComplete::ID) {
+      // The completion message arrives at the end of the message queue.
+      DCHECK(!has_completion_message);
+      DCHECK_EQ(index, messages.size() - 1);
+      has_completion_message = true;
+    } else {
+      Dispatch(messages[index]);
+    }
+    if (is_destroyed->data)

[kinuko, 2016/12/23 13:11:23]

Looking into the usage, I think using WeakPtr is probably more common?  (Have a
WeakPtrFactory, get a WeakPtr of this at the beginning and check if it's null
later)

[yhirano, 2016/12/26 05:08:35]

Done.

+      return;
+    if (is_deferred_) {
+      deferred_messages_.insert(deferred_messages_.begin(),
+                                messages.begin() + index + 1, messages.end());
+      return;
+    }
+  }
+
+  // Dispatch the transfer size update.
+  if (accumulated_transfer_size_diff_during_deferred_ > 0) {
+    auto transfer_size_diff = accumulated_transfer_size_diff_during_deferred_;
+    accumulated_transfer_size_diff_during_deferred_ = 0;
+    resource_dispatcher_->OnTransferSizeUpdated(request_id_,
+                                                transfer_size_diff);
+    if (is_destroyed->data)
+      return;
+    if (is_deferred_) {
+      deferred_messages_.insert(deferred_messages_.begin(),
+                                messages.begin() + index, messages.end());

[kinuko, 2016/12/23 13:11:23]

I'm a little confused, we don't break the for loop (except for returning in the
middle), isn't the index always at the end of the messages after the for loop?

[yhirano, 2016/12/26 05:08:35]

Oops, sorry. Fixed.

[kinuko, 2016/12/26 07:21:17]

Thanks. Should we have a test that must have caught a bug like this?

Let me make sure... so basically we're inserting ResourceMsg_RequestComplete
which must be messages.back(), am I following?  I feel just pushing back the
last one might be clearer, but what do you think?

[yhirano, 2016/12/26 08:22:00]

Done.

+      return;
+    }
+  }
+
+  if (body_consumer_) {
+    // Dispatch the response body.
+    body_consumer_->UnsetDefersLoading();
+    if (is_destroyed->data)
+      return;
+    if (is_deferred_) {
+      deferred_messages_.insert(deferred_messages_.begin(),
+                                messages.begin() + index, messages.end());
+      return;
+    }
+  }
+
+  // Dispatch the completion message.
+  if (has_completion_message) {
+    DCHECK_GT(messages.size(), 0u);
+    DCHECK_EQ(messages.back().type(),
+              static_cast<uint32_t>(ResourceMsg_RequestComplete::ID));
+    Dispatch(messages.back());
+  }
+}
+
 void URLLoaderClientImpl::OnReceiveResponse(
     const ResourceResponseHead& response_head,
     mojom::DownloadedTempFilePtr downloaded_file) {
@@ -59,8 +136,12 @@ void URLLoaderClientImpl::OnDataDownloaded(int64_t data_len,
 }
 
 void URLLoaderClientImpl::OnTransferSizeUpdated(int32_t transfer_size_diff) {
-  resource_dispatcher_->OnTransferSizeUpdated(request_id_,
-                                              transfer_size_diff);
+  if (is_deferred_) {
+    accumulated_transfer_size_diff_during_deferred_ += transfer_size_diff;
+  } else {
+    resource_dispatcher_->OnTransferSizeUpdated(request_id_,
+                                                transfer_size_diff);
+  }
 }
 
 void URLLoaderClientImpl::OnStartLoadingResponseBody(
@@ -70,6 +151,8 @@ void URLLoaderClientImpl::OnStartLoadingResponseBody(
       request_id_, resource_dispatcher_, std::move(body), task_runner_);
   if (has_received_response_)
     body_consumer_->Start();
+  if (is_deferred_)
+    body_consumer_->SetDefersLoading();
 }
 
 void URLLoaderClientImpl::OnComplete(
@@ -82,7 +165,14 @@ void URLLoaderClientImpl::OnComplete(
 }
 
 void URLLoaderClientImpl::Dispatch(const IPC::Message& message) {
-  resource_dispatcher_->OnMessageReceived(message);
+  if (is_deferred_) {
+    deferred_messages_.push_back(message);
+  } else if (deferred_messages_.size() > 0) {
+    deferred_messages_.push_back(message);
+    FlushDeferredMessages();
+  } else {
+    resource_dispatcher_->OnMessageReceived(message);

[kinuko, 2016/12/23 13:11:23]

At this point couldn't we directly call ResourceDispatcher::DispatchMessage()? 
I feel it's less confusing as OnMessageReceived itself has its own deferred
message handling (& sanity checks that don't seem necessary in our case).

[yhirano, 2016/12/26 05:08:35]

ResourceDispatcher::DispatchMessage is a private method and I think exposing it
also has a risk that someone accidentally calls it.

[kinuko, 2016/12/26 07:21:17]

But eventually we plan to get rid of OnMessageReceived and the non-mojo code,
right?  I don't want to leave the possibility that the new code weirdly depends
on the old code that's supposed to be go away.  If you're uncomfortable could we
add another public method with Mojo suffix, or making URLLoaderClientImpl friend
of RD?

[yhirano, 2016/12/26 08:22:00]

Done.

+  }
 }
 
 }  // namespace content