Prefer ScopedNestableTaskAllower over manual save/restore

net/socket/client_socket_pool_base_unittest.cc

Index: net/socket/client_socket_pool_base_unittest.cc
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index 3ef34c48249d7e6079e641c98b0089929413f6e9..c80b4d327d57137ac4c093f613c046ad06ad05bd 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -1410,8 +1410,7 @@ class RequestSocketCallback : public TestCompletionCallbackBase {
       handle_->socket()->Disconnect();
       handle_->Reset();
       {
-        MessageLoop::ScopedNestableTaskAllower nestable(
-            MessageLoop::current());
+        MessageLoop::ScopedNestableTaskAllower allow(MessageLoop::current());
         MessageLoop::current()->RunAllPending();
       }
       within_callback_ = true;

[jar (doing other things), 2012/02/11 03:24:38]

This is a *little* scary.  The names (and pattern at line 1405) suggests that
this is used to preclude recursion.... at the same time, we allowed recursion
(potentially) on line 1414.

Could someone familiar with this code validate this is correct and as planned??

[dhollowa, 2012/02/13 17:44:26]

I've added willchan@ to the CC list.

[willchan no longer on Chromium, 2012/02/14 01:43:42]

I've looked at it and it's minorly scary as jar@ says. My understanding is this
changelist is not a behavior change, so your change is fine by me. It's minor
since the amount of code executed within this test is minimal, so the
theoretical danger of nested message loops should not happen. However, we should
fix it at some point when we have time. Perhaps file a cleanup bug against
Internal-Network-HTTP and link to the bug here.

[dhollowa, 2012/02/14 03:19:49]

Done.

@@ -1437,7 +1436,7 @@ class RequestSocketCallback : public TestCompletionCallbackBase {
           // operations that happen on timers (e.g. cleanup of idle
           // connections) can execute.
           {
-            MessageLoop::ScopedNestableTaskAllower nestable(
+            MessageLoop::ScopedNestableTaskAllower allow(
                 MessageLoop::current());
             base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(10));

[jar (doing other things), 2012/02/11 03:24:38]

This doesn't make sense to me.  All we're doing is sleeping (blocking the
current thread).  We're not running a task.  I think we should remove the
Allower completely.

...alternatively... you could investigate the history of this code... and see
how we got here... and see if we dropped some critical code on the floor.

[jar (doing other things), 2012/02/14 01:51:43]

What about removing this allow call??

On 2012/02/11 03:24:38, jar wrote:

[willchan no longer on Chromium, 2012/02/14 01:54:10]

Jim's suggestion sounds correct to me here, we can get rid of the
ScopedNestableTaskAllower.

On 2012/02/14 01:51:43, jar wrote:

[dhollowa, 2012/02/14 03:19:49]

Done.

[dhollowa, 2012/02/14 16:35:50]

I had to restore the ScopedNestableTaskAllower because removing it hung the
ClientSocketPoolBaseTest.RequestPendingJobTwice test.

             EXPECT_EQ(OK, next_job_callback.WaitForResult());