Channel all low lever scheduling calls through single function

Channel all low lever scheduling calls through single function

The idea of this change is to only call Do***SyncSessionJob from single
function TryJob and have all other functions in class call it. The hope
is that maybe someday we'll call it asynchronously.

Previously it would look like this:
http://www.plantuml.com/plantuml/svg/ZP5HJiCm38RVSue-0jAkW0JRa8OGJCquGDQuRKPg5pUTbjkJfXfQrzBnvlZ_REUtfLFiaiLZygGFRUEsPAt1GWYPbQ1R2s-KAVlUwWAIHQ3EwPZPLBKTWYda34sPmdSBZRjHiS6ZiaOhPmZd9eXHAe9Ru9ouMe5L7dJKnC14JuLuPohWfR9shx8lyOckP7BEzlDsx9SA5VjNselrgIO4T4RPvy1IdgVWs8_13vJ0nz2L427h7AoPYL_C2VIO5fUlAAyeLEQcQ3dQbnup2KqJmewVmAE_srXdzueIiZ_ntsry3LTBcGOdDZv3_vvjuFGVxiNePZFcPbl5khk7Up7vmmjrtFai_W00

Now it will look like this:
http://www.plantuml.com/plantuml/svg/XP5HJiGW48RVEKKVrMIlOAAxQxB6wARK0x1bsg9Jg5FOBBUNqbHRsTGtcE-3uH-u5MIppNts83zSRuy6KUcQCLus81t2Yoa5lZfPGxO9rbIEbKFRuLBCeIBecno-7FJsdnC7VHQefB3AwBKEB2b5yGXK6MftWCA3Z8mjGenMM1Ug1TeAn9CeFzcxjWqPQt7iLv3V2Yt8FviJsryweq5R9V0nkAQgSh3aK_5DSw1pkDLe3UMGO4zaw4bTGAOsc-ycF83Yjx8puQvArOxYGBZN9OU-3yj4Z5dNX27q7yxu-D6pthw3T1O3kJBPEVxvZyM35qOozGLTZHhDcxjRTZNdZAOX9_WD
The change should be roughly equivalent to today's behavior.

BUG=259913
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235814

[rlarocque, 2013-11-16 02:06:17]

This seems like a pretty safe way to approach this transition.

LGTM.

[tim (not reviewing), 2013-11-17 00:36:46]

LG with nits

https://codereview.chromium.org/68013007/diff/1/sync/engine/sync_scheduler_im...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/68013007/diff/1/sync/engine/sync_scheduler_im...
sync/engine/sync_scheduler_impl.cc:675: void
SyncSchedulerImpl::TryJob(JobPriority priority) {
I like it. I've always been a fan of having a consolidation point like this,
even just to wrap my head around.

We could be more consistent in terms of naming with the Do*SyncSessionJob
functions though, e.g TrySyncSessionJob

https://codereview.chromium.org/68013007/diff/1/sync/engine/sync_scheduler_im...
sync/engine/sync_scheduler_impl.cc:688: >= GetPollInterval()) ||
do_poll_after_credentials_updated_) {
Whoa, this is hard to read.  Consider pulling Now() - last_poll_reset_ into a
variable.

I'm not sure if it's stipulated anywhere, but in general in chromium the >=
would go on the previous line.

Also, the indent is off.  The GetPollInterval() line should go 4 spaces from the
left of the previous line.

Maybe if you put the "do_poll_after_credentials_updated_ ||" first, the rest
won't look so messy.

[pavely, 2013-11-18 18:39:54]

https://codereview.chromium.org/68013007/diff/1/sync/engine/sync_scheduler_im...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/68013007/diff/1/sync/engine/sync_scheduler_im...
sync/engine/sync_scheduler_impl.cc:675: void
SyncSchedulerImpl::TryJob(JobPriority priority) {
On 2013/11/17 00:36:47, timsteele wrote:
> I like it. I've always been a fan of having a consolidation point like this,
> even just to wrap my head around.
> 
> We could be more consistent in terms of naming with the Do*SyncSessionJob
> functions though, e.g TrySyncSessionJob

Done.

https://codereview.chromium.org/68013007/diff/1/sync/engine/sync_scheduler_im...
sync/engine/sync_scheduler_impl.cc:688: >= GetPollInterval()) ||
do_poll_after_credentials_updated_) {
On 2013/11/17 00:36:47, timsteele wrote:
> Whoa, this is hard to read.  Consider pulling Now() - last_poll_reset_ into a
> variable.
> 
> I'm not sure if it's stipulated anywhere, but in general in chromium the >=
> would go on the previous line.
> 
> Also, the indent is off.  The GetPollInterval() line should go 4 spaces from
the
> left of the previous line.
> 
> Maybe if you put the "do_poll_after_credentials_updated_ ||" first, the rest
> won't look so messy.
I reordered conditions, decided not to extract subtraction into variable because
this way it will be calculated and potentially not used.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/68013007/70001

[commit-bot: I haz the power, 2013-11-18 20:54:18]

Change committed as 235814