Delay autofill commits to reduce client to server traffic.

chrome/browser/sync/engine/sync_scheduler.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/sync_scheduler.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/bind.h"
@@ skipping 434 matching lines @@
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   PostTask(FROM_HERE, "ScheduleClearUserDataImpl",
            base::Bind(&SyncScheduler::ScheduleClearUserDataImpl,
                       weak_ptr_factory_.GetWeakPtr()));
 }
 
 // TODO(sync): Remove the *Impl methods for the other Schedule*
 // functions, too.
 void SyncScheduler::ScheduleCleanupDisabledTypes() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  ScheduleSyncSessionJob(
-      TimeDelta::FromSeconds(0), SyncSessionJob::CLEANUP_DISABLED_TYPES,
-      CreateSyncSession(SyncSourceInfo()), FROM_HERE);
+  SyncSessionJob job(SyncSessionJob::CLEANUP_DISABLED_TYPES, TimeTicks::Now(),
+                     make_linked_ptr(CreateSyncSession(SyncSourceInfo())),
+                     false,
+                     FROM_HERE);
+  ScheduleSyncSessionJob(job);
 }
 
 void SyncScheduler::ScheduleNudge(
     const TimeDelta& delay,
     NudgeSource source, const ModelTypeBitSet& types,
     const tracked_objects::Location& nudge_location) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SDVLOG_LOC(nudge_location, 2)
       << "Nudge scheduled with delay " << delay.InMilliseconds() << " ms, "
       << "source " << GetNudgeSourceString(source) << ", "
@@ skipping 27 matching lines @@
                       weak_ptr_factory_.GetWeakPtr(),
                       delay,
                       GetUpdatesFromNudgeSource(source),
                       types_with_payloads,
                       false,
                       nudge_location));
 }
 
 void SyncScheduler::ScheduleClearUserDataImpl() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  ScheduleSyncSessionJob(
-      TimeDelta::FromSeconds(0), SyncSessionJob::CLEAR_USER_DATA,
-      CreateSyncSession(SyncSourceInfo()), FROM_HERE);
+  SyncSessionJob job(SyncSessionJob::CLEAR_USER_DATA, TimeTicks::Now(),
+                     make_linked_ptr(CreateSyncSession(SyncSourceInfo())),
+                     false,
+                     FROM_HERE);
+
+  ScheduleSyncSessionJob(job);
 }
 
 void SyncScheduler::ScheduleNudgeImpl(
     const TimeDelta& delay,
     GetUpdatesCallerInfo::GetUpdatesSource source,
     const ModelTypePayloadMap& types_with_payloads,
     bool is_canary_job, const tracked_objects::Location& nudge_location) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   SDVLOG_LOC(nudge_location, 2)
@@ skipping 17 matching lines @@
 
   if (pending_nudge_.get()) {
     if (IsBackingOff() && delay > TimeDelta::FromSeconds(1)) {
       SDVLOG(2) << "Dropping the nudge because we are in backoff";
       return;
     }
 
     SDVLOG(2) << "Coalescing pending nudge";
     pending_nudge_->session->Coalesce(*(job.session.get()));
 
-    if (!IsBackingOff()) {
-      SDVLOG(2) << "Dropping a nudge because"
-                << " we are not in backoff and the job was coalesced";
-      return;
-    } else {
-      SDVLOG(2) << "Rescheduling pending nudge";
-      SyncSession* s = pending_nudge_->session.get();
-      job.session.reset(new SyncSession(s->context(), s->delegate(),
-          s->source(), s->routing_info(), s->workers()));
-      pending_nudge_.reset();
-    }
+    SDVLOG(2) << "Rescheduling pending nudge";
+    SyncSession* s = pending_nudge_->session.get();
+    job.session.reset(new SyncSession(s->context(), s->delegate(),
+        s->source(), s->routing_info(), s->workers()));
+
+    // Choose the start time as the earliest of the 2.
+    job.scheduled_start = std::min(job.scheduled_start,
+                                   pending_nudge_->scheduled_start);
+    pending_nudge_.reset();
   }
 
-  // TODO(lipalani) - pass the job itself to ScheduleSyncSessionJob.
-  ScheduleSyncSessionJob(delay, SyncSessionJob::NUDGE, job.session.release(),
-      nudge_location);
+  ScheduleSyncSessionJob(job);
 }
 
 // Helper to extract the routing info and workers corresponding to types in
 // |types| from |registrar|.
 void GetModelSafeParamsForTypes(const ModelTypeBitSet& types,
     ModelSafeWorkerRegistrar* registrar, ModelSafeRoutingInfo* routes,
     std::vector<ModelSafeWorker*>* workers) {
   ModelSafeRoutingInfo r_tmp;
   std::vector<ModelSafeWorker*> w_tmp;
   registrar->GetModelSafeRoutingInfo(&r_tmp);
@@ skipping 59 matching lines @@
     const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   SDVLOG(2) << "In ScheduleConfigImpl";
   // TODO(tim): config-specific GetUpdatesCallerInfo value?
   SyncSession* session = new SyncSession(session_context_.get(), this,
       SyncSourceInfo(source,
           syncable::ModelTypePayloadMapFromRoutingInfo(
               routing_info, std::string())),
       routing_info, workers);
-  ScheduleSyncSessionJob(TimeDelta::FromSeconds(0),
-      SyncSessionJob::CONFIGURATION, session, FROM_HERE);
+  SyncSessionJob job(SyncSessionJob::CONFIGURATION, TimeTicks::Now(),
+                     make_linked_ptr(session),
+                     false,
+                     FROM_HERE);
+  ScheduleSyncSessionJob(job);
 }
 
 const char* SyncScheduler::GetModeString(SyncScheduler::Mode mode) {
   switch (mode) {
     ENUM_CASE(CONFIGURATION_MODE);
     ENUM_CASE(NORMAL_MODE);
   }
   return "";
 }
 
@@ skipping 25 matching lines @@
   SDVLOG_LOC(from_here, 3) << "Posting " << name << " task with "
                            << delay_ms << " ms delay";
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (!started_) {
     SDVLOG(1) << "Not posting task as scheduler is stopped.";
     return;
   }
   sync_loop_->PostDelayedTask(from_here, task, delay_ms);
 }
 
-void SyncScheduler::ScheduleSyncSessionJob(
-    const base::TimeDelta& delay,
-    SyncSessionJob::SyncSessionJobPurpose purpose,
-    sessions::SyncSession* session,
-    const tracked_objects::Location& from_here) {
+void SyncScheduler::ScheduleSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  SDVLOG_LOC(from_here, 2)
+  TimeDelta delay = job.scheduled_start - TimeTicks::Now();
+  if (delay < TimeDelta::FromMilliseconds(0))
+    delay = TimeDelta::FromMilliseconds(0);
+  SDVLOG_LOC(job.from_here, 2)
       << "In ScheduleSyncSessionJob with "
-      << SyncSessionJob::GetPurposeString(purpose)
+      << SyncSessionJob::GetPurposeString(job.purpose)
       << " job and " << delay.InMilliseconds() << " ms delay";
 
-  SyncSessionJob job(purpose, TimeTicks::Now() + delay,
-                     make_linked_ptr(session), false, from_here);
-  if (purpose == SyncSessionJob::NUDGE) {
-    SDVLOG_LOC(from_here, 2) << "Resetting pending_nudge";
-    DCHECK(!pending_nudge_.get() || pending_nudge_->session.get() == session);
+  if (job.purpose == SyncSessionJob::NUDGE) {
+    SDVLOG_LOC(job.from_here, 2) << "Resetting pending_nudge";
+    DCHECK(!pending_nudge_.get() || pending_nudge_->session.get() ==
+           job.session);
     pending_nudge_.reset(new SyncSessionJob(job));
   }
-  PostDelayedTask(from_here, "DoSyncSessionJob",
+  PostDelayedTask(job.from_here, "DoSyncSessionJob",
                   base::Bind(&SyncScheduler::DoSyncSessionJob,
                              weak_ptr_factory_.GetWeakPtr(),
                              job),
                   delay.InMilliseconds());
 }
 
 void SyncScheduler::SetSyncerStepsForPurpose(
     SyncSessionJob::SyncSessionJobPurpose purpose,
     SyncerStep* start, SyncerStep* end) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
@@ skipping 359 matching lines @@
   return session;
 }
 
 void SyncScheduler::PollTimerCallback() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   ModelSafeRoutingInfo r;
   ModelTypePayloadMap types_with_payloads =
       syncable::ModelTypePayloadMapFromRoutingInfo(r, std::string());
   SyncSourceInfo info(GetUpdatesCallerInfo::PERIODIC, types_with_payloads);
   SyncSession* s = CreateSyncSession(info);
-  ScheduleSyncSessionJob(TimeDelta::FromSeconds(0), SyncSessionJob::POLL, s,
-      FROM_HERE);
+
+  SyncSessionJob job(SyncSessionJob::POLL, TimeTicks::Now(),
+                     make_linked_ptr(s),
+                     false,
+                     FROM_HERE);
+
+  ScheduleSyncSessionJob(job);

[tim (not reviewing), 2011/12/09 23:07:22]

I created this function (ScheduleSyncSessionJob) taking parameters because: if
every callsite has to make this call anyway (there is actually logic happening
in ScheduleSSJ), then out of practicality (and convenience) it should be the
thing that constructs the job. It's less duplicated code everywhere, and it
should be unnatural for callsites to schedule jobs with |is_canary| set to true
because those need to be treated very carefully. Is there a functional reason
for this change?

[lipalani1, 2011/12/15 01:35:16]

The functional reason is in ScheduleNudgeImpl. We need to have the job there for
all the preprocessing we do before calling this function. The preprocessing
include ShouldRunJob and coalescing with the pending nudge. Once we have done
all that then we have to deconstruct to pass to this function. (one side note:
For us to deconstruct the delay we need to subtract from Now() which seems
inelegant as just few lines above that we would have added to Now() to calculate
the scheduled start).

Regarding canary_job we could have a DCHECK in ScheduleSyncSessionJob if needed.
I havent added that because that function by itself is not expected to care
about canary_job or not.

On 2011/12/09 23:07:22, timsteele wrote:

 }
 
 void SyncScheduler::Unthrottle() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK_EQ(WaitInterval::THROTTLED, wait_interval_->mode);
   SDVLOG(2) << "Unthrottled.";
   DoCanaryJob();
   wait_interval_.reset();
 }
 
@@ skipping 90 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // browser_sync