Properly handle defers loading in WebURLLoaderImpl for data uris

content/child/web_url_loader_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // An implementation of WebURLLoader in terms of ResourceLoaderBridge.
 
 #include "content/child/web_url_loader_impl.h"
 
 #include <algorithm>
 #include <deque>
@@ skipping 336 matching lines @@
   scoped_ptr<FtpDirectoryListingResponseDelegate> ftp_listing_delegate_;
   scoped_ptr<MultipartResponseDelegate> multipart_delegate_;
   scoped_ptr<ResourceLoaderBridge> completed_bridge_;
   scoped_ptr<StreamOverrideParameters> stream_override_;
   mojo::ScopedDataPipeProducerHandle body_stream_writer_;
   mojo::common::HandleWatcher body_stream_writer_watcher_;
   // TODO(yhirano): Delete this buffer after implementing the back-pressure
   // mechanism.
   std::deque<char> body_stream_buffer_;
   bool got_all_stream_body_data_;
+  enum {NO_DEFER, DEFERS, DEFERED_DATA} defers_loading_;

[davidben, 2014/11/18 21:02:29]

Nit: I ended up pausing a bit to check whether we usually define enums that way.
Maybe give it a name like enum DeferState or so. (defer_state_ is probably a
better name too? defers_loading_ sounds like it should be a bool.)

Other nit: deferred has two r's. :-)

Other other nit: the three enum names don't really match in tense very well.
Maybe NOT_DEFERRING, DEFERRING, DEFERRED_DATA?

[jochen (gone - plz use gerrit), 2014/11/19 08:54:06]

it also doesn't look clang-formatted.

[João Eiras, 2014/11/21 15:25:31]

Done. (I did find deferring and deferred_data to sound very ambiguous).

 };
 
 WebURLLoaderImpl::Context::Context(
     WebURLLoaderImpl* loader,
     ResourceDispatcher* resource_dispatcher,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : loader_(loader),
       client_(NULL),
       resource_dispatcher_(resource_dispatcher),
       task_runner_(task_runner),
       referrer_policy_(blink::WebReferrerPolicyDefault),
-      got_all_stream_body_data_(false) {
+      got_all_stream_body_data_(false),
+      defers_loading_(NO_DEFER)  {
 }
 
 void WebURLLoaderImpl::Context::Cancel() {
   if (bridge_) {
     bridge_->Cancel();
     bridge_.reset();
   }
 
   // Ensure that we do not notify the multipart delegate anymore as it has
   // its own pointer to the client.
   if (multipart_delegate_)
     multipart_delegate_->Cancel();
   // Ditto for the ftp delegate.
   if (ftp_listing_delegate_)
     ftp_listing_delegate_->Cancel();
 
   // Do not make any further calls to the client.
   client_ = NULL;
   loader_ = NULL;
 }
 
 void WebURLLoaderImpl::Context::SetDefersLoading(bool value) {
   if (bridge_)
     bridge_->SetDefersLoading(value);
+
+  if (value) {
+    DCHECK_EQ(defers_loading_, NO_DEFER);

[davidben, 2014/11/18 21:02:29]

Nit: I'd do DCHECK_EQ(NO_DEFER, defers_loading_) to be consistent with EXPECT_EQ
and ASSERT_EQ. (For the latter, the expected value is on the left.)

[João Eiras, 2014/11/21 15:25:31]

done

+    defers_loading_ = DEFERS;
+  } else {
+    DCHECK_NE(defers_loading_, NO_DEFER);
+    if (defers_loading_ == DEFERED_DATA)

[davidben, 2014/11/18 21:02:29]

Nit: curly braces since the body is multiple lines.

[João Eiras, 2014/11/21 15:25:31]

done

+      base::MessageLoop::current()->PostTask(

[davidben, 2014/11/18 21:02:29]

I believe base::MessageLoop::current() should be task_runner_

[João Eiras, 2014/11/21 15:25:31]

done

+          FROM_HERE, base::Bind(&Context::HandleDataURL, this));
+    defers_loading_ = NO_DEFER;
+  }
 }
 
 void WebURLLoaderImpl::Context::DidChangePriority(
     WebURLRequest::Priority new_priority, int intra_priority_value) {
   if (bridge_)
     bridge_->DidChangePriority(
         ConvertWebKitPriorityToNetPriority(new_priority), intra_priority_value);
 }
 
 bool WebURLLoaderImpl::Context::AttachThreadedDataReceiver(
@@ skipping 426 matching lines @@
 
   std::string mime_type, unused_charset;
   if (net::DataURL::Parse(request_.url(), &mime_type, &unused_charset, NULL) &&
       net::IsSupportedMimeType(mime_type))
     return true;
 
   return false;
 }
 
 void WebURLLoaderImpl::Context::HandleDataURL() {
+  if (defers_loading_ != NO_DEFER) {

[jochen (gone - plz use gerrit), 2014/11/19 08:54:06]

deferes_loading_ should be "DEFERS" if it's != NO_DEFER here, right?

[João Eiras, 2014/11/21 15:25:31]

It should. Added an assert.

+      defers_loading_ = DEFERED_DATA;
+      return;
+  }
+
   ResourceResponseInfo info;
   std::string data;
 
   int error_code = GetInfoFromDataURL(request_.url(), &info, &data);
 
   if (error_code == net::OK) {
     OnReceivedResponse(info);
     if (!data.empty())
       OnReceivedData(data.data(), data.size(), 0);
   }
@@ skipping 305 matching lines @@
                                          int intra_priority_value) {
   context_->DidChangePriority(new_priority, intra_priority_value);
 }
 
 bool WebURLLoaderImpl::attachThreadedDataReceiver(
     blink::WebThreadedDataReceiver* threaded_data_receiver) {
   return context_->AttachThreadedDataReceiver(threaded_data_receiver);
 }
 
 }  // namespace content