Unplumb AllStatus from SyncerThread.

Unplumb AllStatus from SyncerThread.

The AllStatus is now strictly used for status reporting in UI.
Motivation is that the ST can get what it needs from other sources it already depends on, and less dependencies is a good thing (easier to get to MLs, and but also AllStatus is pretty complicated and makes it hard to follow what's going on when looking at the SyncerThread code). And GetRecommendedDelay just didn't belong in there in the first place IMO.

AllStatus is complicated as it tries to keep an instantaneously up-to-date status, which is a) error prone and buggy and b) not needed by the syncer thread, which only needs to check things after a session has completed.  The one subtle fact is on exit, calling Syncer::RequestEarlyExit means we may not hit SyncerEndCommand so previous_session_snapshot() will be out of date.  In this case though we don't care, since we don't need SyncerThread::CalculatePollingWaitTime to do anything.

BUG=26339
TEST=SyncerThreadTest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55010

[akalin, 2010-08-04 21:50:14]

http://codereview.chromium.org/3078022/diff/1/8
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/3078022/diff/1/8#newcode443
chrome/browser/sync/engine/syncer_thread.cc:443: SyncSessionSnapshot* snapshot =
session_context_->previous_session_snapshot();
I think you should move this logic into a helper function in session_context_,
so you can just have:

const bool ... = session-context_->has_work() or something

Then you can avoid the previous_session_snapshot() getter

http://codereview.chromium.org/3078022/diff/1/8#newcode444
chrome/browser/sync/engine/syncer_thread.cc:444: const bool
syncer_has_work_to_do = !snapshot ? false :
const bool ... = snapshot && (snapshot-> ...);

http://codereview.chromium.org/3078022/diff/1/8#newcode662
chrome/browser/sync/engine/syncer_thread.cc:662: int backoff_s = (0 ==
base_delay_seconds) ? 1 :
std::max(1, ...)

http://codereview.chromium.org/3078022/diff/1/8#newcode673
chrome/browser/sync/engine/syncer_thread.cc:673: backoff_s = std::min(backoff_s,
kMaxBackoffSeconds);
max(1, ...) here, too?

http://codereview.chromium.org/3078022/diff/1/11
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/3078022/diff/1/11#newcode108
chrome/browser/sync/sessions/sync_session_context.h:108:
sessions::SyncSessionSnapshot* previous_session_snapshot() {
replace this with a has_work_to_do function

[chron_chromium.org, 2010-08-04 22:02:24]

This makes a lot more sense. Thanks.

LGTM with fred's comments addressed.

[tim (not reviewing), 2010-08-04 22:30:49]

http://codereview.chromium.org/3078022/diff/1/8
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/3078022/diff/1/8#newcode443
chrome/browser/sync/engine/syncer_thread.cc:443: SyncSessionSnapshot* snapshot =
session_context_->previous_session_snapshot();
On 2010/08/04 21:50:14, akalin wrote:
> I think you should move this logic into a helper function in session_context_,
> so you can just have:
> 
> const bool ... = session-context_->has_work() or something
> 
> Then you can avoid the previous_session_snapshot() getter

as we chatted offline about, we'll leave it since a) the context is just a plain
container of state at the moment and b) the thing that has work to do is the
syncer, not the snapshot... so it's a little awkward the other way (e.g.
context->previous_session_had_work_left_for_syncer()

http://codereview.chromium.org/3078022/diff/1/8#newcode444
chrome/browser/sync/engine/syncer_thread.cc:444: const bool
syncer_has_work_to_do = !snapshot ? false :
On 2010/08/04 21:50:14, akalin wrote:
> const bool ... = snapshot && (snapshot-> ...);

haha. oops.

http://codereview.chromium.org/3078022/diff/1/8#newcode662
chrome/browser/sync/engine/syncer_thread.cc:662: int backoff_s = (0 ==
base_delay_seconds) ? 1 :
On 2010/08/04 21:50:14, akalin wrote:
> std::max(1, ...)

good call

http://codereview.chromium.org/3078022/diff/1/8#newcode673
chrome/browser/sync/engine/syncer_thread.cc:673: backoff_s = std::min(backoff_s,
kMaxBackoffSeconds);
On 2010/08/04 21:50:14, akalin wrote:
> max(1, ...) here, too?

makes sense.

[akalin, 2010-08-04 22:37:39]

http://codereview.chromium.org/3078022/diff/7001/8007
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/3078022/diff/7001/8007#newcode445
chrome/browser/sync/engine/syncer_thread.cc:445:
snapshot->num_server_changes_remaining > snapshot->max_local_timestamp
put parantheses: && binds tighter than ||

[tim (not reviewing), 2010-08-04 23:10:58]

http://codereview.chromium.org/3078022/diff/7001/8007
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/3078022/diff/7001/8007#newcode445
chrome/browser/sync/engine/syncer_thread.cc:445:
snapshot->num_server_changes_remaining > snapshot->max_local_timestamp
On 2010/08/04 22:37:39, akalin wrote:
> put parantheses: && binds tighter than ||

Done.

[akalin, 2010-08-05 00:31:00]

LGTM

On 2010/08/04 23:10:58, timsteele wrote:
> http://codereview.chromium.org/3078022/diff/7001/8007
> File chrome/browser/sync/engine/syncer_thread.cc (right):
> 
> http://codereview.chromium.org/3078022/diff/7001/8007#newcode445
> chrome/browser/sync/engine/syncer_thread.cc:445:
> snapshot->num_server_changes_remaining > snapshot->max_local_timestamp
> On 2010/08/04 22:37:39, akalin wrote:
> > put parantheses: && binds tighter than ||
> 
> Done.