sync: Finish the SyncScheduler refactor

sync: Finish the SyncScheduler refactor

This change removes SyncSessionJob entirely.  The first step towards
this goal was to refactor all functions that depended on SyncSessionJob.
All these functions have either been inlined or modified to take
different parameters instead.  DoSyncSessionJob was split into
two separate functions, one for configure jobs and one for nudge jobs,
which removes the need for SyncSessionJob's "purpose" member.

The SyncScheduler's pending_configure_job_ has been replaced with
a ConfigParams member, since that was the only part of the job still
being referenced.  The pending_nudge_job_ has been replaced with
scheduled_nudge_time_ (which is similar to the old scheduled_start_
member of SyncSessionJob) and a new object called a NudgeTracker.

The NudgeTracker inherits the SyncSessionJob's responsibilities with
respect to coalescing nudge sources.  The plan is to extend this class
to support more sophisticated nudge coalescing in the future.  For now
it tries to emulate the old SyncSessionJob behaviour as closely as
possible.

Some of the refactoring does change behaviour.  In particular, the
decision-making logic has been updated to fix issues 179515 and 155296.

BUG=175024, 179515, 155296
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194766

[rlarocque, 2013-04-08 20:19:52]

Please review.

I think the first set of try jobs failed because they failed to patch against an
old HEAD.  The second set should give better results.

This patch looks fairly invasive, but it should be mostly no-ops, at least
compared to the previous two.  It implicitly fixes a few known bugs related to
"DROP" decisions.  The rest is more straightforward, and involves mostly just
deleting old code and tests.

I did not put in much effort to try and save the whitebox unit tests.  We should
talk about if or how much of those tests are worth saving.

[tim (not reviewing), 2013-04-15 22:37:49]

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker.cc
File sync/engine/nudge_tracker.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.cc:27: // TODO(rlarocque): This function often reports
incorrect results.  However, it
Is there a bug you can reference that details what we mean by 'incorrect', and
that tracks ongoing work here?

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker.h
File sync/engine/nudge_tracker.h (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.h:6: #define SYNC_ENGINE_NUDGE_TRACKER_H_
nit - A file (or class) comment would be nice.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.h:17: }  // namespace sessions
nit - the comment isn't necessary when the whole namespace fwd declaration is
this short.

Since it looks like you're only really interacting with sessions/ types and the
purpose of this class is mainly as a smart container, it feels as though it
might belong under sessions itself?

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.h:45: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker_...
File sync/engine/nudge_tracker_unittest.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker_...
sync/engine/nudge_tracker_unittest.cc:25: TEST(NudgeTrackerTest,
CoalesceSources) {

Would it be feasible to have a test that demonstrates a GetLocallyModifiedTypes
case that you'd like to fix and reference that from your TODO?

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:343:
throttled_types.HasAll(nudge_tracker_.GetLocallyModifiedTypes())) {
Can you add a todo that we should probably PruneThrottledTypes here?  This is
orthogonal to your change (I think), but I don't want to forget.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:445: // sooner than it does.  We reschedule
in either case.
'than it does' is confusing. '... or the incoming nudge should be made to arrive
first.' would be clearer.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:470: return;
nit - remain consistent with {} on single line ifs.  e.g on 428 you didn't use
them, which is the convention in this file.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:480: && !sessions::HasSyncerError(
nit - at least the && should go on the previous line

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:488: // If we're here, then we successfully
reached the syncer.  End all backoff.
s/reached the syncer/reached the server

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:536: // This shouldn't happen, but we try to
respect the server's request anyway.
Why shouldn't this happen any more than it should in the nudge case?  We're
giving the server an unconditional lever to pull, right?

One honest disadvantage to splitting up this job-finishing code by job type is
that we want failsafes to be easily understood from a server perspective and as
dead simple of an implementation as possible client side. But we now have to
implement (and inspect, to answer "What happens if we return X?") backoff +
silencing code no less than 3 times, whereas it was 1 before. 

It really seems like this failsafe logic should be shared. None of the
error-case logic depends on job type except that you're not calling
UpdateExpoenentialBackoff for POLLs.  It would be easier to read-off how
backoff/throttle works if we passed control over to single fully inlined
function that always performed the same IsSilenced check and skipped backoff
reset for POLLs.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:39: }  // namespace sessions
nit - don't need comment.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:154: void AdjustPolling(bool force_reset);
nit - enum vs bool parameter as the intent of the parameter isn't immediately
evident from the method name.

[rlarocque, 2013-04-16 01:30:26]

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker.cc
File sync/engine/nudge_tracker.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.cc:27: // TODO(rlarocque): This function often reports
incorrect results.  However, it
On 2013/04/15 22:37:49, timsteele wrote:
> Is there a bug you can reference that details what we mean by 'incorrect', and
> that tracks ongoing work here?

I created crbug.com/231693, which is sort of related.  We should be able to fix
this TODO when we fix that bug, assuming we want this function to return correct
results.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker.h
File sync/engine/nudge_tracker.h (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.h:17: }  // namespace sessions
On 2013/04/15 22:37:49, timsteele wrote:
> nit - the comment isn't necessary when the whole namespace fwd declaration is
> this short.
> 
> Since it looks like you're only really interacting with sessions/ types and
the
> purpose of this class is mainly as a smart container, it feels as though it
> might belong under sessions itself? 

Maybe?  I never understood why sessions had to be a separate namespace in the
first place.  

I had longer-term dreams of having this class ask the sync DB directly about
which types were unsynced, and having interact with (or replace) the
ThrottledDataTypeTracker.  Would it still be a good idea to put this class in
sessions if it starts to interact with other parts of sync?

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.h:45: };
On 2013/04/15 22:37:49, timsteele wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker_...
File sync/engine/nudge_tracker_unittest.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker_...
sync/engine/nudge_tracker_unittest.cc:25: TEST(NudgeTrackerTest,
CoalesceSources) {
On 2013/04/15 22:37:49, timsteele wrote:
> 
> Would it be feasible to have a test that demonstrates a
GetLocallyModifiedTypes
> case that you'd like to fix and reference that from your TODO?

Good idea.  Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:343:
throttled_types.HasAll(nudge_tracker_.GetLocallyModifiedTypes())) {
On 2013/04/15 22:37:49, timsteele wrote:
> Can you add a todo that we should probably PruneThrottledTypes here?  This is
> orthogonal to your change (I think), but I don't want to forget.

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:445: // sooner than it does.  We reschedule
in either case.
On 2013/04/15 22:37:49, timsteele wrote:
> 'than it does' is confusing. '... or the incoming nudge should be made to
arrive
> first.' would be clearer.

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:470: return;
On 2013/04/15 22:37:49, timsteele wrote:
> nit - remain consistent with {} on single line ifs.  e.g on 428 you didn't use
> them, which is the convention in this file.

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:480: && !sessions::HasSyncerError(
On 2013/04/15 22:37:49, timsteele wrote:
> nit - at least the && should go on the previous line

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:488: // If we're here, then we successfully
reached the syncer.  End all backoff.
On 2013/04/15 22:37:49, timsteele wrote:
> s/reached the syncer/reached the server

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:536: // This shouldn't happen, but we try to
respect the server's request anyway.
On 2013/04/15 22:37:49, timsteele wrote:
> Why shouldn't this happen any more than it should in the nudge case?  We're
> giving the server an unconditional lever to pull, right?
> 
> One honest disadvantage to splitting up this job-finishing code by job type is
> that we want failsafes to be easily understood from a server perspective and
as
> dead simple of an implementation as possible client side. But we now have to
> implement (and inspect, to answer "What happens if we return X?") backoff +
> silencing code no less than 3 times, whereas it was 1 before. 
> 
> It really seems like this failsafe logic should be shared. None of the
> error-case logic depends on job type except that you're not calling
> UpdateExpoenentialBackoff for POLLs.  It would be easier to read-off how
> backoff/throttle works if we passed control over to single fully inlined
> function that always performed the same IsSilenced check and skipped backoff
> reset for POLLs.

I was under the impression that the server only throttles for excessive commits,
not excessive GUs.  Per-data-type throttling will do little to prevent GetUpdate
requests; I presumed that global throttling was similar.

I agree, the repeated throttled checking isn't ideal.  What I'd like to do
eventually is move that logic into OnSilencedUntil().  Previously it would have
been difficult to do that because of the ownership rules around SyncSessionJob. 
I think that would be the most sensible place to put the code to setup the
throttling interval.

There is some logic that is not common across the job types right now.  For
example, the configure job needs to run its ready task, while the nudge job
wants to reset its nudge interval.  As the nudge tracker gets smarter, we'll
probably want to implement nudge-specific logic in both the success and failure
cases.  I expect this structure will make more sense in the future, but right
now it looks a bit odd because it's trying very hard to maintain similarity with
the old code.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:39: }  // namespace sessions
On 2013/04/15 22:37:49, timsteele wrote:
> nit - don't need comment.

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:154: void AdjustPolling(bool force_reset);
On 2013/04/15 22:37:49, timsteele wrote:
> nit - enum vs bool parameter as the intent of the parameter isn't immediately
> evident from the method name.

I'd prefer to not add another enum in that style.  Instead, I did what I should
have done in the first place.  I split this into two separate functions, and
factored out the common code.  

It would probably make even more sense to update the poll interval only when
Start() is called, or in OnReceived*PollIntervalUpdate(), or when we receive an
update to our notification status.  I'm not going to press my luck that far,
though.  I'd prefer to maintain the existing code structure.

[tim (not reviewing), 2013-04-16 20:16:24]

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker.h
File sync/engine/nudge_tracker.h (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.h:17: }  // namespace sessions
On 2013/04/16 01:30:26, rlarocque wrote:
> On 2013/04/15 22:37:49, timsteele wrote:
> > nit - the comment isn't necessary when the whole namespace fwd declaration
is
> > this short.
> > 
> > Since it looks like you're only really interacting with sessions/ types and
> the
> > purpose of this class is mainly as a smart container, it feels as though it
> > might belong under sessions itself? 
> 
> Maybe?  I never understood why sessions had to be a separate namespace in the
> first place.  
> 
> I had longer-term dreams of having this class ask the sync DB directly about
> which types were unsynced, and having interact with (or replace) the
> ThrottledDataTypeTracker.  Would it still be a good idea to put this class in
> sessions if it starts to interact with other parts of sync?

I think so. Sessions can interact with whatever it wants, really.  You're right
that the sessions namespace may not be strictly necessary. But things in the
sync engine should only need to use sessions constructs directly. sessions is an
engine-focused wrapper around syncable that is smart enough to filter enabled /
disabled / relevant / irrelevant types and adds to that whatever transient
engine state (versus data model state) is needed to run a sync session /
SyncShare.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:536: // This shouldn't happen, but we try to
respect the server's request anyway.
On 2013/04/16 01:30:26, rlarocque wrote:
> On 2013/04/15 22:37:49, timsteele wrote:
> > Why shouldn't this happen any more than it should in the nudge case?  We're
> > giving the server an unconditional lever to pull, right?
> > 
> > One honest disadvantage to splitting up this job-finishing code by job type
is
> > that we want failsafes to be easily understood from a server perspective and
> as
> > dead simple of an implementation as possible client side. But we now have to
> > implement (and inspect, to answer "What happens if we return X?") backoff +
> > silencing code no less than 3 times, whereas it was 1 before. 
> > 
> > It really seems like this failsafe logic should be shared. None of the
> > error-case logic depends on job type except that you're not calling
> > UpdateExpoenentialBackoff for POLLs.  It would be easier to read-off how
> > backoff/throttle works if we passed control over to single fully inlined
> > function that always performed the same IsSilenced check and skipped backoff
> > reset for POLLs.
> 
> I was under the impression that the server only throttles for excessive
commits,
> not excessive GUs.  Per-data-type throttling will do little to prevent
GetUpdate
> requests; I presumed that global throttling was similar.
> 
> I agree, the repeated throttled checking isn't ideal.  What I'd like to do
> eventually is move that logic into OnSilencedUntil().  Previously it would
have
> been difficult to do that because of the ownership rules around
SyncSessionJob. 
> I think that would be the most sensible place to put the code to setup the
> throttling interval.
> 
> There is some logic that is not common across the job types right now.  For
> example, the configure job needs to run its ready task, while the nudge job
> wants to reset its nudge interval.  As the nudge tracker gets smarter, we'll
> probably want to implement nudge-specific logic in both the success and
failure
> cases.  I expect this structure will make more sense in the future, but right
> now it looks a bit odd because it's trying very hard to maintain similarity
with
> the old code.  

Yeah, global throttling is more of a panic switch than per-data-type throttling.
It will always prevent GUs as well as Commits, and should be triggerable by
either path.

The logic you're talking about that is job-type specific doesn't apply in the
case of an error.  I think error handling (backoff, throttling) is agnostic to
job type.  

I don't think it'd be too hard to pull the error-handling block that is repeated
in each case into a single method to finish off the Do*Job methods?

[rlarocque, 2013-04-16 22:20:05]

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker.h
File sync/engine/nudge_tracker.h (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/nudge_tracker....
sync/engine/nudge_tracker.h:17: }  // namespace sessions
On 2013/04/16 20:16:24, timsteele wrote:
> On 2013/04/16 01:30:26, rlarocque wrote:
> > On 2013/04/15 22:37:49, timsteele wrote:
> > > nit - the comment isn't necessary when the whole namespace fwd declaration
> is
> > > this short.
> > > 
> > > Since it looks like you're only really interacting with sessions/ types
and
> > the
> > > purpose of this class is mainly as a smart container, it feels as though
it
> > > might belong under sessions itself? 
> > 
> > Maybe?  I never understood why sessions had to be a separate namespace in
the
> > first place.  
> > 
> > I had longer-term dreams of having this class ask the sync DB directly about
> > which types were unsynced, and having interact with (or replace) the
> > ThrottledDataTypeTracker.  Would it still be a good idea to put this class
in
> > sessions if it starts to interact with other parts of sync?
> 
> I think so. Sessions can interact with whatever it wants, really.  You're
right
> that the sessions namespace may not be strictly necessary. But things in the
> sync engine should only need to use sessions constructs directly. sessions is
an
> engine-focused wrapper around syncable that is smart enough to filter enabled
/
> disabled / relevant / irrelevant types and adds to that whatever transient
> engine state (versus data model state) is needed to run a sync session /
> SyncShare.

Done.

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/6001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:536: // This shouldn't happen, but we try to
respect the server's request anyway.
On 2013/04/16 20:16:24, timsteele wrote:
> On 2013/04/16 01:30:26, rlarocque wrote:
> > On 2013/04/15 22:37:49, timsteele wrote:
> > > Why shouldn't this happen any more than it should in the nudge case? 
We're
> > > giving the server an unconditional lever to pull, right?
> > > 
> > > One honest disadvantage to splitting up this job-finishing code by job
type
> is
> > > that we want failsafes to be easily understood from a server perspective
and
> > as
> > > dead simple of an implementation as possible client side. But we now have
to
> > > implement (and inspect, to answer "What happens if we return X?") backoff
+
> > > silencing code no less than 3 times, whereas it was 1 before. 
> > > 
> > > It really seems like this failsafe logic should be shared. None of the
> > > error-case logic depends on job type except that you're not calling
> > > UpdateExpoenentialBackoff for POLLs.  It would be easier to read-off how
> > > backoff/throttle works if we passed control over to single fully inlined
> > > function that always performed the same IsSilenced check and skipped
backoff
> > > reset for POLLs.
> > 
> > I was under the impression that the server only throttles for excessive
> commits,
> > not excessive GUs.  Per-data-type throttling will do little to prevent
> GetUpdate
> > requests; I presumed that global throttling was similar.
> > 
> > I agree, the repeated throttled checking isn't ideal.  What I'd like to do
> > eventually is move that logic into OnSilencedUntil().  Previously it would
> have
> > been difficult to do that because of the ownership rules around
> SyncSessionJob. 
> > I think that would be the most sensible place to put the code to setup the
> > throttling interval.
> > 
> > There is some logic that is not common across the job types right now.  For
> > example, the configure job needs to run its ready task, while the nudge job
> > wants to reset its nudge interval.  As the nudge tracker gets smarter, we'll
> > probably want to implement nudge-specific logic in both the success and
> failure
> > cases.  I expect this structure will make more sense in the future, but
right
> > now it looks a bit odd because it's trying very hard to maintain similarity
> with
> > the old code.  
> 
> Yeah, global throttling is more of a panic switch than per-data-type
throttling.
> It will always prevent GUs as well as Commits, and should be triggerable by
> either path.
> 
> The logic you're talking about that is job-type specific doesn't apply in the
> case of an error.  I think error handling (backoff, throttling) is agnostic to
> job type.  
> 
> I don't think it'd be too hard to pull the error-handling block that is
repeated
> in each case into a single method to finish off the Do*Job methods?

I didn't think this would be a good idea, but I implemented it anyway to see
what it would look like.  That's uploaded as patch set 4.  Personally, I would
still prefer patch set #3.  Maybe I misunderstood your proposal?

[tim (not reviewing), 2013-04-16 23:02:12]

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:521: }
Hmm. I was thinking

void SyncSchedulerImpl::Do<JobType>SyncSessionJob (...)
  ...

  bool success = ...
  if (!success) {
    do <JobType> specific stuff
  } else {
    HandleFailure()
  }
}

Although your approach made me wonder why SyncShare didn't just return the full
"success" predicate itself, so that we don't have to re-compute it?  Is any of
that calculation specific to job type?  If we did that, we would essentially
have

void SSI::Do<JobType>SessionJob (...) {
 ...
 bool success = syncer_->SyncShare(...);
 if (!success) {
   HandleFailure(model_neutral_state);
   return;
 }
 do <JobType> stuff
}

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:627: void
SyncSchedulerImpl::MaybeAdjustPollInterval() {
This is not a great name.  In some sense you could prepend 'Maybe' to the
majority of function names...

I would say 

RestartPollTimer()
RestartPollTimerIfRateChanged()

are better.

That said, splitting it up like this seems messy and more likely to lead to
misuse. There are now 4 related functions lying around versus 1 and it's not
clear when you should call one and not the other. The implementations of the two
update routines are virtually identical. All so that these two tiny functions
can share... I get it, but it seems a bit contrived.

The function was pretty well scoped and simple enough that a flag controlling
how aggressive it is in carrying out its clear mission, such as FORCE_RESET,
doesn't seem harmful in the way that adding many flags to functions can often
be, imo.

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.h:39: }  // namespace sessions
still don't need this...

[rlarocque, 2013-04-17 00:50:41]

PTAL.

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:521: }
On 2013/04/16 23:02:13, timsteele wrote:
> Hmm. I was thinking
> 
> void SyncSchedulerImpl::Do<JobType>SyncSessionJob (...)
>   ...
> 
>   bool success = ...
>   if (!success) {
>     do <JobType> specific stuff
>   } else {
>     HandleFailure()
>   }
> }
> 
> Although your approach made me wonder why SyncShare didn't just return the
full
> "success" predicate itself, so that we don't have to re-compute it?  Is any of
> that calculation specific to job type?  If we did that, we would essentially
> have
> 
> void SSI::Do<JobType>SessionJob (...) {
>  ...
>  bool success = syncer_->SyncShare(...);
>  if (!success) {
>    HandleFailure(model_neutral_state);
>    return;
>  }
>  do <JobType> stuff
> }

I've updated the CL to only share code in the failure case.

I think that SyncSessionJob::Finish() used to set some debug-only status flags
based on premature_exit.  That wasn't really necessary before, and it certainly
isn't needed now.  

I could modify SyncShare(), but is it really necessary to do that in this CL?  I
was trying to limit the scope of this change.

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:627: void
SyncSchedulerImpl::MaybeAdjustPollInterval() {
On 2013/04/16 23:02:13, timsteele wrote:
> This is not a great name.  In some sense you could prepend 'Maybe' to the
> majority of function names...
> 
> I would say 
> 
> RestartPollTimer()
> RestartPollTimerIfRateChanged()
> 
> are better.
> 
> That said, splitting it up like this seems messy and more likely to lead to
> misuse. There are now 4 related functions lying around versus 1 and it's not
> clear when you should call one and not the other. The implementations of the
two
> update routines are virtually identical. All so that these two tiny functions
> can share... I get it, but it seems a bit contrived.
> 
> The function was pretty well scoped and simple enough that a flag controlling
> how aggressive it is in carrying out its clear mission, such as FORCE_RESET,
> doesn't seem harmful in the way that adding many flags to functions can often
> be, imo.
> 

I've updated the names and increased the code sharing.  

I'm not convinced that the current code sharing makes sense.  Should Start()
really use the same function to manage its poll interval as
DoConfigureSyncSessionJob() does?  For example, "!poll_timer_.IsRunning()" is
basically asking "Is this function currently being called by Start()?", and that
seems wrong.  If we need to do something special for the Start() case, we should
write that code in the Start() method itself.  

I'd like to rearrange some of this code, but it's also important to take things
one step at a time.  This CL is supposed to be about removing SyncSesionJob, not
refactoring the poll interval management.

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13743003/diff/22001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.h:39: }  // namespace sessions
On 2013/04/16 23:02:13, timsteele wrote:
> still don't need this...

Done.

[tim (not reviewing), 2013-04-17 17:49:00]

Please see my comment and consider it, but otherwise I think this CL LGTM.  

And yeah, updating SyncShare at a later time sounds just fine.

https://codereview.chromium.org/13743003/diff/19002/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/19002/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:620: void
SyncSchedulerImpl::RestartPollTimer() {
If this patch is supposed to be about removing SyncSessionJob and not
refactoring poll interval management, why are we refactoring poll interval
management? :) 

I'd be quite happy if we leaved it as before, replacing the job pointer with a
flag. That seems like the least change and closest to before.

[rlarocque, 2013-04-17 20:45:37]

https://codereview.chromium.org/13743003/diff/19002/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13743003/diff/19002/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:620: void
SyncSchedulerImpl::RestartPollTimer() {
On 2013/04/17 17:49:00, timsteele wrote:
> If this patch is supposed to be about removing SyncSessionJob and not
> refactoring poll interval management, why are we refactoring poll interval
> management? :) 
> 
> I'd be quite happy if we leaved it as before, replacing the job pointer with a
> flag. That seems like the least change and closest to before.
> 

Yes, you're right.  I'll put it back to the way it was, and add a flag.

[rlarocque, 2013-04-17 21:44:17]

I think I've fixed the AuthErrorTest by making it more like XmppAuthErrorTest. 
It no longer tests auth error detection in steady state (it now triggers the
error before startup), but I think that's OK, since our unit tests should cover
the steady-state case.

Let's see if it passes the trybots this time.

[commit-bot: I haz the power, 2013-04-18 01:07:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/13743003/37001

[commit-bot: I haz the power, 2013-04-18 04:52:11]

Change committed as 194766