Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12538015:
sync: Handle POLL jobs in separate a code path (Closed)
|
Created:
7 years, 9 months ago by rlarocque Modified:
7 years, 9 months ago Reviewers:
tim (not reviewing) CC:
chromium-reviews, Raghu Simha, haitaol1, akalin, tim (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionsync: Handle POLL jobs in separate a code path
This is part of the effort to remove the amount of state we track in
SyncSessionJob. We eventually want to remove SyncSessionJob's 'purpose'
member. Splitting the handling of poll, configure, and nudge jobs into
separate functions is a necessary step towards that goal.
We're not ready to remove 'purpose' yet, but we can start by separating
all the logic that deals with POLL jobs. It will be easier to refactor
job decision-making, scheduling, saving, etc. if those functions don't
need to support poll jobs.
This change is almost, but not quite, a no-op. The only notable change
is that the poll timer timeout no longer posts a task. Instead, it will
run the poll job directly.
A side-effect of this change was that ScheduleNextSync() was only called
from one location (ScheduleNudgeImpl()), so its was merged into its only
call site.
BUG=175024
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189398
Patch Set 1 #
Total comments: 19
Patch Set 2 : Review responses #Patch Set 3 : Rebase #
Total comments: 7
Patch Set 4 : Second round of review fixes #
Total comments: 4
Patch Set 5 : Amend comments #
Messages
Total messages: 13 (0 generated)
Please review. As noted in the commit message, this should be mostly a no-op. I admit that this is not entirely obvious when skimming through the diffs. Let me know if you'd like access to the stack of patches that have been squashed into this CL, or if you'd like to discuss these changes. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... File sync/engine/sync_scheduler_impl.cc (right): https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:756: bool success = FinishSyncSessionJob(job.get(), premature_exit); One of the goals of this patch is to take all the poll-specific decision-making out of FinishSyncSessionJob() and ScheduleNextSync(). I admit this change seems a bit out of place in this patch, but I couldn't find any better place to put it. To implement this, it makes ScheduleNextSync() a function that always resets the backoff interval, and inlines most of the rest of its functionality into DoSyncSessionJob(). The few parts of ShceduleNextSync() that apply to poll jobs were inlined into DoPollSyncSessionJob(). FinishSyncSessionJob no longer calls ScheduleNextSync() directly, and it is called by both DoSyncSessionJob and DoPollSyncSessionJob(). https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:787: if (wait_interval_.get()) { The if statements on lines 787-800 are intended to be equivalent to DecideOnJob's old decision making. We never saved a POLL job, so the only decision to be made is whether to drop it (return immediately) or continue (don't return). https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_wh... File sync/engine/sync_scheduler_whitebox_unittest.cc (left): https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_wh... sync/engine/sync_scheduler_whitebox_unittest.cc:177: TEST_F(SyncSchedulerWhiteboxTest, DropPoll) { I think it's safe to drop these tests, since the logic should be well covered by the non-whitebox SyncSchedulerTests like SyncSchedulerTest.BackoffDropsJobs, SyncSchedulerTest.ConfigureMode. If you're really concerned about this, I could be convinced to factor the poll decision-making into a separate function for test purposes.
I haven't fully thought this through yet, but I thought I'd send along some quick initial comments since I have to run to a meeting and may not get a chance to fully digest it until tomorrow morning. I think the direction / goals make sense though. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... File sync/engine/sync_scheduler_impl.cc (right): https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:417: else // job.purpose() == SyncSessionJob::CONFIGURATION I think you mean to delete this. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:682: void SyncSchedulerImpl::ScheduleSyncSessionJob( How about we call this PostNudgeJob, or something, to distinguish it from the "ScheduleNudge" APIs that may not proceed with scheduling for whatever reason, and to make the fact that it is only used for NUDGE jobs self-explanatory. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:756: bool success = FinishSyncSessionJob(job.get(), premature_exit); On 2013/03/18 22:02:18, rlarocque wrote: > One of the goals of this patch is to take all the poll-specific decision-making > out of FinishSyncSessionJob() and ScheduleNextSync(). I admit this change seems > a bit out of place in this patch, but I couldn't find any better place to put > it. > > To implement this, it makes ScheduleNextSync() a function that always resets the > backoff interval, and inlines most of the rest of its functionality into > DoSyncSessionJob(). The few parts of ShceduleNextSync() that apply to poll jobs > were inlined into DoPollSyncSessionJob(). > > FinishSyncSessionJob no longer calls ScheduleNextSync() directly, and it is > called by both DoSyncSessionJob and DoPollSyncSessionJob(). Please audit all the comments that reference DoSyncSessionJob and update them to (e.g) Do[Poll]SyncSessionJob as necessary. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:775: if (!success) { nit - please make { } usage consistent for single line ifs in this file. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:787: if (wait_interval_.get()) { On 2013/03/18 22:02:18, rlarocque wrote: > The if statements on lines 787-800 are intended to be equivalent to > DecideOnJob's old decision making. We never saved a POLL job, so the only > decision to be made is whether to drop it (return immediately) or continue > (don't return). I know this may seem pedantic, but I'd like to try really hard to keep logic of when to DROP / SAVE jobs in one place. At one time we had it split-up just like this. It was harder to grasp all the different reasons we'd decide to continue / save / drop by looking at the code without reading the whole file; we would frequently forget about "that other place we let jobs through". Could we a) at least have a DecideOnPollJob, so that we force ourselves to stick to a pattern (DecideOn*Job) and don't scatter more and more of this code around the class, and b) ideally call it from DecideOnJob if a parameter was POLL, so that we still have one single gauntlet (to examine, to tweak if new requirements arise for existing jobs (or possibly new job types down the road), to test). Maybe you can suggest something better, but I'd like it if the spirit of that reasoning wasn't lost. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:812: RestartWaiting(scoped_ptr<SyncSessionJob>()); Comment that we send a NULL job here because a POLL job does not warrant a canary, but we still want to make sure our bookkeeping is up to date so that any other jobs that come in are unblocked. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:863: return succeeded; Just fall through? https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:1074: NOTREACHED() << "Illegal to schedule job while session in progress."; Can you explain why we can't be here, given that this is a timer callback and hence not an explicit synchronous call from any of the logic in the scheduler? It's not obvious how we "protect" against this value being true when an essentially randomly-timed callback fires. (i.e because if we're processing the callback task and no_scheduling_allowed_ is true, it would imply we either forgot to unset it or we're in DoSyncSessionJob and calling this method explicitly, which would be wrong).
Added some responses. I will update the patch and upload it soon. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... File sync/engine/sync_scheduler_impl.cc (right): https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:682: void SyncSchedulerImpl::ScheduleSyncSessionJob( On 2013/03/18 22:57:40, timsteele wrote: > How about we call this PostNudgeJob, or something, to distinguish it from the > "ScheduleNudge" APIs that may not proceed with scheduling for whatever reason, > and to make the fact that it is only used for NUDGE jobs self-explanatory. I'd like to merge this into ScheduleNudgeImpl. If you prefer, I could do that as part of this patch. I think that would be a better solution that just renaming it. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:775: if (!success) { On 2013/03/18 22:57:40, timsteele wrote: > nit - please make { } usage consistent for single line ifs in this file. I think it's been inconsistent for a while. For example, see lines 825 and and 836 in the old file. I can remove the brackets here. That would at least make this function consistent. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:787: if (wait_interval_.get()) { On 2013/03/18 22:57:40, timsteele wrote: > On 2013/03/18 22:02:18, rlarocque wrote: > > The if statements on lines 787-800 are intended to be equivalent to > > DecideOnJob's old decision making. We never saved a POLL job, so the only > > decision to be made is whether to drop it (return immediately) or continue > > (don't return). > > I know this may seem pedantic, but I'd like to try really hard to keep logic of > when to DROP / SAVE jobs in one place. At one time we had it split-up just like > this. It was harder to grasp all the different reasons we'd decide to continue / > save / drop by looking at the code without reading the whole file; we would > frequently forget about "that other place we let jobs through". > > Could we a) at least have a DecideOnPollJob, so that we force ourselves to stick > to a pattern (DecideOn*Job) and don't scatter more and more of this code around > the class, and b) ideally call it from DecideOnJob if a parameter was POLL, so > that we still have one single gauntlet (to examine, to tweak if new requirements > arise for existing jobs (or possibly new job types down the road), to test). > > Maybe you can suggest something better, but I'd like it if the spirit of that > reasoning wasn't lost. I see your point. I agree that we want to keep all the decision-making logic in one place and all the job running logic in a different place. However, I think that the decision-making is handled differently in different contexts, and trying to merge them all can have its own set of problems. For example, we have crbug.com/179515, which may have come about because it made sense to DROP a job in one context, but not another. There is also crbug.com/177659, where a decision to return "SAVE" made sense for some contexts where DecideOnJob() was being called, but not others. In other words, it's a tradeoff. Both approaches have potential for bugs. I'm hoping that the other changes coming up will reduce the probability of rogue jobs. For one, I plan to limit the number of jobs currently in flight to one. I'd also like to decouple the save/drop logic from the run/don't run logic. Your suggestion to add a DecideOnPollJob() makes sense. All logic for a given purpose (poll, nudge, or config) should be in one place. For example, that means that we'll probably need to keep a "DecideOnNudgeJob" function to share logic between ScheduleNudgeImpl() and DoNudgeSyncSessionJob(). However, I don't see much benefit in sharing code between purpose, since their decision logic is so different. If you're still not convinced, we should talk in person about this. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:812: RestartWaiting(scoped_ptr<SyncSessionJob>()); On 2013/03/18 22:57:40, timsteele wrote: > Comment that we send a NULL job here because a POLL job does not warrant a > canary, but we still want to make sure our bookkeeping is up to date so that any > other jobs that come in are unblocked. Will do. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:863: return succeeded; On 2013/03/18 22:57:40, timsteele wrote: > Just fall through? Will do. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:1074: NOTREACHED() << "Illegal to schedule job while session in progress."; On 2013/03/18 22:57:40, timsteele wrote: > Can you explain why we can't be here, given that this is a timer callback and > hence not an explicit synchronous call from any of the logic in the scheduler? > It's not obvious how we "protect" against this value being true when an > essentially randomly-timed callback fires. > > (i.e because if we're processing the callback task and no_scheduling_allowed_ is > true, it would imply we either forgot to unset it or we're in DoSyncSessionJob > and calling this method explicitly, which would be wrong). The no_scheduling_allowed_ flag is true only when a function that runs only on the sync thread is currently running. Because this callback fires only on the sync thread, we can't be running such a function when this callback is being executed. I'll add a comment to document this fact. By the way, the logic to maintain this flag is a bit strange. We end up unsetting the flag in FinishSyncSessionJob while the protector declared on the stack in DoSyncSessionJob is still in scope. I don't know if that's intentional or not.
On 2013/03/19 00:15:02, rlarocque wrote: > Added some responses. I will update the patch and upload it soon. > > https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... > File sync/engine/sync_scheduler_impl.cc (right): > > https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... > sync/engine/sync_scheduler_impl.cc:682: void > SyncSchedulerImpl::ScheduleSyncSessionJob( > On 2013/03/18 22:57:40, timsteele wrote: > > How about we call this PostNudgeJob, or something, to distinguish it from the > > "ScheduleNudge" APIs that may not proceed with scheduling for whatever reason, > > and to make the fact that it is only used for NUDGE jobs self-explanatory. > > I'd like to merge this into ScheduleNudgeImpl. If you prefer, I could do that > as part of this patch. I think that would be a better solution that just > renaming it. > > https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... > sync/engine/sync_scheduler_impl.cc:775: if (!success) { > On 2013/03/18 22:57:40, timsteele wrote: > > nit - please make { } usage consistent for single line ifs in this file. > > I think it's been inconsistent for a while. For example, see lines 825 and and > 836 in the old file. > > I can remove the brackets here. That would at least make this function > consistent. > > https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... > sync/engine/sync_scheduler_impl.cc:787: if (wait_interval_.get()) { > On 2013/03/18 22:57:40, timsteele wrote: > > On 2013/03/18 22:02:18, rlarocque wrote: > > > The if statements on lines 787-800 are intended to be equivalent to > > > DecideOnJob's old decision making. We never saved a POLL job, so the only > > > decision to be made is whether to drop it (return immediately) or continue > > > (don't return). > > > > I know this may seem pedantic, but I'd like to try really hard to keep logic > of > > when to DROP / SAVE jobs in one place. At one time we had it split-up just > like > > this. It was harder to grasp all the different reasons we'd decide to continue > / > > save / drop by looking at the code without reading the whole file; we would > > frequently forget about "that other place we let jobs through". > > > > Could we a) at least have a DecideOnPollJob, so that we force ourselves to > stick > > to a pattern (DecideOn*Job) and don't scatter more and more of this code > around > > the class, and b) ideally call it from DecideOnJob if a parameter was POLL, so > > that we still have one single gauntlet (to examine, to tweak if new > requirements > > arise for existing jobs (or possibly new job types down the road), to test). > > > > Maybe you can suggest something better, but I'd like it if the spirit of that > > reasoning wasn't lost. > > I see your point. I agree that we want to keep all the decision-making logic in > one place and all the job running logic in a different place. > > However, I think that the decision-making is handled differently in different > contexts, and trying to merge them all can have its own set of problems. For > example, we have crbug.com/179515, which may have come about because it made > sense to DROP a job in one context, but not another. There is also > crbug.com/177659, where a decision to return "SAVE" made sense for some contexts > where DecideOnJob() was being called, but not others. > > In other words, it's a tradeoff. Both approaches have potential for bugs. > > I'm hoping that the other changes coming up will reduce the probability of rogue > jobs. For one, I plan to limit the number of jobs currently in flight to one. > I'd also like to decouple the save/drop logic from the run/don't run logic. > > Your suggestion to add a DecideOnPollJob() makes sense. All logic for a given > purpose (poll, nudge, or config) should be in one place. For example, that > means that we'll probably need to keep a "DecideOnNudgeJob" function to share > logic between ScheduleNudgeImpl() and DoNudgeSyncSessionJob(). However, I don't > see much benefit in sharing code between purpose, since their decision logic is > so different. > > If you're still not convinced, we should talk in person about this. > > https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... > sync/engine/sync_scheduler_impl.cc:812: > RestartWaiting(scoped_ptr<SyncSessionJob>()); > On 2013/03/18 22:57:40, timsteele wrote: > > Comment that we send a NULL job here because a POLL job does not warrant a > > canary, but we still want to make sure our bookkeeping is up to date so that > any > > other jobs that come in are unblocked. > > Will do. > > https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... > sync/engine/sync_scheduler_impl.cc:863: return succeeded; > On 2013/03/18 22:57:40, timsteele wrote: > > Just fall through? > > Will do. > > https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... > sync/engine/sync_scheduler_impl.cc:1074: NOTREACHED() << "Illegal to schedule > job while session in progress."; > On 2013/03/18 22:57:40, timsteele wrote: > > Can you explain why we can't be here, given that this is a timer callback and > > hence not an explicit synchronous call from any of the logic in the scheduler? > > > It's not obvious how we "protect" against this value being true when an > > essentially randomly-timed callback fires. > > > > (i.e because if we're processing the callback task and no_scheduling_allowed_ > is > > true, it would imply we either forgot to unset it or we're in DoSyncSessionJob > > and calling this method explicitly, which would be wrong). > > The no_scheduling_allowed_ flag is true only when a function that runs only on > the sync thread is currently running. Because this callback fires only on the > sync thread, we can't be running such a function when this callback is being > executed. > > I'll add a comment to document this fact. > > By the way, the logic to maintain this flag is a bit strange. We end up > unsetting the flag in FinishSyncSessionJob while the protector declared on the > stack in DoSyncSessionJob is still in scope. I don't know if that's intentional > or not. I've uploaded a new patch. I've also discovered a bug in this patch that confirms both our our concerns about decision-making. Let me know when you have a chance to talk.
> I've uploaded a new patch. > > I've also discovered a bug in this patch that confirms both our our concerns > about decision-making. Let me know when you have a chance to talk. It turns out the patch was right the first time. Per-datatype throttle checking depends on purpose == NUDGE and source == LOCAL. Ignoring per-datatype throttling in DecideOnPollJob() will maintain current behaviour. PTAL.
LGTM with nits below and also fix the comment on line 417. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... File sync/engine/sync_scheduler_impl.cc (right): https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:682: void SyncSchedulerImpl::ScheduleSyncSessionJob( On 2013/03/19 00:15:03, rlarocque wrote: > On 2013/03/18 22:57:40, timsteele wrote: > > How about we call this PostNudgeJob, or something, to distinguish it from the > > "ScheduleNudge" APIs that may not proceed with scheduling for whatever reason, > > and to make the fact that it is only used for NUDGE jobs self-explanatory. > > I'd like to merge this into ScheduleNudgeImpl. If you prefer, I could do that > as part of this patch. I think that would be a better solution that just > renaming it. OK, lets merge it there. Make sure you update comments that speak of ScheduleSyncSessionJob. https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... File sync/engine/sync_scheduler_impl.cc (right): https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... sync/engine/sync_scheduler_impl.cc:792: if (session_context_->connection_manager()->HasInvalidAuthToken()) { Can you add a TODO to track moving this to a DoCommonChecks function? https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... sync/engine/sync_scheduler_impl.cc:805: if (!DecideOnPollJob()) Is your plant o make DecideOnJob() return a bool at some point? If so, verbiage like "ShouldRunJob" may be more appropriate. But we can keep this for now for consistency.
https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... File sync/engine/sync_scheduler_whitebox_unittest.cc (left): https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... sync/engine/sync_scheduler_whitebox_unittest.cc:179: SetMode(SyncScheduler::CONFIGURATION_MODE); This still seems like a good basic test to have...
Patch updated, though I won't try to commit until tomorrow. This patch has changed a lot since the original upload. I'd like to rerun some tests before I send it off to the CQ. https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... File sync/engine/sync_scheduler_impl.cc (right): https://codereview.chromium.org/12538015/diff/1/sync/engine/sync_scheduler_im... sync/engine/sync_scheduler_impl.cc:682: void SyncSchedulerImpl::ScheduleSyncSessionJob( On 2013/03/19 21:50:02, timsteele wrote: > On 2013/03/19 00:15:03, rlarocque wrote: > > On 2013/03/18 22:57:40, timsteele wrote: > > > How about we call this PostNudgeJob, or something, to distinguish it from > the > > > "ScheduleNudge" APIs that may not proceed with scheduling for whatever > reason, > > > and to make the fact that it is only used for NUDGE jobs self-explanatory. > > > > I'd like to merge this into ScheduleNudgeImpl. If you prefer, I could do that > > as part of this patch. I think that would be a better solution that just > > renaming it. > > OK, lets merge it there. Make sure you update comments that speak of > ScheduleSyncSessionJob. Done. The merge wasn't entirely straightforward. I decided to remove redundant DCHECKs and to change a few comments and variable names. It should be a refactoring-only change, though. https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... File sync/engine/sync_scheduler_impl.cc (right): https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... sync/engine/sync_scheduler_impl.cc:792: if (session_context_->connection_manager()->HasInvalidAuthToken()) { On 2013/03/19 21:50:02, timsteele wrote: > Can you add a TODO to track moving this to a DoCommonChecks function? Done. https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... sync/engine/sync_scheduler_impl.cc:805: if (!DecideOnPollJob()) On 2013/03/19 21:50:02, timsteele wrote: > Is your plant o make DecideOnJob() return a bool at some point? If so, verbiage > like "ShouldRunJob" may be more appropriate. But we can keep this for now for > consistency. You're right. This CL is probably the most appropriate place to name this function, so I've updated the name here. https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... sync/engine/sync_scheduler_impl.cc:805: if (!DecideOnPollJob()) On 2013/03/19 21:50:02, timsteele wrote: > Is your plant o make DecideOnJob() return a bool at some point? If so, verbiage > like "ShouldRunJob" may be more appropriate. But we can keep this for now for > consistency. You're right that the name should change. I think that it would be best to choose the correct name from the start, so I've changed it to ShouldPoll() and updated some of the comments. https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... File sync/engine/sync_scheduler_whitebox_unittest.cc (left): https://codereview.chromium.org/12538015/diff/7001/sync/engine/sync_scheduler... sync/engine/sync_scheduler_whitebox_unittest.cc:179: SetMode(SyncScheduler::CONFIGURATION_MODE); On 2013/03/19 21:51:22, timsteele wrote: > This still seems like a good basic test to have... These scenarios are covered by the non-whitebox tests. However, it's not hard to re-add these now that we have a separate ShouldPoll() function. I'll re-add these two tests and a few more. If we're going to test this, then why not go for 100% coverage?
https://codereview.chromium.org/12538015/diff/13001/sync/engine/sync_schedule... File sync/engine/sync_scheduler_impl.h (right): https://codereview.chromium.org/12538015/diff/13001/sync/engine/sync_schedule... sync/engine/sync_scheduler_impl.h:218: // If the scheduler's current state suports it, this will create a job based nit- support. https://codereview.chromium.org/12538015/diff/13001/sync/engine/sync_schedule... sync/engine/sync_scheduler_impl.h:219: // on the passed in parameters and coalesced with any other pending jobs, then nit - "... and coalesce it with..."
https://codereview.chromium.org/12538015/diff/13001/sync/engine/sync_schedule... File sync/engine/sync_scheduler_impl.h (right): https://codereview.chromium.org/12538015/diff/13001/sync/engine/sync_schedule... sync/engine/sync_scheduler_impl.h:218: // If the scheduler's current state suports it, this will create a job based On 2013/03/20 17:08:47, timsteele wrote: > nit- support. Done. https://codereview.chromium.org/12538015/diff/13001/sync/engine/sync_schedule... sync/engine/sync_scheduler_impl.h:219: // on the passed in parameters and coalesced with any other pending jobs, then On 2013/03/20 17:08:47, timsteele wrote: > nit - "... and coalesce it with..." Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlarocque@chromium.org/12538015/18001
Message was sent while issue was closed.
Change committed as 189398 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
