In SdchDictionaryFetcher, don't set state when scheduling if loop is active.

net/url_request/sdch_dictionary_fetcher.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.
 
 #include "net/url_request/sdch_dictionary_fetcher.h"
 
 #include <stdint.h>
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
@@ skipping 42 matching lines @@
     // TODO(rdsmith): log this error to the net log of the URLRequest
     // initiating this fetch, once URLRequest will be passed here.
     SdchManager::SdchErrorRecovery(
         SDCH_DICTIONARY_PREVIOUSLY_SCHEDULED_TO_DOWNLOAD);
     return;
   }
 
   attempted_load_.insert(dictionary_url);
   fetch_queue_.push(dictionary_url);
 
-  next_state_ = STATE_IDLE;
+  // If the loop is already processing, it'll pick up the above in the
+  // normal course of events.  If it isn't processing, we kick it off.
+  if (next_state_ == STATE_NONE) {

[mmenke, 2015/01/21 21:33:57]

I think an early return is cleaner here.

[Randy Smith (Not in Mondays), 2015/01/22 19:04:39]

Right you are.  Done.

+    next_state_ = STATE_IDLE;

[mmenke, 2015/01/21 21:33:57]

It's a bit tangential to this CL, but while we're here, can we rename IDLE,
which sounds exactly like STATE_NONE?

States are generally named after the method they run, or vice-versa, so it
should be STATE_DISPATCH_REQUEST to match the method...

Hrm...Actually, seems like there are a bunch of naming improvements that could
be done here, and don't think it's reasonable to do all that in this CL.

[Randy Smith (Not in Mondays), 2015/01/22 19:04:39]

Naming changes, while they touch a lot of lines, don't take a lot of time.  Want
to give me a nudge in the direction of what you'd like to see and I'll take a
swing at it?  State names should match the functions that they dispatch, check. 
What else?

(Also let me know if you'd prefer them in a separate CL, else I'll just dump
them in this CL for my own convenience.)

[mmenke, 2015/01/22 19:58:32]

I'd go with:

STATE_IDLE => STATE_SEND_REQUEST / DoSendRequest() (Which returns
ERR_IO_PENDING, sets next state to STATE_RESPONSE_STARTED)

STATE_REQUEST_STARTED => STATE_SEND_REQUEST_COMPLETE / DoSendRequestComplete
(Replaces DoRequestStarted, and is called *after* we get headers, instead of
before, and just updates the state to the next one, which I'd no longer do in
OnResponseStarted.  OnResponseStarted also shouldn't be worrying about
synchronous callbacks, as they can't happen)

STATE_REQUEST_READING => STATE_READ_BODY

*New state* => STATE_READ_BODY_COMPLETE (Move all the logic from OnReadCompleted
into here, and again, remove the loop_ check)

STATE_REQUEST_COMPLETE => STATE_REQUEST_COMPLETE


That leaves OnResponseStarted and OnReadCompleted as functions that basically
only call back into the DoLoop (Maybe after some DCHECKs).

Anyhow, you can see why I was thinking all that might not belong in this CL.  :)

[mmenke, 2015/01/24 17:01:24]

And a quick followup about my reasoning:  I think keeping as much logic in the
DoLoop as possible makes the code much easier to follow.  You can just follow
the state machine to figure out what the next state does, rather than follow the
state machine + the callback methods (Which aren't terribly searchable).

[Randy Smith (Not in Mondays), 2015/01/24 23:21:01]

Yes, that makes sense to me (and was appealing to me as I was doing the
rewrite).  I'm doing this in a separate CL, coming soon to an inbox near you. 
Just to give you more exciting things for your weekend :-}.

 
-  // There are no callbacks to user code from the dictionary fetcher,
-  // and Schedule() is only called from user code, so this call to DoLoop()
-  // does not require an |if (in_loop_) return;| guard.
-  DoLoop(OK);
+    // There are no callbacks to user code from the dictionary fetcher,
+    // and Schedule() is only called from user code, so this call to DoLoop()
+    // does not require an |if (in_loop_) return;| guard.
+    DoLoop(OK);
+  }
 }
 
 void SdchDictionaryFetcher::Cancel() {
   DCHECK(CalledOnValidThread());
 
   next_state_ = STATE_NONE;
 
   while (!fetch_queue_.empty())
     fetch_queue_.pop();
   attempted_load_.clear();
@@ skipping 171 matching lines @@
   current_request_.reset();
   buffer_ = NULL;
   dictionary_.clear();
 
   next_state_ = STATE_IDLE;
 
   return OK;
 }
 
 }  // namespace net