Add an action for server to send down rollback command.

Add an action for server to send down rollback command.

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

[Nicolas Zea, 2014-04-15 23:08:31]

do you need to update proto_value_conversions as well?

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.proto
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.pro...
sync/protocol/sync_enums.proto:118: UNKNOWN_ACTION              = 6; // This is
the default.
it's a bit concerning that we're modifying the value. I suppose that's okay if
we don't persist it anywhere.

[maniscalco, 2014-04-15 23:16:26]

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.proto
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.pro...
sync/protocol/sync_enums.proto:118: UNKNOWN_ACTION              = 6; // This is
the default.
On 2014/04/15 23:08:31, Nicolas Zea wrote:
> it's a bit concerning that we're modifying the value. I suppose that's okay if
> we don't persist it anywhere.

I was wondering the same thing.  Is Action ever sent over the wire?

If old code (pre this CL) tries to decode a new Action field (post this CL),
won't it interpret the value as UPGRADE_CLIENT?  Check out this section, it says
the first value of the enum is the default:
https://developers.google.com/protocol-buffers/docs/proto#optional

[haitaol1, 2014-04-15 23:44:03]

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.proto
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.pro...
sync/protocol/sync_enums.proto:118: UNKNOWN_ACTION              = 6; // This is
the default.
I think it's by design that UNKNOWN_ACTION is put at the end so that new enums
won't break legacy client because they will be interpreted as unknown. See
ConvertClientActionPBToLocalClientAction()

On 2014/04/15 23:08:31, Nicolas Zea wrote:
> it's a bit concerning that we're modifying the value. I suppose that's okay if
> we don't persist it anywhere.

[maniscalco, 2014-04-16 00:09:19]

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.proto
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/238273008/diff/1/sync/protocol/sync_enums.pro...
sync/protocol/sync_enums.proto:118: UNKNOWN_ACTION              = 6; // This is
the default.
To make things "obviously safe" I suggest leaving UNKNOWN_ACTION as 5, moving
UNKOWN_ACTION to the top of the enum (based on the doc I referenced above), and
adding the new DISABLE_SYNC_AND_ROLLBACK as 6.  This way, it's clear that
UNKNOWN_ACTION won't be misinterpreted as a DISABLE_SYNC_AND_ROLLBACK and it's
clear the default for an enum Action is UNKNOWN_ACTION.

On 2014/04/15 23:44:03, haitaol1 wrote:
> I think it's by design that UNKNOWN_ACTION is put at the end so that new enums
> won't break legacy client because they will be interpreted as unknown. See
> ConvertClientActionPBToLocalClientAction()
> 
> On 2014/04/15 23:08:31, Nicolas Zea wrote:
> > it's a bit concerning that we're modifying the value. I suppose that's okay
if
> > we don't persist it anywhere.
>

[haitaol1, 2014-04-16 23:04:54]

Turns out that server hasn't switched to using action. So changed to add a new
ErrorType USER_ROLLBACK. PTAL.

[maniscalco, 2014-04-17 18:05:29]

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
sync/protocol/sync_protocol_error.h:46: USER_ROLLBACK,
Should you also update the switch statement in
GetClientToServerResponseErrorType from
chrome/browser/sync/test/integration/sync_test.cc ?

Seems like GetClientToServerResponseErrorType probably belongs in a more central
location (like syncer_proto_util.cc?) so it will be more likely to get updated
in the future.

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
sync/protocol/sync_protocol_error.h:73: DISABLE_SYNC_AND_ROLLBACK,
Similar to my other comment about updating the switch statement in sync_test.cc,
do you need to update ClientAction switch statements?  E.g. the one in
GetClientActionString from sync_protocol_error.cc ?

[haitaol1, 2014-04-17 18:51:39]

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
sync/protocol/sync_protocol_error.h:46: USER_ROLLBACK,
The new enum is missed because switch in GetClientToServerResponseErrorType()
has a default case. After removing that, compiler starts complaining missing
handling of enums. I removed the default case and handled all enums explcitly.

On 2014/04/17 18:05:29, maniscalco wrote:
> Should you also update the switch statement in
> GetClientToServerResponseErrorType from
> chrome/browser/sync/test/integration/sync_test.cc ?
> 
> Seems like GetClientToServerResponseErrorType probably belongs in a more
central
> location (like syncer_proto_util.cc?) so it will be more likely to get updated
> in the future.

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
sync/protocol/sync_protocol_error.h:73: DISABLE_SYNC_AND_ROLLBACK,
On 2014/04/17 18:05:29, maniscalco wrote:
> Similar to my other comment about updating the switch statement in
sync_test.cc,
> do you need to update ClientAction switch statements?  E.g. the one in
> GetClientActionString from sync_protocol_error.cc ?

Done.

[maniscalco, 2014-04-18 16:29:15]

LGTM (of course, you should still get one from zea@; mine won't let you commit)

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/238273008/diff/40001/sync/protocol/sync_proto...
sync/protocol/sync_protocol_error.h:46: USER_ROLLBACK,
Good idea.

On 2014/04/17 18:51:40, haitaol1 wrote:
> The new enum is missed because switch in GetClientToServerResponseErrorType()
> has a default case. After removing that, compiler starts complaining missing
> handling of enums. I removed the default case and handled all enums explcitly.
> 
> On 2014/04/17 18:05:29, maniscalco wrote:
> > Should you also update the switch statement in
> > GetClientToServerResponseErrorType from
> > chrome/browser/sync/test/integration/sync_test.cc ?
> > 
> > Seems like GetClientToServerResponseErrorType probably belongs in a more
> central
> > location (like syncer_proto_util.cc?) so it will be more likely to get
updated
> > in the future.
>

[Nicolas Zea, 2014-04-18 18:06:50]

https://codereview.chromium.org/238273008/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/238273008/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_test.cc:919: return
sync_pb::SyncEnums::USER_NOT_ACTIVATED;
shouldn't this be auth_expired?

https://codereview.chromium.org/238273008/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_test.cc:944: case
syncer::USER_ROLLBACK:
Shouldn't this case lead to DISABLE_SYNC_AND_ROLLBACK?

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_enums...
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_enums...
sync/protocol/sync_enums.proto:101: USER_ROLLBACK     = 11;   // User aksed to
stop syncing and roll back.
"User aksed" -> "Client told"

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_proto...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_proto...
sync/protocol/sync_protocol_error.h:45: // User asked to stop syncing this
device and roll back local data.
Here too

[haitaol1, 2014-04-18 20:01:30]

https://codereview.chromium.org/238273008/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/238273008/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_test.cc:919: return
sync_pb::SyncEnums::USER_NOT_ACTIVATED;
On 2014/04/18 18:06:50, Nicolas Zea wrote:
> shouldn't this be auth_expired?

This should be the reverse mapping of
ConvertSyncProtocolErrorTypePBToLocalType(), which maps USER_NOT_ACTIVATED,
AUTH_INVALID, ACCESS_DENIED to INVALID_CREDENTIAL. But I dont' see any of those
being set in server code. Looks like auth error is actually detected through
http response code. So set NOTREACHED() to prevent testing on false assumption.

https://codereview.chromium.org/238273008/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_test.cc:944: case
syncer::USER_ROLLBACK:
I didn't add corresponding action in sync proto because that's not how server
works today. Set NOTREACHED() to prevent testing.

On 2014/04/18 18:06:50, Nicolas Zea wrote:
> Shouldn't this case lead to DISABLE_SYNC_AND_ROLLBACK?

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_enums...
File sync/protocol/sync_enums.proto (right):

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_enums...
sync/protocol/sync_enums.proto:101: USER_ROLLBACK     = 11;   // User aksed to
stop syncing and roll back.
On 2014/04/18 18:06:50, Nicolas Zea wrote:
> "User aksed" -> "Client told"

Done.

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_proto...
File sync/protocol/sync_protocol_error.h (right):

https://codereview.chromium.org/238273008/diff/80001/sync/protocol/sync_proto...
sync/protocol/sync_protocol_error.h:45: // User asked to stop syncing this
device and roll back local data.
On 2014/04/18 18:06:50, Nicolas Zea wrote:
> Here too

Done.

[Nicolas Zea, 2014-04-18 20:55:04]

lgtm

[haitaol1, 2014-04-18 21:09:09]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-04-18 21:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitaol@chromium.org/238273008/100001

[commit-bot: I haz the power, 2014-04-18 22:31:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 22:31:50]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[haitaol1, 2014-04-18 22:43:07]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-04-18 22:43:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitaol@chromium.org/238273008/110001

[commit-bot: I haz the power, 2014-04-19 01:03:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-19 01:03:33]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[haitaol1, 2014-04-21 19:57:46]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-04-21 19:58:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitaol@chromium.org/238273008/140001

[commit-bot: I haz the power, 2014-04-22 01:04:14]

Change committed as 265134