Refactor blocking network delegates

net/url_request/url_request_unittest.cc

 // Copyright (c) 2012 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.
 
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shlobj.h>
 #endif
@@ skipping 128 matching lines @@
     return false;
 
   for (size_t i = 0; i < size; ++i) {
     if (!a[i].Equals(b[i]))
       return false;
   }
 
   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. When blocking, the delegate can do one of the following:
+//  * synchronously return a pre-specified error code, or
+//  * asynchronously return that value via an automatically called callback,
+//    or
+//  * 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, independent of whether
+// the delegate blocks in ON_BEFORE_URL_REQUEST or not.
 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; }
-  void set_auth_credentials(const AuthCredentials& auth_credentials) {
-    auth_credentials_ = auth_credentials;
-  }
-
- 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;
-    }
-
-    TestNetworkDelegate::OnBeforeURLRequest(request, callback, new_url);
-
-    if (!redirect_url_.is_empty())
-      *new_url = redirect_url_;
-
-    if (retval_ != ERR_IO_PENDING)
-      return retval_;
-
-    MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&BlockingNetworkDelegate::DoCallback,
-                   weak_factory_.GetWeakPtr(), callback));
-    return ERR_IO_PENDING;
-  }
-
-  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_);
-  }
-
-  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_);
-  }
-
-
-  int retval_;
-  int callback_retval_;
-  GURL redirect_url_;
-  NetworkDelegate::AuthRequiredResponse auth_retval_;
-  NetworkDelegate::AuthRequiredResponse auth_callback_retval_;
-  AuthCredentials auth_credentials_;
-  base::WeakPtrFactory<BlockingNetworkDelegate> weak_factory_;
-};
-
-// A network delegate that allows blocking requests until a callback function is
-// called.
-class BlockingNetworkDelegateWithManualCallback : public TestNetworkDelegate {
- public:
-  enum State {
+  // Stages in which the delegate can block.
+  enum Stage {
     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
   };
 
-  BlockingNetworkDelegateWithManualCallback()
-      : block_on_(0),
-        state_(NOT_BLOCKED) {
-  }
-
-  // Activates blocking on |state|.
-  void BlockOn(State state) {
-    block_on_ |= state;
-  }
-
-  void DoCallback(int rv) {
-    ASSERT_NE(NOT_BLOCKED, state_);
-    CompletionCallback callback = callback_;
-    Reset();
-    callback.Run(rv);
-  }
-
-  void DoAuthCallback(NetworkDelegate::AuthRequiredResponse response) {
-    ASSERT_EQ(ON_AUTH_REQUIRED, state_);
-    AuthCallback auth_callback = auth_callback_;
-    Reset();
-    auth_callback.Run(response);
-  }
-
-  // Runs the message loop until |state| is reached.
-  void WaitForState(State state) {
-    while (state_ != state)
-      MessageLoop::current()->RunAllPending();
+  // Behavior during blocked stages.  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| posts a task to run the callback using the
+                    // specified return codes.
+    USER_CALLBACK,  // User takes care of doing a callback.  |retval_| and
+                    // |auth_retval_| are ignored.

[mmenke, 2012/09/21 15:15:59]

Should add something along the lines of "Also, The message loop is quit during
blocked stages."

[vabr (Chromium), 2012/09/21 20:03:42]

Done. Played with the wording, feel free to protest.

+  };
+
+  // Creates a delegate which does not block at all.
+  explicit BlockingNetworkDelegate(BlockMode block_mode);
+
+  // Setters.
+  void set_retval(int retval) {
+    ASSERT_NE(USER_CALLBACK, block_mode_);

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

In USER_CALLBACK mode |retval_| is used at all.
Similarly for |auth_retval_| below.
(Maybe I should add it as a comment.)

[mmenke, 2012/09/21 15:15:59]

There's a comment about it in BlockMode, which I think should be enough.  If you
think it's worth another comment, though, feel free.

[vabr (Chromium), 2012/09/21 20:03:42]

Good point with the BlockMode. I think it is OK to leave it just there.

+    ASSERT_NE(ERR_IO_PENDING, retval);
+    ASSERT_NE(OK, retval);
+    retval_ = retval;
+  }
+
+  // If |auth_retval| == AUTH_REQUIRED_RESPONSE_SET_AUTH, then
+  // |auth_credentials_| will be passed with the response.
+  void set_auth_retval(AuthRequiredResponse auth_retval) {
+    ASSERT_NE(USER_CALLBACK, block_mode_);
+    ASSERT_NE(AUTH_REQUIRED_RESPONSE_IO_PENDING, auth_retval);
+    auth_retval_ = auth_retval;
+  }
+  void set_auth_credentials(const AuthCredentials& auth_credentials) {
+    auth_credentials_ = auth_credentials;
+  }
+
+  void set_redirect_url(const GURL& url) {
+    redirect_url_ = url;
+  }
+
+  // Add a stage to block on. Unless |stage| is NOT_BLOCKED or ON_AUTH_REQUIRED,
+  // it is mandatory to call set_retval().
+  void BlockOn(Stage stage) {

[mmenke, 2012/09/21 15:15:59]

nit:  Since the |= behavior doesn't seem to be needed, or even used, suggest
changing this to a simple setter:  set_block_on(Stage block_on)...

And updating the comment correspondingly.

[vabr (Chromium), 2012/09/21 20:03:42]

I don't think I agree that this is not needed -- what if, e.g., the user wants
to do USER_CALLBACK both before request and in the authorisation stage.
True, this does not happen right now, but removing it now and probably adding
back later seems to me not worth the effort.

If you still think it would be worth it, I'll do it, including changing the type
of |block_on_| to Stage, removing the explicit values for Stage constants, and
re-evaluating some of the related ASSERT/EXPECTs.

[mmenke, 2012/09/21 20:20:18]

Both blocks could be set with a single call.  I did forget the prototype would
have to use "int stage" rather than "Stage stage", which is slightly less safe,
but I think it's cleaner than the "|=".  My main complaint is that aggregating
flags like this is pretty uncommon across the portion of the Chrome code base
I'm familiar with, and I completely missed the "|=" on my first read through the
code.  Much more common, when wanting to set a flag, seems to be
blah->set_flags(blah->flags() | BLAH_SUPER_BLAH_FLAG);.

Anyhow, I can live with it as-is, I just think the current behavior is
completely unexpected, which makes it bug prone.

You missed my (non-inline) suggestion of adding a test where all 3 return OK
asynchronously, since we no longer have an async non-redirect test, and seems
reasonable to test async behavior everywhere, which would actually require
blocking on multiple stages.

[vabr (Chromium), 2012/09/23 08:46:16]

Ah, I see. I misunderstood you previously. Implemented as requested.
Also, I removed the comment at the no set_blocked_on, as the comment was
out-dated, and now it is pretty obvious what the function does.
Sorry about that! The test is in now as NetworkDelegateBlockAsynchronously.
Please let me know what you think.

+    block_on_ |= stage;
+  }
+
+  // For users to trigger a callback returning |response|.
+  // Side-effects: resets |stage_blocked_for_callback_| and stored callbacks.
+  // Only call if |block_mode_| == USER_CALLBACK.
+  void DoCallback(int response);
+  void DoAuthCallback(NetworkDelegate::AuthRequiredResponse response);

[mmenke, 2012/09/21 15:15:59]

optional nit:  Suggest these two functions be put just below the constructor,
just to keep all the simple getters and setters together.  Or just below
stage_blocked_for_callback.  I have a slight preference for putting
setters/getters last, but either is fine.

[vabr (Chromium), 2012/09/21 20:03:42]

Done.

+
+  Stage stage_blocked_for_callback() const {

[mmenke, 2012/09/21 15:15:59]

May want a comment on this, though it's not exactly a complicated function.

[vabr (Chromium), 2012/09/21 20:03:42]

Done.

+    EXPECT_EQ(USER_CALLBACK, block_mode_);
+    return stage_blocked_for_callback_;

[mmenke, 2012/09/21 15:15:59]

Good idea.

   }
 
  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;
-      callback_ = callback;
-      return ERR_IO_PENDING;
-    }
-  }
+                                 GURL* new_url) OVERRIDE;
 
   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;
-      callback_ = callback;
-      return ERR_IO_PENDING;
-    }
-  }
+                                  HttpRequestHeaders* headers) OVERRIDE;
 
   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;
-      callback_ = callback;
-      return ERR_IO_PENDING;
-    }
-  }
+      scoped_refptr<HttpResponseHeaders>* override_response_headers) OVERRIDE;
 
   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;
-      auth_callback_ = callback;
-      return NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING;
-    }
-  }
+      AuthCredentials* credentials) OVERRIDE;
 
   void Reset() {

[mmenke, 2012/09/21 15:15:59]

May want to add an:

EXPECT_NE(NOT_BLOCKED, stage_blocked_for_callback_);

[mmenke, 2012/09/21 15:18:26]

Suggesting this more for documentation purposes than to catch bugs.

[vabr (Chromium), 2012/09/21 20:03:42]

Done.

-    state_ = NOT_BLOCKED;
+    stage_blocked_for_callback_ = NOT_BLOCKED;
     callback_.Reset();
     auth_callback_.Reset();
   }

[mmenke, 2012/09/21 15:15:59]

Think this should be de-inlined, since everything else is.

[vabr (Chromium), 2012/09/21 20:03:42]

Done. Plus added comment to the declaration.

 
-  int block_on_;  // Bit mask on which states to block.
-  State state_;
+  void RunCallback(int response, const CompletionCallback& callback);
+  void RunAuthCallback(AuthRequiredResponse response,
+                       const AuthCallback& callback);
+
+  // Checks whether we should block in |stage|. If yes, returns an error code
+  // and optionally sets up callback based on |block_mode_|. If no, returns OK.
+  int MaybeBlockStage(Stage stage, const CompletionCallback& callback);
+
+  // Configuration parameters:
+  const BlockMode block_mode_;
+
+  // Values returned on blocking stages when mode is SYNCHRONOUS or
+  // AUTO_CALLBACK. For USER_CALLBACK these are set automatically to IO_PENDING.
+  int retval_;  // To be returned in non-auth stages.
+  AuthRequiredResponse auth_retval_;
+
+  GURL redirect_url_;  // Used if non-empty.
+  int block_on_;  // Bit mask: in which stages to block.
+
+  // |auth_credentials_| will be copied to |*target_auth_credential_| on
+  // callback.
+  AuthCredentials auth_credentials_;
+  AuthCredentials* target_auth_credentials_;
+
+  // Internal parameters:

[mmenke, 2012/09/21 15:15:59]

Think "internal parameters" is a little vague, and "parameters" doesn't quite
seem accurate.

[vabr (Chromium), 2012/09/21 20:03:42]

Attempted to be more specific, also with the "configuration parameters".

+  // Last blocked stage waiting for user callback (unused if |block_mode_| !=
+  // USER_CALLBACK).
+  Stage stage_blocked_for_callback_;
+
+  // Callback objects stored during blocking stages.
   CompletionCallback callback_;
   AuthCallback auth_callback_;
+
+  base::WeakPtrFactory<BlockingNetworkDelegate> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlockingNetworkDelegate);
 };
 
+BlockingNetworkDelegate::BlockingNetworkDelegate(BlockMode block_mode)
+    : block_mode_(block_mode),
+      retval_(block_mode == USER_CALLBACK ? ERR_IO_PENDING : OK),
+      auth_retval_(block_mode == USER_CALLBACK ?
+                   AUTH_REQUIRED_RESPONSE_IO_PENDING :
+                   AUTH_REQUIRED_RESPONSE_NO_ACTION),

[mmenke, 2012/09/21 15:15:59]

These USER_CALLBACK checks aren't needed, since they're never used never used in
those cases.

[vabr (Chromium), 2012/09/21 20:03:42]

True, I forgot I hard-coded those IO_PENDING values.
Thanks, for spotting this.

+      block_on_(0),
+      target_auth_credentials_(NULL),
+      stage_blocked_for_callback_(NOT_BLOCKED),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+}
+
+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.
+
+  TestNetworkDelegate::OnBeforeURLRequest(request, callback, new_url);
+
+  if (!redirect_url_.is_empty())
+    *new_url = redirect_url_;
+
+  return MaybeBlockStage(ON_BEFORE_URL_REQUEST, callback);
+}
+
+int BlockingNetworkDelegate::OnBeforeSendHeaders(
+    URLRequest* request,
+    const CompletionCallback& callback,
+    HttpRequestHeaders* headers) {
+  TestNetworkDelegate::OnBeforeSendHeaders(request, callback, headers);
+
+  return MaybeBlockStage(ON_BEFORE_SEND_HEADERS, callback);
+}
+
+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);
+
+  return MaybeBlockStage(ON_HEADERS_RECEIVED, callback);
+}
+
+NetworkDelegate::AuthRequiredResponse BlockingNetworkDelegate::OnAuthRequired(
+    URLRequest* request,
+    const AuthChallengeInfo& auth_info,
+    const AuthCallback& callback,
+    AuthCredentials* credentials) {
+  TestNetworkDelegate::OnAuthRequired(request, auth_info, callback,
+                                      credentials);
+  // Check that the user has provided callback for the previous blocked stage.
+  EXPECT_EQ(NOT_BLOCKED, stage_blocked_for_callback_);
+
+  if ((block_on_ & ON_AUTH_REQUIRED) == 0) {
+    return AUTH_REQUIRED_RESPONSE_NO_ACTION;
+  }
+
+  if (block_mode_ == USER_CALLBACK)
+    stage_blocked_for_callback_ = ON_AUTH_REQUIRED;

[mmenke, 2012/09/21 15:15:59]

Think that this line should just be move into the USER_CALLBACK case.  Then we
always set auth_callback_ and stage_blocked_for_callback_ together, also gets
rid of the conditional.

[vabr (Chromium), 2012/09/21 20:03:42]

Nice, thanks!
Done.

+
+  if (auth_retval_ == AUTH_REQUIRED_RESPONSE_SET_AUTH)
+    target_auth_credentials_ = credentials;

[mmenke, 2012/09/21 15:15:59]

This seems weird in the USER_CALLBACK case.  My suggestion is to just always set
this, even when it's not used.  Alternatively, could add in a check for
USER_CALLBACK, though I don't think the gain isn't worth the slightly added
complexity.

[vabr (Chromium), 2012/09/21 20:03:42]

I'm not sure I understood the alternative solution (adding a check for
USER_CALLBACK), because this is used in both *_CALLBACK modes, only in
SYNCHRONOUS it is not.

Anyway, since it is only a pointer, I'm fine with removing the "if" and just
copying it all the times. :)

[mmenke, 2012/09/21 20:20:18]

The alternative was "if (auth_retval_ == AUTH_REQUIRED_RESPONSE_SET_AUTH ||
block_mode_ == USER_CALLBACK)", but I prefer the approach you used.

+
+  switch (block_mode_) {
+    case SYNCHRONOUS:
+      if (auth_retval_ == AUTH_REQUIRED_RESPONSE_SET_AUTH)
+        *target_auth_credentials_ = auth_credentials_;
+      return auth_retval_;
+
+    case AUTO_CALLBACK:
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&BlockingNetworkDelegate::RunAuthCallback,
+                     weak_factory_.GetWeakPtr(), auth_retval_, callback));
+      return AUTH_REQUIRED_RESPONSE_IO_PENDING;
+
+    case USER_CALLBACK:
+      auth_callback_ = callback;
+      MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
+      return AUTH_REQUIRED_RESPONSE_IO_PENDING;
+  }
+  NOTREACHED();
+  return AUTH_REQUIRED_RESPONSE_NO_ACTION;  // Dummy value.
+}
+
+void BlockingNetworkDelegate::DoCallback(int response) {

[mmenke, 2012/09/21 15:15:59]

Per the style guide, The four callback functions should be in the same order
they're declared in.  Suggest moving the private callback functions up to the
top of the private section, so they'll all still be together.

[vabr (Chromium), 2012/09/21 20:03:42]

Done.

+  ASSERT_EQ(USER_CALLBACK, block_mode_);
+  ASSERT_NE(NOT_BLOCKED, stage_blocked_for_callback_);
+  ASSERT_NE(ON_AUTH_REQUIRED, stage_blocked_for_callback_);
+  CompletionCallback callback = callback_;
+  Reset();
+  RunCallback(response, callback);
+}
+
+void BlockingNetworkDelegate::DoAuthCallback(
+    NetworkDelegate::AuthRequiredResponse response) {
+  ASSERT_EQ(USER_CALLBACK, block_mode_);
+  ASSERT_EQ(ON_AUTH_REQUIRED, stage_blocked_for_callback_);
+  AuthCallback auth_callback = auth_callback_;
+  Reset();
+  RunAuthCallback(response, auth_callback);
+}
+
+void BlockingNetworkDelegate::RunCallback(int response,
+                                          const CompletionCallback& callback) {
+  callback.Run(response);
+}
+
+void BlockingNetworkDelegate::RunAuthCallback(AuthRequiredResponse response,
+                                                 const AuthCallback& callback) {

[mmenke, 2012/09/21 15:15:59]

nit:  Fix indent.

[vabr (Chromium), 2012/09/21 20:03:42]

Done.

+  if (auth_retval_ == AUTH_REQUIRED_RESPONSE_SET_AUTH) {
+    ASSERT_TRUE(target_auth_credentials_ != NULL);
+    *target_auth_credentials_ = auth_credentials_;
+  }
+  callback.Run(response);
+}
+
+int BlockingNetworkDelegate::MaybeBlockStage(
+    BlockingNetworkDelegate::Stage stage,
+    const CompletionCallback& callback) {
+  // Check that the user has provided callback for the previous blocked stage.
+  EXPECT_EQ(NOT_BLOCKED, stage_blocked_for_callback_);
+
+  if ((block_on_ & stage) == 0) {
+    return OK;
+  }
+
+  if (block_mode_ == USER_CALLBACK)
+    stage_blocked_for_callback_ = stage;

[mmenke, 2012/09/21 15:15:59]

As above, think this should be moved down right next to callback_ = callback.

[vabr (Chromium), 2012/09/21 20:03:42]

Done.

+
+  switch (block_mode_) {
+    case SYNCHRONOUS:
+      EXPECT_NE(OK, retval_);
+      return retval_;
+
+    case AUTO_CALLBACK:
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&BlockingNetworkDelegate::RunCallback,
+                     weak_factory_.GetWeakPtr(), retval_, callback));
+      return ERR_IO_PENDING;
+
+    case USER_CALLBACK:
+      callback_ = callback;
+      MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
+      return ERR_IO_PENDING;
+  }
+  NOTREACHED();
+  return 0;
+}
+
 class TestURLRequestContextWithProxy : public TestURLRequestContext {
  public:
   // Does not own |delegate|.
   TestURLRequestContextWithProxy(const std::string& proxy,
                                  NetworkDelegate* delegate)
       : TestURLRequestContext(true) {
     context_storage_.set_proxy_service(ProxyService::CreateFixed(proxy));
     set_network_delegate(delegate);
     Init();
   }
@@ skipping 1539 matching lines @@
     EXPECT_EQ(1, network_delegate.error_count());
     EXPECT_EQ(ERR_TUNNEL_CONNECTION_FAILED, network_delegate.last_error());
   }
 }
 
 // Tests that the network delegate can block and cancel a request.
 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(),
       &network_delegate);
 
   {
     URLRequest r(test_server_.GetURL(""), &d, &context);
 
     r.Start();
     MessageLoop::current()->Run();
 
     EXPECT_EQ(URLRequestStatus::FAILED, r.status().status());
     EXPECT_EQ(ERR_EMPTY_RESPONSE, r.status().error());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that the network delegate can cancel a request synchronously.
 TEST_F(URLRequestTestHTTP, NetworkDelegateCancelRequestSynchronously) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::SYNCHRONOUS);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST);
   network_delegate.set_retval(ERR_EMPTY_RESPONSE);
 
   TestURLRequestContextWithProxy context(
       test_server_.host_port_pair().ToString(),
       &network_delegate);
 
   {
     URLRequest r(test_server_.GetURL(""), &d, &context);
 
     r.Start();
     MessageLoop::current()->Run();
 
     EXPECT_EQ(URLRequestStatus::FAILED, r.status().status());
     EXPECT_EQ(ERR_EMPTY_RESPONSE, r.status().error());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that the network delegate can block and redirect a request to a new
 // URL.
 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);
 
   TestURLRequestContextWithProxy context(
       test_server_.host_port_pair().ToString(),
       &network_delegate);
 
   {
     GURL original_url(test_server_.GetURL("empty.html"));
     URLRequest r(original_url, &d, &context);
@@ skipping 11 matching lines @@
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that the network delegate can block and redirect a request to a new
 // URL by setting a redirect_url and returning in OnBeforeURLRequest directly.
 TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestSynchronously) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::SYNCHRONOUS);
   GURL redirect_url(test_server_.GetURL("simple.html"));
   network_delegate.set_redirect_url(redirect_url);
-  network_delegate.set_retval(OK);
 
   TestURLRequestContextWithProxy context(
       test_server_.host_port_pair().ToString(),
       &network_delegate);
 
   {
     GURL original_url(test_server_.GetURL("empty.html"));
     URLRequest r(original_url, &d, &context);
 
     r.Start();
@@ skipping 10 matching lines @@
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that redirects caused by the network delegate preserve POST data.
 TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestPost) {
   ASSERT_TRUE(test_server_.Start());
 
   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);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
   context.Init();
 
   {
     GURL original_url(test_server_.GetURL("empty.html"));
     URLRequest r(original_url, &d, &context);
@@ skipping 20 matching lines @@
 }
 
 // Tests that the network delegate can synchronously complete OnAuthRequired
 // by taking no action. This indicates that the NetworkDelegate does not want to
 // handle the challenge, and is passing the buck along to the
 // URLRequest::Delegate.
 TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredSyncNoAction) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
-  network_delegate.set_auth_retval(
-      NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION);
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::SYNCHRONOUS);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);

[mmenke, 2012/09/21 15:15:59]

This is fine, though not actually needed.

[vabr (Chromium), 2012/09/21 20:03:42]

Removed to be consistent with NetworkDelegateRedirectRequestSynchronously (the
same situation but before request and with OK).

 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
   context.Init();
 
   d.set_credentials(AuthCredentials(kUser, kSecret));
 
   {
     GURL url(test_server_.GetURL("auth-basic"));
     URLRequest r(url, &d, &context);
     r.Start();
     MessageLoop::current()->Run();
 
     EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
     EXPECT_EQ(0, r.status().error());
     EXPECT_EQ(200, r.GetResponseCode());
     EXPECT_TRUE(d.auth_required_called());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that the network delegate can synchronously complete OnAuthRequired
 // by setting credentials.
 TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredSyncSetAuth) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::SYNCHRONOUS);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
   network_delegate.set_auth_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_SET_AUTH);
 
   network_delegate.set_auth_credentials(AuthCredentials(kUser, kSecret));
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
   context.Init();
 
   {
@@ skipping 11 matching lines @@
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that the network delegate can synchronously complete OnAuthRequired
 // by cancelling authentication.
 TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredSyncCancel) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::SYNCHRONOUS);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
   network_delegate.set_auth_retval(
       NetworkDelegate::AUTH_REQUIRED_RESPONSE_CANCEL_AUTH);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
   context.Init();
 
   {
     GURL url(test_server_.GetURL("auth-basic"));
     URLRequest r(url, &d, &context);
@@ skipping 11 matching lines @@
 }
 
 // Tests that the network delegate can asynchronously complete OnAuthRequired
 // by taking no action. This indicates that the NetworkDelegate does not want
 // to handle the challenge, and is passing the buck along to the
 // URLRequest::Delegate.
 TEST_F(URLRequestTestHTTP, NetworkDelegateOnAuthRequiredAsyncNoAction) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
-  BlockingNetworkDelegate network_delegate;
-  network_delegate.set_auth_retval(
-      NetworkDelegate::AUTH_REQUIRED_RESPONSE_IO_PENDING);
-  network_delegate.set_auth_callback_retval(
-      NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION);
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.BlockOn(BlockingNetworkDelegate::ON_AUTH_REQUIRED);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
   context.Init();
 
   d.set_credentials(AuthCredentials(kUser, kSecret));
 
   {
     GURL url(test_server_.GetURL("auth-basic"));
     URLRequest r(url, &d, &context);
     r.Start();
     MessageLoop::current()->Run();
 
     EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
     EXPECT_EQ(0, r.status().error());
     EXPECT_EQ(200, r.GetResponseCode());
     EXPECT_TRUE(d.auth_required_called());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that the network delegate can asynchronously complete OnAuthRequired
 // by setting credentials.
 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);
   network_delegate.set_auth_credentials(auth_credentials);
 
   TestURLRequestContext context(true);
   context.set_network_delegate(&network_delegate);
   context.Init();
 
   {
@@ skipping 12 matching lines @@
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that the network delegate can asynchronously complete OnAuthRequired
 // by cancelling authentication.
 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);
   context.set_network_delegate(&network_delegate);
   context.Init();
 
   {
     GURL url(test_server_.GetURL("auth-basic"));
     URLRequest r(url, &d, &context);
     r.Start();
     MessageLoop::current()->Run();
 
     EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
     EXPECT_EQ(OK, r.status().error());
     EXPECT_EQ(401, r.GetResponseCode());
     EXPECT_FALSE(d.auth_required_called());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that we can handle when a network request was canceled while we were
 // waiting for the network delegate.
 // Part 1: Request is cancelled while waiting for OnBeforeURLRequest callback.
 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);
   context.Init();
 
   {
     URLRequest r(test_server_.GetURL(""), &d, &context);
 
     r.Start();
-    network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_BEFORE_URL_REQUEST);
+    MessageLoop::current()->Run();
+    EXPECT_EQ(BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST,
+              network_delegate.stage_blocked_for_callback());
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();
     // Ensure that network delegate is notified.
     EXPECT_EQ(1, network_delegate.completed_requests());
     EXPECT_EQ(URLRequestStatus::CANCELED, r.status().status());
     EXPECT_EQ(ERR_ABORTED, r.status().error());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that we can handle when a network request was canceled while we were
 // waiting for the network delegate.
 // Part 2: Request is cancelled while waiting for OnBeforeSendHeaders callback.
 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);
   context.Init();
 
   {
     URLRequest r(test_server_.GetURL(""), &d, &context);
 
     r.Start();
-    network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_BEFORE_SEND_HEADERS);
+    MessageLoop::current()->Run();
+    EXPECT_EQ(BlockingNetworkDelegate::ON_BEFORE_SEND_HEADERS,
+              network_delegate.stage_blocked_for_callback());
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();
     // Ensure that network delegate is notified.
     EXPECT_EQ(1, network_delegate.completed_requests());
     EXPECT_EQ(URLRequestStatus::CANCELED, r.status().status());
     EXPECT_EQ(ERR_ABORTED, r.status().error());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that we can handle when a network request was canceled while we were
 // waiting for the network delegate.
 // Part 3: Request is cancelled while waiting for OnHeadersReceived callback.
 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);
   context.Init();
 
   {
     URLRequest r(test_server_.GetURL(""), &d, &context);
 
     r.Start();
-    network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_HEADERS_RECEIVED);
+    MessageLoop::current()->Run();
+    EXPECT_EQ(BlockingNetworkDelegate::ON_HEADERS_RECEIVED,
+              network_delegate.stage_blocked_for_callback());
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();
     // Ensure that network delegate is notified.
     EXPECT_EQ(1, network_delegate.completed_requests());
     EXPECT_EQ(URLRequestStatus::CANCELED, r.status().status());
     EXPECT_EQ(ERR_ABORTED, r.status().error());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
 // Tests that we can handle when a network request was canceled while we were
 // waiting for the network delegate.
 // Part 4: Request is cancelled while waiting for OnAuthRequired callback.
 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);
   context.Init();
 
   {
     URLRequest r(test_server_.GetURL("auth-basic"), &d, &context);
 
     r.Start();
-    network_delegate.WaitForState(
-        BlockingNetworkDelegateWithManualCallback::ON_AUTH_REQUIRED);
+    MessageLoop::current()->Run();
+    EXPECT_EQ(BlockingNetworkDelegate::ON_AUTH_REQUIRED,
+              network_delegate.stage_blocked_for_callback());
     EXPECT_EQ(0, network_delegate.completed_requests());
     // Cancel before callback.
     r.Cancel();
     // Ensure that network delegate is notified.
     EXPECT_EQ(1, network_delegate.completed_requests());
     EXPECT_EQ(URLRequestStatus::CANCELED, r.status().status());
     EXPECT_EQ(ERR_ABORTED, r.status().error());
     EXPECT_EQ(1, network_delegate.created_requests());
     EXPECT_EQ(0, network_delegate.destroyed_requests());
   }
@@ skipping 2249 matching lines @@
 
     EXPECT_FALSE(r.is_pending());
     EXPECT_EQ(1, d->response_started_count());
     EXPECT_FALSE(d->received_data_before_response());
     EXPECT_EQ(d->bytes_received(), static_cast<int>(file_size));
   }
 }
 #endif  // !defined(DISABLE_FTP_SUPPORT)
 
 }  // namespace net