Properly handle defers loading in WebURLLoaderImpl for data uris

content/child/web_url_loader_impl.cc

Index: content/child/web_url_loader_impl.cc
diff --git a/content/child/web_url_loader_impl.cc b/content/child/web_url_loader_impl.cc
index bd6fe10b6bc29e409ab96515e44c96ec55e60d40..b5c2ec87c8df166daf052188881a900493cb31dc 100644
--- a/content/child/web_url_loader_impl.cc
+++ b/content/child/web_url_loader_impl.cc
@@ -354,6 +354,7 @@ class WebURLLoaderImpl::Context : public base::RefCounted<Context>,
   // 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(
@@ -365,7 +366,8 @@ WebURLLoaderImpl::Context::Context(
       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() {
@@ -390,6 +392,17 @@ void WebURLLoaderImpl::Context::Cancel() {
 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(
@@ -836,6 +849,11 @@ bool WebURLLoaderImpl::Context::CanHandleDataURLRequestLocally() const {
 }
 
 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;