sync: make scheduling logic and job ownership more obvious.

sync: make scheduling logic and job ownership more obvious. 

- inlines (and removes) SaveJob, to perform only the relevant "saving" bits
where needed. As it turns out, most of it was only necessary for ShouldRunJob
(which I'd like to rename, as it's destructive), and it's a lot easier to read
what's happening and why. Note also removed TODO at SaveJob callsites. 

- inlines (and removes) InitOrCoalescePendingJob to perform only the relevant
bits at each callsite. 

- pulls SyncSessionJob into a class, to handle basic responsibilities that need
the job Purpose + session (like "Succeeded()" and "Finish") that were previously
strewn about and partially copy/pasted in the scheduler. 

- meticulously transfers ownership (removes linked_ptr usage!) of jobs instead
of making many implicit struct copies, as it resulted in non-obvious behavior.
To do this, ownership is given to the scheduling mechanism (the MessageLoop for
ScheduleSyncSessionJob, WaitInterval::timer for canary jobs), instead of jobs
sitting around without knowing whether they're scheduled or not. There are a
few cases where we can't actually "schedule" a job -- which wasn't even obvious
from the old code, and other interesting revelations, like fact that we were 
actually pre-empt nudge canary jobs and "unscheduling" them in certain cases.

- removes DoPendingJobIfPossible, since it is no longer needed for 
DoCanaryJob/Unthrottle cases, and make the behavior more explicit in the other
cases (mode-switch and SCM error change), see TakePendingJobForCurrentMode. 

- removes the ability to create jobs as canary, since this wasn't needed and
was adding complexity (see DoCanaryJob / GrantCanaryPrivilege). 

- removes retry_scheduled as it wasn't used anywhere 

- adds lots of comments.

Also discovered that THROTTLED intervals seem to never fire a canary job with 
today's code (broken), config jobs are immune to per-data-type throttling, and
the had_nudge allowance was defeated by extra IsBackingOff check in 
ScheduleNudgeImpl (see comment on review).

BUG=

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

[tim (not reviewing), 2012-09-13 09:00:47]

http://codereview.chromium.org/10917234/diff/2001/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (left):

http://codereview.chromium.org/10917234/diff/2001/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:655: if (IsBackingOff() && delay >
TimeDelta::FromSeconds(1)) {
Two cases - 1)  this nudge was scheduled to start within this WaitInterval (but
more than 1s away) and could have potentially used the had_nudge allowance. I
see no reason to drop in this case.  2) this nudge was scheduled to start
*after* this WaitInterval.  Dropping here would mean that we'd still have a
nudge when the timer fired, so no biggie.  It would mean the new payloads /
hints were dropped, but most likely if we're in backoff it's not *that* big of a
deal (although, if backoff expired in just over 1s, it could help, I guess). But
I still think it's better to just coalesce the nudge with the pending nudge, to
make sure we e.g. union all datatypes (doesn't matter so much now, but may in
future) and keep payloads.

[tim (not reviewing), 2012-09-22 01:49:51]

Ready for review.  Asking for double review - initially I thought Richard could
focus on logical changes (e.g. removal of SaveJob / InitOrCoalesce and impact on
scheduling), and Fred could focus on storage / ownership changes, but the two
are pretty coupled.

[rlarocque, 2012-09-24 21:15:14]

Still reviewing.  Hitting publish early because this could take a while.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:88: // NOTE: |delay| is best-effort. If a nudge is
already scheduled to depart
Rename to max_delay?  I think that would make the effect clear without requiring
a comment.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:430: if (!requested_types.Empty() &&
throttled_types.HasAll(requested_types))
This isn't related to your change, but your comment made me think of this
scenario:

Imagine bookmarks are throttled and we receive a bookmarks nudge.  We will save
the nudge here.

Then an autofill nudge arrives.  We merged it with the saved bookmarks nudge.

When the job completes successfully, we forget that the bookmarks nudge ever
happened.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:506: if (decision != SAVE)
These lines have always bugged me.  It's not clear to the casual reader that the
decision is necessarily one of {DROP,SAVE,CONTINUE}.  Since you're refactoring
in this area anyway, could you change it so that all three cases are explicitly
mentioned?  ie.

if (decision == DROP)
  return false;

if (decision == CONTINUE)
  return true;

DCHECK_EQ(decision, SAVE)

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:512: // TODO(tim): This basically means we
never use the more-careful coalescing
This is a regression, right?  How/Why did this happen?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:517:
pending_nudge_->set_scheduled_start(job->scheduled_start());
When a user is actively using their browser, I expect there will be at least one
sessions nudge queued up most of the time.  If that's the case, then this line
would have the effect of making the syncer only commit in batches every ~10
seconds.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:521: scoped_ptr<SyncSessionJob> job_to_save =
job->CloneAndAbandon();
This makes me think ShouldRunJobSaveIfNecessary() should return a
SyncSessionJob*.  That way you could claim that the function always takes
ownership of the job passed in and returns an updated job.  The returned job may
or may not be based on the one that was passed in.  

That makes more sense to me than the current semantics where the job may be
corrupted, and the function uses a boolean flag that implies the job *may* have
been corrupted if the return value was false, but is still good if the return
value was true.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:536: unscheduled_nudge_storage_ =
pending_nudge_->CloneAndAbandon();
What effect do these two lines have?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:541: wait_interval_->length =
Could this end up being zero or negative?  What effect would that have?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:730: if (job->purpose() ==
SyncSessionJob::NUDGE) {
I get the feeling that there's not much point in sharing this function between
CONFIGURE and non-CONFIGURE jobs.  Most of the code in here is irrelevant to
either one job or the other.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:1076: // TODO(tim): ?! This must have been
broken.  The way DecideOnJob works today
Agreed, but this comment won't make much sense other than in the context of this
diff.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.h:234: // The pointer is owned by the caller.
I think this comment is no longer necessary.

[rlarocque, 2012-09-24 21:56:47]

We should talk in person/over VC about some of the job ownership semantics.  I
intend to carry this work a bit further when I change the scheduler in
preparation for removing the pre-commit GetUpdate.  Some of the ownership
concerns may be rendered obsolete by that change.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
File sync/engine/sync_session_job.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.cc:67: #ifndef NDEBUG
I wouldn't bother with the #ifndef.  The compiler should notice that this block
of statements has no effect other than a few DCHECKs and optimize accordingly. 
Even in the unlikely event that it fails to optimize away the whole thing, the
cost would be tiny because these statements are inexpensive and rarely executed.


However, if you leave this #ifndef, you increase the risk that different
warnings or errors will be generated in debug and release modes.  This is
especially dangerous because we're not likely to compile with NDEBUG set most of
the time.

http://codereview.chromium.org/10917234/diff/7029/sync/internal_api/public/se...
File sync/internal_api/public/sessions/sync_session_snapshot.h (left):

http://codereview.chromium.org/10917234/diff/7029/sync/internal_api/public/se...
sync/internal_api/public/sessions/sync_session_snapshot.h:92: bool
retry_scheduled_;
Cool!  I didn't realize this was no longer in use.  It looks like the need for
this was removed when Nicolas made the Configure job more synchronous.

[rlarocque, 2012-09-24 22:01:45]

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.h:305: scoped_ptr<SyncSessionJob>
unscheduled_nudge_storage_;
I didn't see anywhere in this patch where we read from this member, other than
in ShouldRunJobSaveIfNecessary, where we first check that it's NULL then
overwrite it on the line before we read it.

[akalin, 2012-09-25 22:37:24]

no substantial comments yet.  Still digesting...

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:88: // NOTE: |delay| is best-effort. If a nudge is
already scheduled to depart
On 2012/09/24 21:15:14, rlarocque wrote:
> Rename to max_delay?  I think that would make the effect clear without
requiring
> a comment.

but it's not a max delay, right?  the nudge can happen anytime after the delay
(due to the nature of timers, delayed tasks, etc.)

delay_hint?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:88: pending_configure_job(NULL) {
three different styles for empty function bodies here!

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:721: PostDelayedTask(loc, "DoSyncSessionJob",
why not just inline job->from_location() here?  Or define loc higher, since you
use it in multiple places

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:1005: // Only set canary privlieges here,
when we are about to run the job. This
privileges

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
File sync/engine/sync_session_job.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.cc:67: #ifndef NDEBUG
On 2012/09/24 21:56:47, rlarocque wrote:
> I wouldn't bother with the #ifndef.  The compiler should notice that this
block
> of statements has no effect other than a few DCHECKs and optimize accordingly.

> Even in the unlikely event that it fails to optimize away the whole thing, the
> cost would be tiny because these statements are inexpensive and rarely
executed.
> 
> 
> However, if you leave this #ifndef, you increase the risk that different
> warnings or errors will be generated in debug and release modes.  This is
> especially dangerous because we're not likely to compile with NDEBUG set most
of
> the time.

better to use DCHECK_IS_ON() anyway

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_job.h
File sync/engine/sync_session_job.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:34: const base::TimeTicks& start,
no const ref for timeticks

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:81: const base::TimeTicks& scheduled_start()
const;
return by value

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:82: void set_scheduled_start(const
base::TimeTicks& start);
here too

[akalin, 2012-10-03 00:11:34]

A more substantial comment/question.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:775: FinishSyncSessionJob(job.get(),
premature_exit);
can you make SyncSessionJob::Finish() return the value of job->Succeeded() and
then pass job (+ ownership) directly to FinishSyncSessionJob?  I think
simplifies the ownership of job.  You may be able to eliminate
SyncSessionJob::Succeeded() completely, too.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:823: scoped_ptr<SyncSessionJob> clone =
job->CloneAndAbandon();
Assume you implement my comment above re. passing ownership of job into this
function, do you still need this CloneAndAbandon() call?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:889: scoped_ptr<SyncSessionJob> new_job =
old_job->Clone();
same question here -- can you pass ownership to this function and avoid the
clone?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:938: const SyncSessionJob* old_job) {
similar to the above, can you pass ownership to this function and avoid the
clone? (or change it to just reset the location)

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_job.h
File sync/engine/sync_session_job.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:100: const Purpose purpose_;
newline between start of variables

[tim (not reviewing), 2012-10-08 00:20:03]

Addressed all comments.  Please take a look, also, happy to chat in person about
future plans.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:88: // NOTE: |delay| is best-effort. If a nudge is
already scheduled to depart
On 2012/09/25 22:37:24, akalin wrote:
> On 2012/09/24 21:15:14, rlarocque wrote:
> > Rename to max_delay?  I think that would make the effect clear without
> requiring
> > a comment.
> 
> but it's not a max delay, right?  the nudge can happen anytime after the delay
> (due to the nature of timers, delayed tasks, etc.)
> 
> delay_hint?

I renamed to desired_delay (following the base/timer.h desired_run_time pattern)
but I still think the comment is interesting. I added to it to mention the fact
that it may happen later as well, although that part is arguably "status quo"
when interacting with timers / delayed tasks in Chrome... but let me know what
you think.  If you prefer delay_hint, I'm fine with that too.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:88: pending_configure_job(NULL) {
On 2012/09/25 22:37:24, akalin wrote:
> three different styles for empty function bodies here!

Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:430: if (!requested_types.Empty() &&
throttled_types.HasAll(requested_types))
On 2012/09/24 21:15:14, rlarocque wrote:
> This isn't related to your change, but your comment made me think of this
> scenario:
> 
> Imagine bookmarks are throttled and we receive a bookmarks nudge.  We will
save
> the nudge here.
> 
> Then an autofill nudge arrives.  We merged it with the saved bookmarks nudge.
> 
> When the job completes successfully, we forget that the bookmarks nudge ever
> happened.  

Can you define 'forget'? The job would have accounted for both bookmarks and
autofill, and successful job completion implies end of the wait interval.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:506: if (decision != SAVE)
On 2012/09/24 21:15:14, rlarocque wrote:
> These lines have always bugged me.  It's not clear to the casual reader that
the
> decision is necessarily one of {DROP,SAVE,CONTINUE}.  Since you're refactoring
> in this area anyway, could you change it so that all three cases are
explicitly
> mentioned?  ie.
> 
> if (decision == DROP)
>   return false;
> 
> if (decision == CONTINUE)
>   return true;
> 
> DCHECK_EQ(decision, SAVE)

Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:512: // TODO(tim): This basically means we
never use the more-careful coalescing
On 2012/09/24 21:15:14, rlarocque wrote:
> This is a regression, right?  How/Why did this happen?

It is, but I suspect it has been this way for quite some time. This is one of
the reasons I renamed ShouldRunJob to point out that it's actually doing more
than just returning true / false.  I'd like to keep to no functional change with
my CL here if that sounds okay, but I plan on addressing this soon after.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:517:
pending_nudge_->set_scheduled_start(job->scheduled_start());
On 2012/09/24 21:15:14, rlarocque wrote:
> When a user is actively using their browser, I expect there will be at least
one
> sessions nudge queued up most of the time.  If that's the case, then this line
> would have the effect of making the syncer only commit in batches every ~10
> seconds.

Hmm. Afaik, this set_scheduled_start call doesn't even have a real effect now
that we removed the freshness condition, so I actually removed it altogether
now... lets chat about this in person?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:521: scoped_ptr<SyncSessionJob> job_to_save =
job->CloneAndAbandon();
On 2012/09/24 21:15:14, rlarocque wrote:
> This makes me think ShouldRunJobSaveIfNecessary() should return a
> SyncSessionJob*.  That way you could claim that the function always takes
> ownership of the job passed in and returns an updated job.  The returned job
may
> or may not be based on the one that was passed in.  
> 
> That makes more sense to me than the current semantics where the job may be
> corrupted, and the function uses a boolean flag that implies the job *may*
have
> been corrupted if the return value was false, but is still good if the return
> value was true.

Well, note the intended contract here is simpler than you describe - a false
return means the caller should assume the job is not fit for scheduling (session
== null).

If we do what you propose, I think we'd have a similar (I'd argue, worse,
problem): The returned pointer would *sometimes* require external ownership (in
the case that we Coalesced), but sometimes not (in the case that it was
rescheduled as wait interval). 

Maybe making it more clear that a false return means the job should be assumed
to no longer have a valid session and is now owned elsewhere would address the
confusion you point out?

The only alternative I can see is that in the Coalesce case, we'd
CloneAndAbandon() the pending_nudge_, Coalesce with the passed-in job, and
invoke ScheduleSyncSessionJob on the product. Then we'd return NULL (?) in all
cases we return false, and otherwise return a pointer to the job that was passed
in and already owned by the caller? That seems a bit more complicated to me, and
introduces unnecessary posts... wdyt?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:536: unscheduled_nudge_storage_ =
pending_nudge_->CloneAndAbandon();
On 2012/09/24 21:15:14, rlarocque wrote:
> What effect do these two lines have?

By "pre-empt" I mean cancel the pending nudge job by calling CloneAndAbandon, so
that we can give priority to the config job.  The first line abandons the old
nudge and stores the clone in unscheduled_nudge_storage_ (no longer a canary). 
The second line upholds the pending_nudge_ invariant, that it always points to
the one and only pending nudge (whether it is stored on the ML or u_n_s).

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:541: wait_interval_->length =
On 2012/09/24 21:15:14, rlarocque wrote:
> Could this end up being zero or negative?  What effect would that have?

Hmph. Good catch - I had fixed this since I hit a failure in a test, but somehow
undid the fix.  Fixed + added dcheck in RestartWaiting.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:721: PostDelayedTask(loc, "DoSyncSessionJob",
On 2012/09/25 22:37:24, akalin wrote:
> why not just inline job->from_location() here?  Or define loc higher, since
you
> use it in multiple places

:( Because base::Passed takes ownership of the job, and the statement is
evaluated right-to-left, so job is null.  Am I misusing base::Passed?  It seemed
weird to me too. Moved declaration up higher.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:730: if (job->purpose() ==
SyncSessionJob::NUDGE) {
On 2012/09/24 21:15:14, rlarocque wrote:
> I get the feeling that there's not much point in sharing this function between
> CONFIGURE and non-CONFIGURE jobs.  Most of the code in here is irrelevant to
> either one job or the other.

There's still a chunk of common code, but also, for now I'd like to leave it so
that the callsites of DoSyncSessionJob don't need to check the job type, and be
able to state "DoSyncSessionJob" as the thing that takes ownership of running
and ending a job.  We could potentially break it up into one-per type but still
leave DoSyncSessionJob as the top level call that forks internally to address my
concerns, (and make it that much simpler if we ever had to add a new job type,
though this is a lower priority concern).

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:775: FinishSyncSessionJob(job.get(),
premature_exit);
On 2012/10/03 00:11:34, akalin wrote:
> can you make SyncSessionJob::Finish() return the value of job->Succeeded() and
> then pass job (+ ownership) directly to FinishSyncSessionJob?  I think
> simplifies the ownership of job.  You may be able to eliminate
> SyncSessionJob::Succeeded() completely, too.

Yeah, that works! Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:823: scoped_ptr<SyncSessionJob> clone =
job->CloneAndAbandon();
On 2012/10/03 00:11:34, akalin wrote:
> Assume you implement my comment above re. passing ownership of job into this
> function, do you still need this CloneAndAbandon() call?

Interesting point.  In general the logic is if you're cloning a job before
having Finish()ed it, you need to CloneAndAbandon, because, well, it's not
Finished.  But agreed that's strange here, since we have full control over what
happens with the job.  This suggested to me that the call to Finish() below
should be moved up before this block, which let me convert CloneAndAbandon to
Clone. Then I kind of realized it's weird to do this scheduling (in the
THROTTLED) case here, and then call in to ScheduleNextSync, so I moved the
rescheduling code into that function as well.

I think it's clearer now, ptal.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:889: scoped_ptr<SyncSessionJob> new_job =
old_job->Clone();
On 2012/10/03 00:11:34, akalin wrote:
> same question here -- can you pass ownership to this function and avoid the
> clone?

I did change this around a bit, see my response above. But note finished job is
finished -- it can't be run again. I commented about this elsewhere but I think
it's easier to follow this pattern than define a bunch of "transform for retry"
style methods on SyncSessionJob.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:938: const SyncSessionJob* old_job) {
On 2012/10/03 00:11:34, akalin wrote:
> similar to the above, can you pass ownership to this function and avoid the
> clone? (or change it to just reset the location)

I'm going with the logic that a job has a well defined life  (until
DoSyncSessionJob is run through) and supports very limited modifications, versus
having one job that lives forever that we poke at in varying ways and add an
army of fancy setters and methods (to e.g.
reset-this-but-not-that-and-tweak-this for each different case). I actually
started by going down that route, but it was getting messy / fragmented. I think
overall this makes for more consistent code. It makes SyncSessionJob itself
simpler and easier to test and reason about.

As for resetting location, tracked_objects::Location doesn't allow assignment.
We'd have to make it a pointer and reset :/

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:1005: // Only set canary privlieges here,
when we are about to run the job. This
On 2012/09/25 22:37:24, akalin wrote:
> privileges

Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:1076: // TODO(tim): ?! This must have been
broken.  The way DecideOnJob works today
On 2012/09/24 21:15:14, rlarocque wrote:
> Agreed, but this comment won't make much sense other than in the context of
this
> diff.

Ah, right.  I had left this as a comment for myself while working.  Reworded and
filed bug.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.h:234: // The pointer is owned by the caller.
On 2012/09/24 21:15:14, rlarocque wrote:
> I think this comment is no longer necessary.

Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.h:305: scoped_ptr<SyncSessionJob>
unscheduled_nudge_storage_;
On 2012/09/24 22:01:46, rlarocque wrote:
> I didn't see anywhere in this patch where we read from this member, other than
> in ShouldRunJobSaveIfNecessary, where we first check that it's NULL then
> overwrite it on the line before we read it.

Right, as I said in the comment, we should not read from it.  Jobs become owned
by the fact that they're scheduled.  In rare some cases, we can't schedule a job
because there's no known point in time when we know we should try running it
again (it may be dependent on the user entering credentials, for example).  So
this just stores the job.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
File sync/engine/sync_session_job.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.cc:67: #ifndef NDEBUG
On 2012/09/25 22:37:24, akalin wrote:
> On 2012/09/24 21:56:47, rlarocque wrote:
> > I wouldn't bother with the #ifndef.  The compiler should notice that this
> block
> > of statements has no effect other than a few DCHECKs and optimize
accordingly.
> 
> > Even in the unlikely event that it fails to optimize away the whole thing,
the
> > cost would be tiny because these statements are inexpensive and rarely
> executed.
> > 
> > 
> > However, if you leave this #ifndef, you increase the risk that different
> > warnings or errors will be generated in debug and release modes.  This is
> > especially dangerous because we're not likely to compile with NDEBUG set
most
> of
> > the time.
> 
> better to use DCHECK_IS_ON() anyway

Agree w/ both points.  Removed.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_job.h
File sync/engine/sync_session_job.h (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:34: const base::TimeTicks& start,
On 2012/09/25 22:37:24, akalin wrote:
> no const ref for timeticks

Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:81: const base::TimeTicks& scheduled_start()
const;
On 2012/09/25 22:37:24, akalin wrote:
> return by value

Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:82: void set_scheduled_start(const
base::TimeTicks& start);
On 2012/09/25 22:37:24, akalin wrote:
> here too

Done.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_session_jo...
sync/engine/sync_session_job.h:100: const Purpose purpose_;
On 2012/10/03 00:11:34, akalin wrote:
> newline between start of variables

Done.

[rlarocque, 2012-10-08 19:18:04]

A few comments.  Some are in response to the old diff, others on the new.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:430: if (!requested_types.Empty() &&
throttled_types.HasAll(requested_types))
On 2012/10/08 00:20:03, timsteele wrote:
> On 2012/09/24 21:15:14, rlarocque wrote:
> > This isn't related to your change, but your comment made me think of this
> > scenario:
> > 
> > Imagine bookmarks are throttled and we receive a bookmarks nudge.  We will
> save
> > the nudge here.
> > 
> > Then an autofill nudge arrives.  We merged it with the saved bookmarks
nudge.
> > 
> > When the job completes successfully, we forget that the bookmarks nudge ever
> > happened.  
> 
> Can you define 'forget'? The job would have accounted for both bookmarks and
> autofill, and successful job completion implies end of the wait interval.   

When we calculate the items to commit in GetCommitIds, we'll filter out the
throttled types.  The job would not have accounted for bookmarks.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:512: // TODO(tim): This basically means we
never use the more-careful coalescing
On 2012/10/08 00:20:03, timsteele wrote:
> On 2012/09/24 21:15:14, rlarocque wrote:
> > This is a regression, right?  How/Why did this happen?
> 
> It is, but I suspect it has been this way for quite some time. This is one of
> the reasons I renamed ShouldRunJob to point out that it's actually doing more
> than just returning true / false.  I'd like to keep to no functional change
with
> my CL here if that sounds okay, but I plan on addressing this soon after.

Sounds good.  I was confused for a bit there; I didn't realize that this code is
only for saved jobs.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:730: if (job->purpose() ==
SyncSessionJob::NUDGE) {
On 2012/10/08 00:20:03, timsteele wrote:
> On 2012/09/24 21:15:14, rlarocque wrote:
> > I get the feeling that there's not much point in sharing this function
between
> > CONFIGURE and non-CONFIGURE jobs.  Most of the code in here is irrelevant to
> > either one job or the other.
> 
> There's still a chunk of common code, but also, for now I'd like to leave it
so
> that the callsites of DoSyncSessionJob don't need to check the job type, and
be
> able to state "DoSyncSessionJob" as the thing that takes ownership of running
> and ending a job.  We could potentially break it up into one-per type but
still
> leave DoSyncSessionJob as the top level call that forks internally to address
my
> concerns, (and make it that much simpler if we ever had to add a new job type,
> though this is a lower priority concern).

I see your point, but I think the call sites should be easy to fix.  They
already know which kind of job is being scheduled.

-> ScheduleConfiguration() only call DoSyncSessionJob on CONFIGURE jobs.
-> ScheduleSyncSessionJob() only deals with POLL and NUDGE jobs.
-> Except for the part where it calls DoSyncSessionJob, DoPendingJobIfPossible()
is one big if statement with branches for CONFIGURATION_MODE and NORMAL_MODE.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:1076: // TODO(tim): ?! This must have been
broken.  The way DecideOnJob works today
On 2012/10/08 00:20:03, timsteele wrote:
> On 2012/09/24 21:15:14, rlarocque wrote:
> > Agreed, but this comment won't make much sense other than in the context of
> this
> > diff.
> 
> Ah, right.  I had left this as a comment for myself while working.  Reworded
and
> filed bug.

On second thought, this function resets the wait_interval_.  IIRC, the flag that
indicates we're currently "globally" throttled is the wait_interval_'s mode. 
Wouldn't clearing the wait_interval_ get us out of the throttled state?

(Granted, it may be too late for this to matter for the Canary job.  However, it
should take effect eventually, right?)

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:514: // TODO(tim): This basically means we
never use the more-careful coalescing
After some more thought, I'm not sure this ever mattered.  This can only happen
when we decided to save the job, which implies we're in a WaitInterval or
Configure mode.  In either case, this job's scheduled start time is meaningless.
 We'll either run it when we switch out of configure mode, or on the next
attempt to break out of the WaitInterval.

Based on your other comment, I'm guessing you already knew this.  However, it
didn't register for me until now.

[tim (not reviewing), 2012-10-11 17:35:14]

Addressed Richard's followups, though no new code here (I filed + updated bugs).

Fred, have you had a chance to take another look?

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:430: if (!requested_types.Empty() &&
throttled_types.HasAll(requested_types))
On 2012/10/08 19:18:04, rlarocque wrote:
> On 2012/10/08 00:20:03, timsteele wrote:
> > On 2012/09/24 21:15:14, rlarocque wrote:
> > > This isn't related to your change, but your comment made me think of this
> > > scenario:
> > > 
> > > Imagine bookmarks are throttled and we receive a bookmarks nudge.  We will
> > save
> > > the nudge here.
> > > 
> > > Then an autofill nudge arrives.  We merged it with the saved bookmarks
> nudge.
> > > 
> > > When the job completes successfully, we forget that the bookmarks nudge
ever
> > > happened.  
> > 
> > Can you define 'forget'? The job would have accounted for both bookmarks and
> > autofill, and successful job completion implies end of the wait interval.   
> 
> When we calculate the items to commit in GetCommitIds, we'll filter out the
> throttled types.  The job would not have accounted for bookmarks.

Hm, I see. Good point.  I filed crbug.com/155296 to track this.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:730: if (job->purpose() ==
SyncSessionJob::NUDGE) {
On 2012/10/08 19:18:04, rlarocque wrote:
> On 2012/10/08 00:20:03, timsteele wrote:
> > On 2012/09/24 21:15:14, rlarocque wrote:
> > > I get the feeling that there's not much point in sharing this function
> between
> > > CONFIGURE and non-CONFIGURE jobs.  Most of the code in here is irrelevant
to
> > > either one job or the other.
> > 
> > There's still a chunk of common code, but also, for now I'd like to leave it
> so
> > that the callsites of DoSyncSessionJob don't need to check the job type, and
> be
> > able to state "DoSyncSessionJob" as the thing that takes ownership of
running
> > and ending a job.  We could potentially break it up into one-per type but
> still
> > leave DoSyncSessionJob as the top level call that forks internally to
address
> my
> > concerns, (and make it that much simpler if we ever had to add a new job
type,
> > though this is a lower priority concern).
> 
> I see your point, but I think the call sites should be easy to fix.  They
> already know which kind of job is being scheduled.
> 
> -> ScheduleConfiguration() only call DoSyncSessionJob on CONFIGURE jobs.
> -> ScheduleSyncSessionJob() only deals with POLL and NUDGE jobs.
> -> Except for the part where it calls DoSyncSessionJob,
DoPendingJobIfPossible()
> is one big if statement with branches for CONFIGURATION_MODE and NORMAL_MODE.

I'm not convinced we should do this yet.  I still see common code in here, I
think it is reasonable to assume we'll want to add common code to all jobs in
the future (random thought - the minute we want more UMA), and for now while I
wrap up this patch it's easiest to say "DoSyncSessionJob" takes ownership if the
job is scheduled, else unscheduled_nudge_storage.

I can add a TODO mentioning we should consider splitting per-job type code into
tailored methods. I'm still of the opinion the fork should happen in here
though, so we have a single choke point, but we can revisit later.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:1076: // TODO(tim): ?! This must have been
broken.  The way DecideOnJob works today
On 2012/10/08 19:18:04, rlarocque wrote:
> On 2012/10/08 00:20:03, timsteele wrote:
> > On 2012/09/24 21:15:14, rlarocque wrote:
> > > Agreed, but this comment won't make much sense other than in the context
of
> > this
> > > diff.
> > 
> > Ah, right.  I had left this as a comment for myself while working.  Reworded
> and
> > filed bug.
> 
> On second thought, this function resets the wait_interval_.  IIRC, the flag
that
> indicates we're currently "globally" throttled is the wait_interval_'s mode. 
> Wouldn't clearing the wait_interval_ get us out of the throttled state?
> 
> (Granted, it may be too late for this to matter for the Canary job.  However,
it
> should take effect eventually, right?)

You're right. It should mean we're good to go on the next nudge. I updated the
bug (155296).

[rlarocque, 2012-10-11 18:15:08]

I still haven't reviewed the job ownership semantics very thoroughly, though I
did take a second look and didn't see any obvious problems.  Based on previous
comments, I get the impression that Fred has taken the time to understand that
logic, and I'm letting him handle that part of the review.

My comments have been either addressed or deferred, so LGTM.  I'll look forward
to seeing this change landed.

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_...
sync/engine/sync_scheduler_impl.cc:730: if (job->purpose() ==
SyncSessionJob::NUDGE) {
On 2012/10/11 17:35:14, timsteele wrote:
> On 2012/10/08 19:18:04, rlarocque wrote:
> > On 2012/10/08 00:20:03, timsteele wrote:
> > > On 2012/09/24 21:15:14, rlarocque wrote:
> > > > I get the feeling that there's not much point in sharing this function
> > between
> > > > CONFIGURE and non-CONFIGURE jobs.  Most of the code in here is
irrelevant
> to
> > > > either one job or the other.
> > > 
> > > There's still a chunk of common code, but also, for now I'd like to leave
it
> > so
> > > that the callsites of DoSyncSessionJob don't need to check the job type,
and
> > be
> > > able to state "DoSyncSessionJob" as the thing that takes ownership of
> running
> > > and ending a job.  We could potentially break it up into one-per type but
> > still
> > > leave DoSyncSessionJob as the top level call that forks internally to
> address
> > my
> > > concerns, (and make it that much simpler if we ever had to add a new job
> type,
> > > though this is a lower priority concern).
> > 
> > I see your point, but I think the call sites should be easy to fix.  They
> > already know which kind of job is being scheduled.
> > 
> > -> ScheduleConfiguration() only call DoSyncSessionJob on CONFIGURE jobs.
> > -> ScheduleSyncSessionJob() only deals with POLL and NUDGE jobs.
> > -> Except for the part where it calls DoSyncSessionJob,
> DoPendingJobIfPossible()
> > is one big if statement with branches for CONFIGURATION_MODE and
NORMAL_MODE.
> 
> I'm not convinced we should do this yet.  I still see common code in here, I
> think it is reasonable to assume we'll want to add common code to all jobs in
> the future (random thought - the minute we want more UMA), and for now while I
> wrap up this patch it's easiest to say "DoSyncSessionJob" takes ownership if
the
> job is scheduled, else unscheduled_nudge_storage.
> 
> I can add a TODO mentioning we should consider splitting per-job type code
into
> tailored methods. I'm still of the opinion the fork should happen in here
> though, so we have a single choke point, but we can revisit later.

I think it would be easier to define a function "DoCommonJobTasks()" and call it
from both paths.  

I agree that we should revisit later.

[tim (not reviewing), 2012-10-16 17:29:27]

ping!

[akalin, 2012-10-16 18:00:54]

On 2012/10/16 17:29:27, timsteele wrote:
> ping!

will look later today!

[akalin, 2012-10-17 17:39:17]

Few more comments, some possibly naive questions.

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:368:
SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob* job) {
why does this need a pointer?

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:407: const SyncSessionJob* job) {
why does this need a pointer?

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:495: bool
SyncSchedulerImpl::ShouldRunJobSaveIfNecessary(SyncSessionJob* job) {
the name for this is unclear -- "ShouldRunJob" implies that it's just checks a
condition, but the "SaveIfNecessary" part seems more important.  Perhaps just
rename to "SaveIfNecessary" and comment re. the return value?

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:631: if
(!ShouldRunJobSaveIfNecessary(job.get()))
Naively, I'd expect to be able to split this call up like this:

If (!ShouldRunJob(*job))
  return;

// Abandons job and save a clone.
Save(job);


Anything wrong with that?

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:654: ScheduleSyncSessionJob(job.Pass());
Isn't the job abandoned at this point?  Do we still need to schedule it?

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.h (right):

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:52: const base::TimeDelta& desired_delay,
no const ref!

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:57: const base::TimeDelta& desired_delay,
NudgeSource source,
no const ref!

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:307: scoped_ptr<SyncSessionJob>
unscheduled_nudge_storage_;
so it looks like this is the backing store for pending_nudge_ and/or
pending_configure_job_, right?  Instead of having those pointers, is it possible
to rename this var appropriately (say, pending_job_) and have flags that
indicate whether this is a pending nudge and/or configure job?  Then we can
maybe provide accessors like SyncSessionJob* GetPendingNudge() { if
(is_pending_nudge_) return pending_job_.get(); } or check the flags directly.

But this way, the ownership is clear -- there's no lingering question like "is
there a chance that this raw pointer could be dangling?".  What do you think?

[tim (not reviewing), 2012-10-17 20:33:32]

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:368:
SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob* job) {
On 2012/10/17 17:39:17, akalin wrote:
> why does this need a pointer?

Every callsite has a pointer, so it seems simpler / cleaner?

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:407: const SyncSessionJob* job) {
On 2012/10/17 17:39:17, akalin wrote:
> why does this need a pointer?

Every callsite has a pointer, so it seems simpler / cleaner?

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:495: bool
SyncSchedulerImpl::ShouldRunJobSaveIfNecessary(SyncSessionJob* job) {
On 2012/10/17 17:39:17, akalin wrote:
> the name for this is unclear -- "ShouldRunJob" implies that it's just checks a
> condition, but the "SaveIfNecessary" part seems more important.  Perhaps just
> rename to "SaveIfNecessary" and comment re. the return value?

SaveIfNecessary works, although there's also the case where it returns false and
doesn't save.  The return value is really there to tell the caller what to do.

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:631: if
(!ShouldRunJobSaveIfNecessary(job.get()))
On 2012/10/17 17:39:17, akalin wrote:
> Naively, I'd expect to be able to split this call up like this:
> 
> If (!ShouldRunJob(*job))
>   return;
> 
> // Abandons job and save a clone.
> Save(job);
> 
> 
> Anything wrong with that?

Yes.  !ShouldRunJob doesn't mean you can forget about the job, it means you
shouldn't run it right now.  

What could be done is to call DecideOnJob and call a save function if it needs
to be saved.  I was explicitly trying to avoid having a 'SaveJob' function
floating around like before, because we wound up wanting to call it all over the
place and added some special cases to it that led to comments like "it's weird
that save doesn't do what it says sometimes".  The only things that really need
the save behavior are the two pre-scheduling cases, and in this case I think
inlining the saving code right after the call to DecideOnJob in ShouldRun is
clearest, and is least likely to be abused because the method is pretty targeted
at that use case.  

But if that leaves you unconvinced I'm not opposed.

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:654: ScheduleSyncSessionJob(job.Pass());
On 2012/10/17 17:39:17, akalin wrote:
> Isn't the job abandoned at this point?  Do we still need to schedule it?

Err, did you mean to put this comment elsewhere? This is actually _the_ main
place jobs get scheduled?

[akalin, 2012-10-17 22:55:45]

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:368:
SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob* job) {
On 2012/10/17 20:33:32, timsteele wrote:
> On 2012/10/17 17:39:17, akalin wrote:
> > why does this need a pointer?
> 
> Every callsite has a pointer, so it seems simpler / cleaner?

I don't know...const references are the usual style, unless the pointer value is
nullable or otherwise used.  I was scanning the function wondering how it was
used that made it necessary for a pointer instead of a reference. :/

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:407: const SyncSessionJob* job) {
On 2012/10/17 20:33:32, timsteele wrote:
> On 2012/10/17 17:39:17, akalin wrote:
> > why does this need a pointer?
> 
> Every callsite has a pointer, so it seems simpler / cleaner?

same.

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:495: bool
SyncSchedulerImpl::ShouldRunJobSaveIfNecessary(SyncSessionJob* job) {
On 2012/10/17 20:33:32, timsteele wrote:
> On 2012/10/17 17:39:17, akalin wrote:
> > the name for this is unclear -- "ShouldRunJob" implies that it's just checks
a
> > condition, but the "SaveIfNecessary" part seems more important.  Perhaps
just
> > rename to "SaveIfNecessary" and comment re. the return value?
> 
> SaveIfNecessary works, although there's also the case where it returns false
and
> doesn't save.  The return value is really there to tell the caller what to do.

I see.  See my comment below.

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:631: if
(!ShouldRunJobSaveIfNecessary(job.get()))
On 2012/10/17 20:33:32, timsteele wrote:
> On 2012/10/17 17:39:17, akalin wrote:
> > Naively, I'd expect to be able to split this call up like this:
> > 
> > If (!ShouldRunJob(*job))
> >   return;
> > 
> > // Abandons job and save a clone.
> > Save(job);
> > 
> > 
> > Anything wrong with that?
> 
> Yes.  !ShouldRunJob doesn't mean you can forget about the job, it means you
> shouldn't run it right now.  
> 
> What could be done is to call DecideOnJob and call a save function if it needs
> to be saved.  I was explicitly trying to avoid having a 'SaveJob' function
> floating around like before, because we wound up wanting to call it all over
the
> place and added some special cases to it that led to comments like "it's weird
> that save doesn't do what it says sometimes".  The only things that really
need
> the save behavior are the two pre-scheduling cases, and in this case I think
> inlining the saving code right after the call to DecideOnJob in ShouldRun is
> clearest, and is least likely to be abused because the method is pretty
targeted
> at that use case.  
> 
> But if that leaves you unconvinced I'm not opposed.

I see.  See comment below...

http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:654: ScheduleSyncSessionJob(job.Pass());
On 2012/10/17 20:33:32, timsteele wrote:
> On 2012/10/17 17:39:17, akalin wrote:
> > Isn't the job abandoned at this point?  Do we still need to schedule it?
> 
> Err, did you mean to put this comment elsewhere? This is actually _the_ main
> place jobs get scheduled?

Right, I was misunderstanding the flow.

So it looks like we create a new job, we pass it to ShouldRunJobSaveIfNecessary;
if it returns false, we exit early (and destroy the job, which may have been
cloned and stashed somewhere).  If it returns true, we continue to schedule the
job, passing ownership to ScheduleSyncSessionJob.

What about making ShouldRunJobSaveIfNecessary take ownership of job, then have
it return a scoped_ptr<Job> that may be null -- if it's not null, that means we
should continue to schedule the job.  If it's null, we may or may not have
saved/cloned the job somewhere, but we're not going to run it now.

Maybe it can then be renamed to GetJobToRun()?  WDYT?

I guess it'd look something lke:

scoped_ptr<SyncSessionJob> new_job(...);

scoped_ptr<SyncSessionJob> job_to_run(GetJobToRun(new_job));

if (pending_nudge_) {
  // coalesce into job_to_run
}

ScheduleSyncSessionJob(job_to_run.Pass());

[tim (not reviewing), 2012-10-24 23:39:17]

Addressed Fred's last review comments as per offline discussion, by splitting
ShouldRunJobSaveIfNecessary into DecideOnJob and HandleSaveJobDecision.

Please take a look.

[akalin, 2012-10-26 06:52:29]

okay i think this lgtm after nits

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:278:
pending->mutable_session()->Coalesce(*(session));
just *session

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:438: return SAVE; // Running requires a mode
switch.
two spaces before //

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:496: DCHECK_EQ(DecideOnJob(*job.get()),
SAVE);
just *job

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:616: JobProcessDecision decision =
DecideOnJob(*job.get());
just *job

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:750: JobProcessDecision decision =
DecideOnJob(*job.get());
*job (here and elsewhere)

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:807: SYNC_DATA_TYPE_HISTOGRAM(iter->first);
indent?

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.h (right):

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:143: SyncSessionJob* pending_configure_job;
i don't like these raw pointers, but i guess there's no other way

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:167: // reset our state.  |premature_exit| is
true if the Syncer did not manage
update comment (premature_exit -> exited_prematurely)

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:185: void HandleContinuationError(const
SyncSessionJob* old_job);
const ref, please!

actually, looking at this again, maybe this should take ownership of old_job?

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:190: // If DecideOnJob decides that |job|
should be SAVEd, this function will
indentation

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:228: scoped_ptr<MessageLoop>
message_loop_;
why'd you have to do this?

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_whitebox_unittest.cc (right):

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_whitebox_unittest.cc:95: const SyncSessionJob* job) {
const ref please

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_session_j...
File sync/engine/sync_session_job.cc (right):

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_session_j...
sync/engine/sync_session_job.cc:66: DCHECK(state.last_download_updates_result !=
UNSET &&
split into two DCHECK_NEs?

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_session_j...
sync/engine/sync_session_job.cc:128: const sessions::SyncSession*
SyncSessionJob::session() const {
const ref?

[tim (not reviewing), 2012-10-26 22:52:27]

Addressed all your points as requested minus one, which I commented below. I'm
planning to try and land this in the meantime but I'm happy to pick up and keep
going to make that change in a follow up.

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:228: scoped_ptr<MessageLoop>
message_loop_;
On 2012/10/26 06:52:29, akalin wrote:
> why'd you have to do this?

Ah, leftover from some debugging.  Undone.

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_session_j...
File sync/engine/sync_session_job.cc (right):

http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_session_j...
sync/engine/sync_session_job.cc:128: const sessions::SyncSession*
SyncSessionJob::session() const {
On 2012/10/26 06:52:29, akalin wrote:
> const ref?

We use session() for equality checks in several places right now, so it's
convenient to use a pointer, but yeah, this should probably be made to be a
const &.  In fact, I'm considering removing the need to compare equality and
just add an abandoned flag to this class.  I'd like to address that in a follow
up if that's okay, though? This has been dragging around for a while (and we
were previously doing pointer comparisons, too, so it's not a regression), and
I'd like to checkpoint.

[commit-bot: I haz the power, 2012-10-26 23:11:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10917234/56004

[commit-bot: I haz the power, 2012-10-27 03:09:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-27 23:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10917234/68001

[commit-bot: I haz the power, 2012-10-28 13:05:43]

Change committed as 164565