Remove canary member from SyncSessionJob

Remove canary member from SyncSessionJob

This commit removes the 'canary' flag from the SyncSessionJob.  This
makes the code a bit simpler, and makes it slightly more difficult to
create bugs that would schedule too many canary jobs.

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

[rlarocque, 2013-03-01 18:54:28]

The effort to shrink SyncSessionJob continues.

Please review.

[rlarocque, 2013-03-01 19:19:30]

+pavely

Pavel is working in this area, so he might have some comments to add as well. 
At the very least, he deserves a heads up for any potential merge conflicts.

[pavely, 2013-03-02 00:51:04]

lgtm

[tim (not reviewing), 2013-03-02 01:03:39]

One comment below.  Also, how does this make it *less* likely to schedule more
canary jobs?  It seems like the opposite to me, since I now see the dangling
canary carrot every time I call DoSyncSessionJob.

https://codereview.chromium.org/12317104/diff/14001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (left):

https://codereview.chromium.org/12317104/diff/14001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:1022: // was scheduled as canary, and send it
to an "unscheduled" state.
This comment is even more relevant than it was before, since at least before you
had to type the characters "GrantCanaryPrivilege", and the thinking was that
sounded scary enough to make you think twice (i.e canary is a privilege you have
to grant, versus just a bool setting). So we should keep the comment.  We should
also use an enum instead of a bool parameter to DoSyncSessionJob for the same
reason, and also because bool parameters are discouraged by the chromium style
guide.

[rlarocque, 2013-03-02 01:21:12]

I believe this makes it less easy to grant canary privileges by accident because
we no longer store the flag anywhere.

Prior to this change, there was nothing (aside from DCHECKs) to enforce that
saved jobs did not have the canary bit set.  The maintenance of that condition
depends on code that lives nowhere near here, like SyncSessionJob::Clone(),
SyncSessionJob's constructor, HandleContinuationError(), etc.

DoSyncSessionJob(job, true) is pretty much equivalent to DoCanaryJob(job), so I
don't see how it's more dangerous than what we already have.  Once I replace the
bool with an enum, they will require an equal amount of "implicit
acknowledgement of the consequences through typing".

https://codereview.chromium.org/12317104/diff/14001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (left):

https://codereview.chromium.org/12317104/diff/14001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:1022: // was scheduled as canary, and send it
to an "unscheduled" state.
On 2013/03/02 01:03:39, timsteele wrote:
> This comment is even more relevant than it was before, since at least before
you
> had to type the characters "GrantCanaryPrivilege", and the thinking was that
> sounded scary enough to make you think twice (i.e canary is a privilege you
have
> to grant, versus just a bool setting). So we should keep the comment.  We
should
> also use an enum instead of a bool parameter to DoSyncSessionJob for the same
> reason, and also because bool parameters are discouraged by the chromium style
> guide.

OK, will fix.

[rlarocque, 2013-03-02 01:56:48]

Patch updated.  PTAL.

[tim (not reviewing), 2013-03-05 00:02:06]

On 2013/03/02 01:56:48, rlarocque wrote:
> Patch updated.  PTAL.

Thanks, changes look good.

[commit-bot: I haz the power, 2013-03-05 00:06:37]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tim (not reviewing), 2013-03-05 00:14:09]

lgtm

[commit-bot: I haz the power, 2013-03-05 00:20:02]

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

[commit-bot: I haz the power, 2013-03-05 00:20:04]

Failed to apply patch for sync/engine/sync_scheduler_whitebox_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file sync/engine/sync_scheduler_whitebox_unittest.cc
  Hunk #1 succeeded at 93 (offset 1 line).
  Hunk #2 FAILED at 107.
  Hunk #3 FAILED at 159.
  Hunk #4 succeeded at 258 with fuzz 2.
  2 out of 4 hunks FAILED -- saving rejects to file
sync/engine/sync_scheduler_whitebox_unittest.cc.rej

Patch:       sync/engine/sync_scheduler_whitebox_unittest.cc
Index: sync/engine/sync_scheduler_whitebox_unittest.cc
diff --git a/sync/engine/sync_scheduler_whitebox_unittest.cc
b/sync/engine/sync_scheduler_whitebox_unittest.cc
index
5a9d77408116ddb1d26ccd3e9dee5d3c3e931ec1..e1179e50f4f6274f8760ee452b287798ebeef543
100644
--- a/sync/engine/sync_scheduler_whitebox_unittest.cc
+++ b/sync/engine/sync_scheduler_whitebox_unittest.cc
@@ -92,8 +92,9 @@ class SyncSchedulerWhiteboxTest : public testing::Test {
   }
 
   SyncSchedulerImpl::JobProcessDecision DecideOnJob(
-      const SyncSessionJob& job) {
-    return scheduler_->DecideOnJob(job);
+      const SyncSessionJob& job,
+      SyncSchedulerImpl::JobPriority priority) {
+    return scheduler_->DecideOnJob(job, priority);
   }
 
   void InitializeSyncerOnNormalMode() {
@@ -106,7 +107,7 @@ class SyncSchedulerWhiteboxTest : public testing::Test {
     scoped_ptr<SyncSession> s(scheduler_->CreateSyncSession(SyncSourceInfo()));
     SyncSessionJob job(purpose, TimeTicks::Now(), s.Pass(),
         ConfigurationParams(), FROM_HERE);
-    return DecideOnJob(job);
+    return DecideOnJob(job, SyncSchedulerImpl::NORMAL_PRIORITY);
   }
 
   SyncSessionContext* context() { return context_.get(); }
@@ -158,7 +159,8 @@ TEST_F(SyncSchedulerWhiteboxTest,
SaveNudgeWhileTypeThrottled) {
                      s.Pass(),
                      ConfigurationParams(),
                      FROM_HERE);
-  SyncSchedulerImpl::JobProcessDecision decision = DecideOnJob(job);
+  SyncSchedulerImpl::JobProcessDecision decision =
+      DecideOnJob(job, SyncSchedulerImpl::NORMAL_PRIORITY);
   // TODO(tim): This shouldn't drop. Bug 177659.
   EXPECT_EQ(decision, SyncSchedulerImpl::DROP);
 }
@@ -257,8 +259,8 @@ TEST_F(SyncSchedulerWhiteboxTest, ContinueCanaryJobConfig) {
                      TimeTicks::Now(), scoped_ptr<SyncSession>(),
                      ConfigurationParams(), FROM_HERE);
 
-  job.GrantCanaryPrivilege();
-  SyncSchedulerImpl::JobProcessDecision decision = DecideOnJob(job);
+  SyncSchedulerImpl::JobProcessDecision decision =
+      DecideOnJob(job, SyncSchedulerImpl::CANARY_PRIORITY);
 
   EXPECT_EQ(decision, SyncSchedulerImpl::CONTINUE);
 }

[commit-bot: I haz the power, 2013-03-05 18:09:21]

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

[commit-bot: I haz the power, 2013-03-05 21:33:33]

Change committed as 186263