Refactor blocking network delegates

net/url_request/url_request_unittest.cc

Index: net/url_request/url_request_unittest.cc
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 17f60297aed0ae98ead58b5435323a1d9be97dfe..1d1ecc59f284c1db4647628fd1216767453b9a0c 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -146,215 +146,292 @@ bool FingerprintsEqual(const HashValueVector& a, const HashValueVector& b) {
   return true;
 }
 
-// A network delegate that blocks requests, optionally cancelling or redirecting
-// them.
+// A network delegate that allows the user to choose a subset of request stages
+// to block in. On blocking, the delegate allows either:
+//  * to synchronously return a pre-specified value, or

[mmenke, 2012/09/17 19:25:23]

nit:  pre-specified error code?

[vabr (Chromium), 2012/09/18 17:21:48]

Done.

+//  * to asynchronously return that value via an automatically called callback,
+//    or
+//  * to block and wait for the user to do a callback.
+// Additionally, the user may also specify a redirect URL -- then each request
+// with the current URL different from the redirect target will be redirected
+// to that target, in the on-before-URL-request stage.
 class BlockingNetworkDelegate : public TestNetworkDelegate {
  public:
-  BlockingNetworkDelegate()
-      : retval_(ERR_IO_PENDING),
-        callback_retval_(OK),
-        auth_retval_(NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING),
-        auth_callback_retval_(
-            NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION),
-        ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {}
-
-  void set_retval(int retval) { retval_ = retval; }
-  void set_callback_retval(int retval) { callback_retval_ = retval; }
-  void set_redirect_url(const GURL& url) { redirect_url_ = url; }
-  void set_auth_retval(NetworkDelegate::AuthRequiredResponse retval) {
-    auth_retval_ = retval; }
-  void set_auth_callback_retval(NetworkDelegate::AuthRequiredResponse retval) {
-    auth_callback_retval_ = retval; }
+  enum Stage {  // Stages in which the delegate can block.

[mmenke, 2012/09/17 19:25:23]

Please put the comment on the above line.  For enums, classes, and functions,
think it's best to put the API documentation on its own line.

[vabr (Chromium), 2012/09/18 17:21:48]

Done.

+    NOT_BLOCKED = 0,
+    ON_BEFORE_URL_REQUEST = 1 << 0,
+    ON_BEFORE_SEND_HEADERS = 1 << 1,
+    ON_HEADERS_RECEIVED = 1 << 2,
+    ON_AUTH_REQUIRED = 1 << 3
+  };

[mmenke, 2012/09/17 19:25:23]

nit:  Add blank line.

[vabr (Chromium), 2012/09/18 17:21:48]

Done.

+  enum Processing {  // What to do in blocking stages?
+    SYNCHRONOUS,    // No callback, return values synchronously.
+    AUTO_CALLBACK,  // |this| takes care of doing a callback.
+    USER_CALLBACK   // User takes care of doing a callback.

[mmenke, 2012/09/17 19:25:23]

I suggest you rename this to "BlockMode" and rename the corresponding field as
well.  Open to other ideas - I just want the name to imply how it relates to
other values.  Also would like the comments to be clearer.

Here's my suggestions, open to better ideas, or moving some of the comments
elsewhere:

// Behavior during BlockedStages.  During other stages, just
// returns net::OK or NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION
enum BlockMode {
  SYNCHRONOUS,    // No callback, returns specified return values.
  AUTO_CALLBACK,  // |this| takes care of doing a callback using the specified
return codes.
  USER_CALLBACK,  // User takes care of doing a callback.  |retval_| and
|auth_retval_| are ignored.
}

[vabr (Chromium), 2012/09/18 17:21:48]

I really like both the new enum name, and the extended comments.
The only change I made was BlockedStages -> blocked stages, because the former
is neither a variable or type, nor a common English word.

+  };
+
+  // Default constructor: creates a delegate which does not block at all.

[mmenke, 2012/09/17 19:25:23]

I think you can remove the "Default constructor:", since it's the only
constructor.

[vabr (Chromium), 2012/09/18 17:21:48]

Done.

+  explicit BlockingNetworkDelegate(Processing p);

[mmenke, 2012/09/17 19:25:23]

Single letter variable names like this violate Google style.

[vabr (Chromium), 2012/09/18 17:21:48]

Done.

+
+  // Setters.
+  void set_retval(int retval) {
+    ASSERT_NE(retval, ERR_IO_PENDING);

[mmenke, 2012/09/17 19:25:23]

I suggest an ASSERT_NE(retval, OK) as well.

[mmenke, 2012/09/17 19:32:57]

Err...  ASSERT_NE(OK, retval), rather.

You should reverse the order of your asserts - first param should be the
expected / unexpected value, second should be the value you're checking.  Rule
only applies to _NE and _EQ, not to comparison functions.

[vabr (Chromium), 2012/09/18 17:21:48]

Thanks, that's good to know!
Done.

[vabr (Chromium), 2012/09/18 17:21:48]

Done.
Do you think there might be a reason for the user to set
AUTH_REQUIRED_RESPONSE_NO_ACTION (let's assume they ignore the fact that it's
being set by the constructor) in set_auth_retval(), or should I check against
that as well?

If I should check, then I will also add an aut_retval_ !=
AUTH_REQUIRED_RESPONSE_NO_ACTION in OnAuthRequired(), and amend the comment at
BlockOn() declaration.

+    retval_ = retval;
+  }
+  void set_auth_retval(AuthRequiredResponse auth_retval) {
+    ASSERT_NE(auth_retval, AUTH_REQUIRED_RESPONSE_IO_PENDING);
+    auth_retval_ = auth_retval;
+  }
+  void set_redirect_url(const GURL& url) {
+    redirect_url_ = url;
+  }
   void set_auth_credentials(const AuthCredentials& auth_credentials) {
     auth_credentials_ = auth_credentials;
   }
 
+  void BlockOn(Stage stage) {  // Add a stage to block on.
+    block_on_ |= stage;
+  }
+
+  // For users to trigger a callback returning |response|.
+  // Side-effects: resets |blocked_stage_| and stored callbacks.
+  // Only call if |processing_| == USER_CALLBACK.
+  void DoCallback(int response);
+  void DoAuthCallback(NetworkDelegate::AuthRequiredResponse response);
+
+  // Runs the message loop until |stage| is reached.
+  void WaitForState(Stage stage) {

[mmenke, 2012/09/17 19:25:23]

Should we make sure the mode isn't USER_CALLBACK for this?

[vabr (Chromium), 2012/09/18 17:21:48]

Actually, it only makes sense to call this if the mode _is_ USER_CALLBACK, no?
At least this is how it's used now.

I added the check against other modes, because I don't think there is any use
for WaitForState with SYNCHRONOUS or AUTO_CALLBACK modes, and checking that
might signal an incorrect use of the delegate.

WDYT?

[mmenke, 2012/09/19 19:39:47]

Erm...You're right.  I was thinking everything was done synchronously, but only
the delegate is synchronous, its external callers may not be.

I don't like this function - it's weird, and waiting for the wrong stage will
result in it hanging.  Seems like waiting until callback_ or auth_callback_ is
non-NULL would make more sense, and renaming the function
"WaitForNextBlockedState" or some such, and checking that the mode is
USER_CALLBACK makes more sense.

However, even simpler than that - we could just explicitly exit the message loop
when we hit a blocked state in the USER_CALLBACK Mode, and get rid of this
function entirely.

[vabr (Chromium), 2012/09/20 11:45:44]

You have a good point. I tried to modify it a bit differently, but with
addressing the issues you raised:
It still waits until the blocked stage is signalled, but also checks that we did
not get into the final stage of the request. So it should not get stuck. I could
remove the |blocked_stage_| and check that the callbacks have been set, but this
seems a little bit clearer to me.
Here I had a problem with blocking in the ON_BEFORE_REQUEST stage. The tests
first start the request, and then the message loop, so attempting to exit the
loop on before request fails for no loop being started. On the other hand, once
the loop is started, we have no option to exit it, because OnBeforeURLRequest
already ran.
I feel that working this around would be more complex than the currently
repaired WaitForWhatever function, but I'm open to suggestions.

[mmenke, 2012/09/20 15:37:52]

Right.  I had not realized starting the requests would call ON_BEFORE_REQUEST
synchronously.  The solution to this is to do what TestDelegate does: 
MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());

My issue with WaitForWhatever is that in one set of tests we run the message
loop until quit, and in the other set, we run it bit by bit.  I think using the
different behaviors when not strictly needed is a bad design pattern, if we can
easily avoid it.  Also think that "while (!x) RunMessageLoop();" isn't a great
pattern in general.

[vabr (Chromium), 2012/09/21 11:11:36]

Thanks for the idea with posting the QuitClosure task. That worked,
WaitForWhatever is now removed.

+    while (blocked_stage_ != stage)
+      MessageLoop::current()->RunAllPending();
+  }
+
  private:
   // TestNetworkDelegate implementation.
   virtual int OnBeforeURLRequest(URLRequest* request,
                                  const CompletionCallback& callback,
-                                 GURL* new_url) OVERRIDE {
-    if (redirect_url_ == request->url()) {
-      // We've already seen this request and redirected elsewhere.
-      return OK;
-    }
+                                 GURL* new_url) OVERRIDE;
 
-    TestNetworkDelegate::OnBeforeURLRequest(request, callback, new_url);
-
-    if (!redirect_url_.is_empty())
-      *new_url = redirect_url_;
-
-    if (retval_ != ERR_IO_PENDING)
-      return retval_;
+  virtual int OnBeforeSendHeaders(URLRequest* request,
+                                  const CompletionCallback& callback,
+                                  HttpRequestHeaders* headers) OVERRIDE;
 
-    MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&BlockingNetworkDelegate::DoCallback,
-                   weak_factory_.GetWeakPtr(), callback));
-    return ERR_IO_PENDING;
-  }
+  virtual int OnHeadersReceived(
+      URLRequest* request,
+      const CompletionCallback& callback,
+      HttpResponseHeaders* original_response_headers,
+      scoped_refptr<HttpResponseHeaders>* override_response_headers) OVERRIDE;
 
   virtual NetworkDelegate::AuthRequiredResponse OnAuthRequired(
       URLRequest* request,
       const AuthChallengeInfo& auth_info,
       const AuthCallback& callback,
-      AuthCredentials* credentials) OVERRIDE {
-    TestNetworkDelegate::OnAuthRequired(request, auth_info, callback,
-                                        credentials);
-    switch (auth_retval_) {
-      case NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION:
-        break;
-      case NetworkDelegate::AUTH_REQUIRED_RESPONSE_SET_AUTH:
-        *credentials = auth_credentials_;
-      case NetworkDelegate::AUTH_REQUIRED_RESPONSE_CANCEL_AUTH:
-        break;
-      case NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING:
-        MessageLoop::current()->PostTask(
-            FROM_HERE,
-            base::Bind(&BlockingNetworkDelegate::DoAuthCallback,
-                       weak_factory_.GetWeakPtr(), callback, credentials));
-        break;
-    }
-    return auth_retval_;
-  }
-
-  void DoCallback(const CompletionCallback& callback) {
-    callback.Run(callback_retval_);
-  }
+      AuthCredentials* credentials) OVERRIDE;
 
-  void DoAuthCallback(const AuthCallback& callback,
-                      AuthCredentials* credentials) {
-    if (auth_callback_retval_ ==
-        NetworkDelegate::AUTH_REQUIRED_RESPONSE_SET_AUTH) {
-      *credentials = auth_credentials_;
-    }
-    callback.Run(auth_callback_retval_);
+  void Reset() {
+    blocked_stage_ = NOT_BLOCKED;
+    callback_.Reset();
+    auth_callback_.Reset();
   }
 
+  void DoAutoCallback(const CompletionCallback& callback);
+  void DoAutoAuthCallback(const AuthCallback& callback,
+                      AuthCredentials* credentials);
 
-  int retval_;
-  int callback_retval_;
-  GURL redirect_url_;
-  NetworkDelegate::AuthRequiredResponse auth_retval_;
-  NetworkDelegate::AuthRequiredResponse auth_callback_retval_;
+  // Configuration parameters:
+  const Processing processing_;
+  // Return values. Must not be *IO_PENDING.

[mmenke, 2012/09/17 19:25:23]

nit:  "Values returned on blocking stages when mode is SYNCHRONOUS or
AUTO_CALLBACK"

[vabr (Chromium), 2012/09/18 17:21:48]

Done.
I also removed the part about IO_PENDING, because it was confusing (the
constructor is allowed to assign this, but not the user).

+  int retval_;  // To be returned in non-auth stages.
+  AuthRequiredResponse auth_retval_;
+  GURL redirect_url_;  // Used if non-empty and we block before request.
+  int block_on_;  // Bit mask on which states to block.
   AuthCredentials auth_credentials_;
+
+  // Internal parameters:
+  Stage blocked_stage_;  // Contains the last stage in which delegate blocked.
+  // Callback objects stored during blocking stages.
+  CompletionCallback callback_;
+  AuthCallback auth_callback_;
   base::WeakPtrFactory<BlockingNetworkDelegate> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlockingNetworkDelegate);
 };
 
-// A network delegate that allows blocking requests until a callback function is
-// called.
-class BlockingNetworkDelegateWithManualCallback : public TestNetworkDelegate {
- public:
-  enum State {
-    NOT_BLOCKED = 0,
-    ON_BEFORE_URL_REQUEST = 1 << 0,
-    ON_BEFORE_SEND_HEADERS = 1 << 1,
-    ON_HEADERS_RECEIVED = 1 << 2,
-    ON_AUTH_REQUIRED = 1 << 3
-  };
+BlockingNetworkDelegate::BlockingNetworkDelegate(Processing processing)
+    : processing_(processing),
+      retval_(processing == USER_CALLBACK ? ERR_IO_PENDING : OK),
+      auth_retval_(processing == USER_CALLBACK ?
+                   AUTH_REQUIRED_RESPONSE_IO_PENDING :
+                   AUTH_REQUIRED_RESPONSE_NO_ACTION),
+      block_on_(0),
+      blocked_stage_(NOT_BLOCKED),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+}
 
-  BlockingNetworkDelegateWithManualCallback()
-      : block_on_(0),
-        state_(NOT_BLOCKED) {
-  }
+int BlockingNetworkDelegate::OnBeforeURLRequest(
+    URLRequest* request,
+    const CompletionCallback& callback,
+    GURL* new_url) {
+  if (redirect_url_ == request->url())
+    return OK;  // We've already seen this request and redirected elsewhere.
 
-  // Activates blocking on |state|.
-  void BlockOn(State state) {
-    block_on_ |= state;
-  }
+  TestNetworkDelegate::OnBeforeURLRequest(request, callback, new_url);
 
-  void DoCallback(int rv) {
-    ASSERT_NE(NOT_BLOCKED, state_);
-    CompletionCallback callback = callback_;
-    Reset();
-    callback.Run(rv);
-  }
+  if ((block_on_ & ON_BEFORE_URL_REQUEST) == 0)
+    return OK;

[mmenke, 2012/09/17 19:25:23]

In all cases, I think it makes sense to clear blocked_stage_ when we don't
block.

[vabr (Chromium), 2012/09/18 17:21:48]

Done.

 
-  void DoAuthCallback(NetworkDelegate::AuthRequiredResponse response) {
-    ASSERT_EQ(ON_AUTH_REQUIRED, state_);
-    AuthCallback auth_callback = auth_callback_;
-    Reset();
-    auth_callback.Run(response);
-  }
+  blocked_stage_ = ON_BEFORE_URL_REQUEST;

[mmenke, 2012/09/17 19:25:23]

May want an EXPECT_NE(OK, retval_) around here.

[vabr (Chromium), 2012/09/18 17:21:48]

Done, with the exclusion of the case when we are just redirecting.
Also amended the comment at BlockOn().
I would prefer ASSERT here rather than EXPECT, but that would not work in a
function returning non-void.

 
-  // Runs the message loop until |state| is reached.
-  void WaitForState(State state) {
-    while (state_ != state)
-      MessageLoop::current()->RunAllPending();
-  }
+  if (!redirect_url_.is_empty())
+    *new_url = redirect_url_;
 
- private:
-  // TestNetworkDelegate implementation.
-  virtual int OnBeforeURLRequest(URLRequest* request,
-                                 const CompletionCallback& callback,
-                                 GURL* new_url) OVERRIDE {
-    TestNetworkDelegate::OnBeforeURLRequest(request, callback, new_url);
-    if ((block_on_ & ON_BEFORE_URL_REQUEST) == 0) {
-      return OK;
-    } else {
-      state_ = ON_BEFORE_URL_REQUEST;
+  switch (processing_) {

[mmenke, 2012/09/17 19:25:23]

We have this nearly identical code in 3 locations.  My suggestion:  Move setting
redirect_url_ up to just after the call to TestNetworkDelegate, then turn the
shared code into a single shared function:

return MaybeBlockOnStage(Stage);

[vabr (Chromium), 2012/09/18 17:21:48]

Done.
I like this suggestion very much, thanks!
I also changed the comments at the start of the class def., and at redirect_url_
declaration, to reflect that redirection is independent of blocking.

To extend this also for use in OnAuthRequired a template function would probably
needed, which does not look as worth the effort to me. WDYT?

+    case SYNCHRONOUS:
+      return retval_;

[mmenke, 2012/09/17 19:25:23]

I don't think blocked_stage_ should be set on the synchronous case.  May want to
rename it stage_blocked_for_callback_ or something.

[vabr (Chromium), 2012/09/18 17:21:48]

Done. Also in OnAuthRequired.

+
+    case AUTO_CALLBACK:
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&BlockingNetworkDelegate::DoAutoCallback,
+                     weak_factory_.GetWeakPtr(), callback));
+      return ERR_IO_PENDING;
+
+    case USER_CALLBACK:
       callback_ = callback;
       return ERR_IO_PENDING;
-    }
   }
+  // We never get past the switch but compiler won't believe it.
+  NOTREACHED();
+  return 0;

[mmenke, 2012/09/17 19:25:23]

optional nits:  Suggest making this the default case, just so the switch
statement returns on all paths.  I also don't think the comment is needed, since
the pattern is pretty common across the code base for the same reason, but if
you disagree, that's fine.

Same goes for the other cases as well.

[vabr (Chromium), 2012/09/18 17:21:48]

I also don't have strong opinions.
My argument against the "default:" is that compiler will check that all the
cases are handled if I don't put "default:" there.
I removed the comment, since I believe you with the common pattern, and also it
takes probably less time to understand the code than to read the sentence.

If you change your mind more strongly in favour of introducing "default:", just
let me know and I'll do it.

[mmenke, 2012/09/19 19:39:47]

Your argument makes sense.  I was not aware of that compiler warning.

+}
 
-  virtual int OnBeforeSendHeaders(URLRequest* request,
-                                  const CompletionCallback& callback,
-                                  HttpRequestHeaders* headers) OVERRIDE {
-    TestNetworkDelegate::OnBeforeSendHeaders(request, callback, headers);
-    if ((block_on_ & ON_BEFORE_SEND_HEADERS) == 0) {
-      return OK;
-    } else {
-      state_ = ON_BEFORE_SEND_HEADERS;
+int BlockingNetworkDelegate::OnBeforeSendHeaders(
+    URLRequest* request,
+    const CompletionCallback& callback,
+    HttpRequestHeaders* headers) {
+  TestNetworkDelegate::OnBeforeSendHeaders(request, callback, headers);
+
+  if ((block_on_ & ON_BEFORE_SEND_HEADERS) == 0)
+    return OK;
+
+  blocked_stage_ = ON_BEFORE_SEND_HEADERS;
+
+  switch (processing_) {
+    case SYNCHRONOUS:
+      return retval_;
+
+    case AUTO_CALLBACK:
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&BlockingNetworkDelegate::DoAutoCallback,
+                     weak_factory_.GetWeakPtr(), callback));
+      return ERR_IO_PENDING;
+
+    case USER_CALLBACK:
       callback_ = callback;
       return ERR_IO_PENDING;
-    }
   }
+  // We never get past the switch but compiler won't believe it.
+  NOTREACHED();
+  return 0;
+}
 
-  virtual int OnHeadersReceived(
-      URLRequest* request,
-      const CompletionCallback& callback,
-      HttpResponseHeaders* original_response_headers,
-      scoped_refptr<HttpResponseHeaders>* override_response_headers)
-      OVERRIDE {
-    TestNetworkDelegate::OnHeadersReceived(
-        request, callback, original_response_headers,
-        override_response_headers);
-    if ((block_on_ & ON_HEADERS_RECEIVED) == 0) {
-      return OK;
-    } else {
-      state_ = ON_HEADERS_RECEIVED;
+int BlockingNetworkDelegate::OnHeadersReceived(
+    URLRequest* request,
+    const CompletionCallback& callback,
+    HttpResponseHeaders* original_response_headers,
+    scoped_refptr<HttpResponseHeaders>* override_response_headers) {
+  TestNetworkDelegate::OnHeadersReceived(
+      request, callback, original_response_headers,
+      override_response_headers);
+
+  if ((block_on_ & ON_HEADERS_RECEIVED) == 0)
+    return OK;
+
+  blocked_stage_ = ON_HEADERS_RECEIVED;
+
+  switch (processing_) {
+    case SYNCHRONOUS:
+      return retval_;
+
+    case AUTO_CALLBACK:
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&BlockingNetworkDelegate::DoAutoCallback,
+                     weak_factory_.GetWeakPtr(), callback));
+      return ERR_IO_PENDING;
+
+    case USER_CALLBACK:
       callback_ = callback;
       return ERR_IO_PENDING;
-    }
   }
+  // We never get past the switch but compiler won't believe it.
+  NOTREACHED();
+  return 0;
+}
 
-  virtual NetworkDelegate::AuthRequiredResponse OnAuthRequired(
-      URLRequest* request,
-      const AuthChallengeInfo& auth_info,
-      const AuthCallback& callback,
-      AuthCredentials* credentials) OVERRIDE {
-    if ((block_on_ & ON_AUTH_REQUIRED) == 0) {
-      return NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION;
-    } else {
-      state_ = ON_AUTH_REQUIRED;
+NetworkDelegate::AuthRequiredResponse BlockingNetworkDelegate::OnAuthRequired(
+    URLRequest* request,
+    const AuthChallengeInfo& auth_info,
+    const AuthCallback& callback,
+    AuthCredentials* credentials) {
+  TestNetworkDelegate::OnAuthRequired(request, auth_info, callback,
+                                      credentials);
+
+  if ((block_on_ & ON_AUTH_REQUIRED) == 0)
+    return AUTH_REQUIRED_RESPONSE_NO_ACTION;
+
+  blocked_stage_ = ON_AUTH_REQUIRED;
+
+  switch (processing_) {
+    case SYNCHRONOUS:
+      return auth_retval_;
+
+    case AUTO_CALLBACK:
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&BlockingNetworkDelegate::DoAutoAuthCallback,
+                     weak_factory_.GetWeakPtr(), callback, credentials));
+      return AUTH_REQUIRED_RESPONSE_IO_PENDING;
+
+    case USER_CALLBACK:
+      if (auth_retval_ == AUTH_REQUIRED_RESPONSE_SET_AUTH)
+        *credentials = auth_credentials_;
       auth_callback_ = callback;
-      return NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING;
-    }
+      return AUTH_REQUIRED_RESPONSE_IO_PENDING;
   }
+  // We never get past the switch but compiler won't believe it.
+  NOTREACHED();
+  return AUTH_REQUIRED_RESPONSE_NO_ACTION;  // Dummy value.
+}
 
-  void Reset() {
-    state_ = NOT_BLOCKED;
-    callback_.Reset();
-    auth_callback_.Reset();
-  }
+void BlockingNetworkDelegate::DoCallback(int response) {
+  ASSERT_EQ(processing_, USER_CALLBACK);
+  ASSERT_NE(NOT_BLOCKED, blocked_stage_);
+  CompletionCallback callback = callback_;
+  Reset();
+  callback.Run(response);
+}
 
-  int block_on_;  // Bit mask on which states to block.
-  State state_;
-  CompletionCallback callback_;
-  AuthCallback auth_callback_;
-};
+void BlockingNetworkDelegate::DoAuthCallback(
+    NetworkDelegate::AuthRequiredResponse response) {
+  ASSERT_EQ(processing_, USER_CALLBACK);
+  ASSERT_EQ(ON_AUTH_REQUIRED, blocked_stage_);
+  AuthCallback auth_callback = auth_callback_;
+  Reset();
+  auth_callback.Run(response);
+}
 
+void BlockingNetworkDelegate::DoAutoCallback(

[mmenke, 2012/09/17 19:25:23]

I think having two completely unrelated pairs of callback functions is a little
weird.  Wonder if there's a way to make one call into the other.  Unfortunately,
I'm not sure there's a good way to do this, without either having to worry about
destructor order, or changing behavior slightly.

[vabr (Chromium), 2012/09/18 17:21:48]

I tried to make DoCallback call DoAutoCallback (renamed to RunCallback) and
analogously for auth.
Which destructors are you worried about?

+    const CompletionCallback& callback) {
+  callback.Run(retval_);
+}
+
+void BlockingNetworkDelegate::DoAutoAuthCallback(const AuthCallback& callback,
+                                                 AuthCredentials* credentials) {
+  if (auth_retval_ == AUTH_REQUIRED_RESPONSE_SET_AUTH)
+    *credentials = auth_credentials_;

[mmenke, 2012/09/17 19:25:23]

Looks like we don't support AUTH_REQUIRED_RESPONSE_SET_AUTH in the USER_CALLBACK
case.  May want to add support, to keep the two paths consistent.

[vabr (Chromium), 2012/09/18 17:21:48]

Done (see in OnAuthRequired, case SYNCHRONOUS).

+  callback.Run(auth_retval_);
+}
 
 // A simple Interceptor that returns a pre-built URLRequestJob one time.
 class TestJobInterceptor : public URLRequestJobFactory::Interceptor {
@@ -1949,8 +2026,10 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelRequest) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
-  network_delegate.set_callback_retval(ERR_EMPTY_RESPONSE);
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
+  network_delegate.set_retval(ERR_EMPTY_RESPONSE);
 
   TestURLRequestContextWithProxy context(
       test_server_.host_port_pair().ToString(),
@@ -1975,7 +2054,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelRequestSynchronously) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
   network_delegate.set_retval(ERR_EMPTY_RESPONSE);
 
   TestURLRequestContextWithProxy context(
@@ -2002,7 +2083,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
   GURL redirect_url(test_server_.GetURL("simple.html"));
   network_delegate.set_redirect_url(redirect_url);
 
@@ -2034,7 +2117,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestSynchronously) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
   GURL redirect_url(test_server_.GetURL("simple.html"));
   network_delegate.set_redirect_url(redirect_url);
   network_delegate.set_retval(OK);
@@ -2068,7 +2153,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestPost) {
   const char kData[] = "hello world";
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
   GURL redirect_url(test_server_.GetURL("echo"));
   network_delegate.set_redirect_url(redirect_url);
 
@@ -2109,7 +2196,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredSyncNoAction) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
   network_delegate.set_auth_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION);
 
@@ -2141,7 +2230,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredSyncSetAuth) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
   network_delegate.set_auth_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_SET_AUTH);
 
@@ -2173,7 +2264,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredSyncCancel) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
   network_delegate.set_auth_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_CANCEL_AUTH);
 
@@ -2205,10 +2298,10 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredAsyncNoAction) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
   network_delegate.set_auth_retval(
-      NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING);
-  network_delegate.set_auth_callback_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION);
 
   TestURLRequestContext context(true);
@@ -2239,10 +2332,10 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredAsyncSetAuth) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
   network_delegate.set_auth_retval(
-      NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING);
-  network_delegate.set_auth_callback_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_SET_AUTH);
 
   AuthCredentials auth_credentials(kUser, kSecret);
@@ -2275,10 +2368,10 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredAsyncCancel) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
   network_delegate.set_auth_retval(
-      NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING);
-  network_delegate.set_auth_callback_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_CANCEL_AUTH);
 
   TestURLRequestContext context(true);
@@ -2308,9 +2401,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting1) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegateWithManualCallback network_delegate;
-  network_delegate.BlockOn(
-      BlockingNetworkDelegateWithManualCallback::ON_BEFORE_URL_REQUEST);
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::USER_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
@@ -2321,7 +2414,7 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting1) {
 
     r.Start();
     network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_BEFORE_URL_REQUEST);
+        BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();
@@ -2342,9 +2435,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting2) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegateWithManualCallback network_delegate;
-  network_delegate.BlockOn(
-      BlockingNetworkDelegateWithManualCallback::ON_BEFORE_SEND_HEADERS);
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::USER_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_SEND_HEADERS);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
@@ -2355,7 +2448,7 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting2) {
 
     r.Start();
     network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_BEFORE_SEND_HEADERS);
+        BlockingNetworkDelegate::ON_BEFORE_SEND_HEADERS);
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();
@@ -2376,9 +2469,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting3) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegateWithManualCallback network_delegate;
-  network_delegate.BlockOn(
-      BlockingNetworkDelegateWithManualCallback::ON_HEADERS_RECEIVED);
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::USER_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_HEADERS_RECEIVED);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
@@ -2389,7 +2482,7 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting3) {
 
     r.Start();
     network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_HEADERS_RECEIVED);
+        BlockingNetworkDelegate::ON_HEADERS_RECEIVED);
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();
@@ -2410,9 +2503,9 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting4) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegateWithManualCallback network_delegate;
-  network_delegate.BlockOn(
-      BlockingNetworkDelegateWithManualCallback::ON_AUTH_REQUIRED);
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::USER_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
@@ -2423,7 +2516,7 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateCancelWhileWaiting4) {
 
     r.Start();
     network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_AUTH_REQUIRED);
+        BlockingNetworkDelegate::ON_AUTH_REQUIRED);
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();