[Sync] Add history delete directive type

[Sync] Add history delete directive type

Add new HISTORY_DELETE_DIRECTIVE model type.  Update everything that
depends on model type.

Rename SyncCryptographer test to Cryptographer (since it's already in sync_unit_tests).

Add EncryptableUserTypes() function and use that instead of UserTypes() for
encryption-related stuff.

BUG=141245
TBR=jam@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162323

[akalin, 2012-08-07 23:13:31]

+zea for review

[Nicolas Zea, 2012-08-08 01:26:18]

LGTM with some comment nits

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
File sync/protocol/history_delete_directive_specifics.proto (right):

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:19: // The full URL to
delete.
I think something like "If present, only those history entries with the
specified url are deleted" might be clearer.

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:21: // The ID of the
client from which to delete the URL above.
mention that if empty, the server consumes this delete.

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:25: // above.
or the server timestamp if message is going to server.

[akalin, 2012-08-08 22:54:20]

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
File sync/protocol/history_delete_directive_specifics.proto (right):

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:19: // The full URL to
delete.
On 2012/08/08 01:26:19, nzea wrote:
> I think something like "If present, only those history entries with the
> specified url are deleted" might be clearer.

Just to be clear, if it's not present (empty or invalid) it should be ignored,
right?

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:21: // The ID of the
client from which to delete the URL above.
On 2012/08/08 01:26:19, nzea wrote:
> mention that if empty, the server consumes this delete.

hmm, i see.

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:25: // above.
On 2012/08/08 01:26:19, nzea wrote:
> or the server timestamp if message is going to server.

Perhaps we should have a source_id, target_id, and source_start_time,
target_start_time, etc.?  Only some fields will be filled in for a particular
instance, but I think if we're messing with timestamps, we want to be as clear
as possible about which clock a timestamp is based on.  What do you think?

[Nicolas Zea, 2012-08-08 23:18:55]

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
File sync/protocol/history_delete_directive_specifics.proto (right):

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:19: // The full URL to
delete.
On 2012/08/08 22:54:20, akalin wrote:
> On 2012/08/08 01:26:19, nzea wrote:
> > I think something like "If present, only those history entries with the
> > specified url are deleted" might be clearer.
> 
> Just to be clear, if it's not present (empty or invalid) it should be ignored,
> right?

Right, it means we're deleting all url's across the specified period.

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:25: // above.
On 2012/08/08 22:54:20, akalin wrote:
> On 2012/08/08 01:26:19, nzea wrote:
> > or the server timestamp if message is going to server.
> 
> Perhaps we should have a source_id, target_id, and source_start_time,
> target_start_time, etc.?  Only some fields will be filled in for a particular
> instance, but I think if we're messing with timestamps, we want to be as clear
> as possible about which clock a timestamp is based on.  What do you think?

Well, it's always the destination's clock. Perhaps client_id should be
destination id in that case? The source doesn't really matter I believe.

[akalin, 2012-08-08 23:24:03]

On 2012/08/08 23:18:55, nzea wrote:
> > Just to be clear, if it's not present (empty or invalid) it should be
ignored,
> > right?
> 
> Right, it means we're deleting all url's across the specified period.

I meant the delete directive should be ignored, meaning nothing should happen. 
But it sounds like that's not the case.

Relying on an empty value to mean "delete all" seems dangerous.  What do you
think of just adding a boolean flag like "delete everything" instead?

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
> sync/protocol/history_delete_directive_specifics.proto:25: // above.
> On 2012/08/08 22:54:20, akalin wrote:
> > On 2012/08/08 01:26:19, nzea wrote:
> > > or the server timestamp if message is going to server.
> > 
> > Perhaps we should have a source_id, target_id, and source_start_time,
> > target_start_time, etc.?  Only some fields will be filled in for a
particular
> > instance, but I think if we're messing with timestamps, we want to be as
clear
> > as possible about which clock a timestamp is based on.  What do you think?
> 
> Well, it's always the destination's clock. Perhaps client_id should be
> destination id in that case? The source doesn't really matter I believe.

But it's not always the destination's clock, right?  If I'm creating one meant
for the server, I can't know the server's clock, but only mine.

[Nicolas Zea, 2012-08-08 23:45:27]

On 2012/08/08 23:24:03, akalin wrote:
> On 2012/08/08 23:18:55, nzea wrote:
> > > Just to be clear, if it's not present (empty or invalid) it should be
> ignored,
> > > right?
> > 
> > Right, it means we're deleting all url's across the specified period.
> 
> I meant the delete directive should be ignored, meaning nothing should happen.

> But it sounds like that's not the case.
> 
> Relying on an empty value to mean "delete all" seems dangerous.  What do you
> think of just adding a boolean flag like "delete everything" instead?
> 

Hmm, yeah, I suppose if in the future we want to add a new field which specifies
what to delete in a different way we shouldn't rely on the field being empty for
this. SG.

>
https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
> > sync/protocol/history_delete_directive_specifics.proto:25: // above.
> > On 2012/08/08 22:54:20, akalin wrote:
> > > On 2012/08/08 01:26:19, nzea wrote:
> > > > or the server timestamp if message is going to server.
> > > 
> > > Perhaps we should have a source_id, target_id, and source_start_time,
> > > target_start_time, etc.?  Only some fields will be filled in for a
> particular
> > > instance, but I think if we're messing with timestamps, we want to be as
> clear
> > > as possible about which clock a timestamp is based on.  What do you think?
> > 
> > Well, it's always the destination's clock. Perhaps client_id should be
> > destination id in that case? The source doesn't really matter I believe.
> 
> But it's not always the destination's clock, right?  If I'm creating one meant
> for the server, I can't know the server's clock, but only mine.

So, my original thought here was that the web history chrome UI would actually
download server clock values for navigations when it interacts with the web
history server. In that case, we would have the server timestamps for each
navigation, and would use those.

If for some reason we don't have server values for navigations it means two
things:
a) if we're deleting specific navigations, those navigations aren't on the
server so no need to create a directive, just delete locally
b) if we're deleting across a time period, we use the closest server timestamps
to those navigations, effectively narrowing down the time period to whatever we
know the server is aware of.

(b) might seem bad, but the goal here is to ensure that if we show you something
as being pressent in the web history, you'll be able to delete it. If another
client is adding items to the web history while this client is offline, and you
do the deletions without coming back online, your deletions aren't guaranteed to
capture those navigations created by the other client. You need to be online to
ensure you have the most up to date state.

[akalin, 2012-08-08 23:48:06]

On 2012/08/08 23:45:27, nzea wrote:
> So, my original thought here was that the web history chrome UI would actually
> download server clock values for navigations when it interacts with the web
> history server. In that case, we would have the server timestamps for each
> navigation, and would use those.
> 
> If for some reason we don't have server values for navigations it means two
> things:
> a) if we're deleting specific navigations, those navigations aren't on the
> server so no need to create a directive, just delete locally
> b) if we're deleting across a time period, we use the closest server
timestamps
> to those navigations, effectively narrowing down the time period to whatever
we
> know the server is aware of.
> 
> (b) might seem bad, but the goal here is to ensure that if we show you
something
> as being pressent in the web history, you'll be able to delete it. If another
> client is adding items to the web history while this client is offline, and
you
> do the deletions without coming back online, your deletions aren't guaranteed
to
> capture those navigations created by the other client. You need to be online
to
> ensure you have the most up to date state.

[akalin, 2012-08-08 23:48:39]

On 2012/08/08 23:48:06, akalin wrote:
> On 2012/08/08 23:45:27, nzea wrote:
> > So, my original thought here was that the web history chrome UI would
actually
> > download server clock values for navigations when it interacts with the web
> > history server. In that case, we would have the server timestamps for each
> > navigation, and would use those.
> > 
> > If for some reason we don't have server values for navigations it means two
> > things:
> > a) if we're deleting specific navigations, those navigations aren't on the
> > server so no need to create a directive, just delete locally
> > b) if we're deleting across a time period, we use the closest server
> timestamps
> > to those navigations, effectively narrowing down the time period to whatever
> we
> > know the server is aware of.
> > 
> > (b) might seem bad, but the goal here is to ensure that if we show you
> something
> > as being pressent in the web history, you'll be able to delete it. If
another
> > client is adding items to the web history while this client is offline, and
> you
> > do the deletions without coming back online, your deletions aren't
guaranteed
> to
> > capture those navigations created by the other client. You need to be online
> to
> > ensure you have the most up to date state.

Hmm, really?  I didn't know we'd have access to server timestamps.  I thought
raz's server side work had to do with translating timestamps on the server? 
I'll add him.

[akalin, 2012-08-08 23:48:51]

+raz for question

[Raz Mathias, 2012-08-09 00:50:45]

On 2012/08/08 23:48:51, akalin wrote:
> +raz for question

Server-side we'll have both the client timestamp (from session.proto) and the
server-committed time (Ktimestamp) for the stream of records.  As such, we can
begin by anchoring times for range deletes to some coordinate system (say the
server's).  Once we have a start/end record to anchor us in the stream, we can
scan across the range to find clients that committed within the range along
local and server times (giving us ability calculate skew), and can create
directives tailored to those clients.  This is all pending a WH review since
they'd be doing the scan.

[albertb, 2012-08-09 12:44:11]

I'm joining the discussion a bit late, but since I'll be consuming this proto, I
have a few comments.

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
File sync/protocol/history_delete_directive_specifics.proto (right):

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:20: optional string url =
1;
I find it slightly scary that if none of the fields are set, this directive
instructs all clients to delete all history. Maybe the url should be "*" in that
case, and at least one timestamp should be set for a directive to be consumed by
a client.

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:25: // above.
Why do the server-to-client clock translation in the server? It seems pretty
hard to figure out by what offset a client clock is off based on its past
commits. Also, this assumes that client clocks are off by a constant amount:
what if the clock moves too fast, too slow, in the wrong direction or has been
manually changed by the user since the last commit? (By the way how *does*
chrome history handle the user manually changing the clock?)

Since the client must already translate timestamps in order to interleave local
history with web history on the history page, wouldn't it make more sense for
this proto to only ever refer to server time? And let the client translate from
server time to local time, using the same code?

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:28: optional int64
creation_time = 3;
What is this for? And since (afaiu) HistoryDeleteDirectives are immutable, how
is this different from SyncEntity.mtime?

https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
sync/protocol/history_delete_directive_specifics.proto:29: // The time on or
after which entries with the URL above should be deleted.
The unit should be specified. I'm assuming this is all microseconds.

[akalin, 2012-08-09 17:10:27]

On 2012/08/09 12:44:11, albertb wrote:
https://chromiumcodereview.appspot.com/10855037/diff/1/sync/protocol/history_...
> sync/protocol/history_delete_directive_specifics.proto:25: // above.
> Why do the server-to-client clock translation in the server? It seems pretty
> hard to figure out by what offset a client clock is off based on its past
> commits. Also, this assumes that client clocks are off by a constant amount:
> what if the clock moves too fast, too slow, in the wrong direction or has been
> manually changed by the user since the last commit? (By the way how *does*
> chrome history handle the user manually changing the clock?)
> 
> Since the client must already translate timestamps in order to interleave
local
> history with web history on the history page, wouldn't it make more sense for
> this proto to only ever refer to server time? And let the client translate
from
> server time to local time, using the same code?

Yeah, if that's possible, that would simplify stuff.  What about the case where
client says "i want to delete everything today for foo.com, i.e. 12am to
11:59pm"?  How do we translate that to server time (given that the client skew
may be non-constant, etc.)?  Just fuzz a bit and not worry about it?

[Raz Mathias, 2012-10-02 19:26:22]

Doing a server-code review, the units on timestamps got confusing.  Can we
rename the time-related fields to postfix _usec or _millis?

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
File sync/protocol/history_delete_directive_specifics.proto (right):

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:28: optional int64
creation_time = 3;
Please postfix _usec or _millis

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:29: // The time on or
after which entries with the URL above should be deleted.
On 2012/08/09 12:44:11, albertb wrote:
> The unit should be specified. I'm assuming this is all microseconds.
Yes, seconded.  Please name the field start_time_usec or start_time_millis (it
gets a little confusing without the units server-side :)

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:32: optional int64
end_time = 5;
Please postfix _usec or _millis

[akalin, 2012-10-10 01:22:54]

Arise, CL, from the dead!

PTAL!

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
File sync/protocol/history_delete_directive_specifics.proto (right):

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:19: // The full URL to
delete.
On 2012/08/08 23:18:55, nzea wrote:
> On 2012/08/08 22:54:20, akalin wrote:
> > On 2012/08/08 01:26:19, nzea wrote:
> > > I think something like "If present, only those history entries with the
> > > specified url are deleted" might be clearer.
> > 
> > Just to be clear, if it's not present (empty or invalid) it should be
ignored,
> > right?
> 
> Right, it means we're deleting all url's across the specified period.

As discussed, we don't need the URL anymore.  Replaced this with
GlobalIdDirective. (See http://codereview.chromium.org/11098002/ for global_id)

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:20: optional string url =
1;
On 2012/08/09 12:44:11, albertb wrote:
> I find it slightly scary that if none of the fields are set, this directive
> instructs all clients to delete all history. Maybe the url should be "*" in
that
> case, and at least one timestamp should be set for a directive to be consumed
by
> a client.

Done.  Define an EverythingDirective.

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:25: // above.
On 2012/08/09 12:44:11, albertb wrote:
> Why do the server-to-client clock translation in the server? It seems pretty
> hard to figure out by what offset a client clock is off based on its past
> commits. Also, this assumes that client clocks are off by a constant amount:
> what if the clock moves too fast, too slow, in the wrong direction or has been
> manually changed by the user since the last commit? (By the way how *does*
> chrome history handle the user manually changing the clock?)
> 
> Since the client must already translate timestamps in order to interleave
local
> history with web history on the history page, wouldn't it make more sense for
> this proto to only ever refer to server time? And let the client translate
from
> server time to local time, using the same code?

I think using sane time handles this.

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:28: optional int64
creation_time = 3;
On 2012/08/09 12:44:11, albertb wrote:
> What is this for? And since (afaiu) HistoryDeleteDirectives are immutable, how
> is this different from SyncEntity.mtime?

Removed.

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:29: // The time on or
after which entries with the URL above should be deleted.
On 2012/10/02 19:26:23, Raz Mathias wrote:
> On 2012/08/09 12:44:11, albertb wrote:
> > The unit should be specified. I'm assuming this is all microseconds.
> Yes, seconded.  Please name the field start_time_usec or start_time_millis (it
> gets a little confusing without the units server-side :)

Done.

http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
sync/protocol/history_delete_directive_specifics.proto:32: optional int64
end_time = 5;
On 2012/10/02 19:26:23, Raz Mathias wrote:
> Please postfix _usec or _millis

Done.

[albertb, 2012-10-10 15:15:31]

Protobuf looks good. But note that although GlobalIdDirective are technically
possible, Web History doesn't support this type of deletion today.

On 2012/10/10 01:22:54, akalin wrote:
> Arise, CL, from the dead!
> 
> PTAL!
> 
>
http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
> File sync/protocol/history_delete_directive_specifics.proto (right):
> 
>
http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
> sync/protocol/history_delete_directive_specifics.proto:19: // The full URL to
> delete.
> On 2012/08/08 23:18:55, nzea wrote:
> > On 2012/08/08 22:54:20, akalin wrote:
> > > On 2012/08/08 01:26:19, nzea wrote:
> > > > I think something like "If present, only those history entries with the
> > > > specified url are deleted" might be clearer.
> > > 
> > > Just to be clear, if it's not present (empty or invalid) it should be
> ignored,
> > > right?
> > 
> > Right, it means we're deleting all url's across the specified period.
> 
> As discussed, we don't need the URL anymore.  Replaced this with
> GlobalIdDirective. (See http://codereview.chromium.org/11098002/ for
global_id)
> 
>
http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
> sync/protocol/history_delete_directive_specifics.proto:20: optional string url
=
> 1;
> On 2012/08/09 12:44:11, albertb wrote:
> > I find it slightly scary that if none of the fields are set, this directive
> > instructs all clients to delete all history. Maybe the url should be "*" in
> that
> > case, and at least one timestamp should be set for a directive to be
consumed
> by
> > a client.
> 
> Done.  Define an EverythingDirective.
> 
>
http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
> sync/protocol/history_delete_directive_specifics.proto:25: // above.
> On 2012/08/09 12:44:11, albertb wrote:
> > Why do the server-to-client clock translation in the server? It seems pretty
> > hard to figure out by what offset a client clock is off based on its past
> > commits. Also, this assumes that client clocks are off by a constant amount:
> > what if the clock moves too fast, too slow, in the wrong direction or has
been
> > manually changed by the user since the last commit? (By the way how *does*
> > chrome history handle the user manually changing the clock?)
> > 
> > Since the client must already translate timestamps in order to interleave
> local
> > history with web history on the history page, wouldn't it make more sense
for
> > this proto to only ever refer to server time? And let the client translate
> from
> > server time to local time, using the same code?
> 
> I think using sane time handles this.
> 
>
http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
> sync/protocol/history_delete_directive_specifics.proto:28: optional int64
> creation_time = 3;
> On 2012/08/09 12:44:11, albertb wrote:
> > What is this for? And since (afaiu) HistoryDeleteDirectives are immutable,
how
> > is this different from SyncEntity.mtime?
> 
> Removed.
> 
>
http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
> sync/protocol/history_delete_directive_specifics.proto:29: // The time on or
> after which entries with the URL above should be deleted.
> On 2012/10/02 19:26:23, Raz Mathias wrote:
> > On 2012/08/09 12:44:11, albertb wrote:
> > > The unit should be specified. I'm assuming this is all microseconds.
> > Yes, seconded.  Please name the field start_time_usec or start_time_millis
(it
> > gets a little confusing without the units server-side :)
> 
> Done.
> 
>
http://codereview.chromium.org/10855037/diff/1/sync/protocol/history_delete_d...
> sync/protocol/history_delete_directive_specifics.proto:32: optional int64
> end_time = 5;
> On 2012/10/02 19:26:23, Raz Mathias wrote:
> > Please postfix _usec or _millis
> 
> Done.

[akalin, 2012-10-10 23:04:08]

On 2012/10/10 15:15:31, albertb wrote:
> Protobuf looks good. But note that although GlobalIdDirective are technically
> possible, Web History doesn't support this type of deletion today.

Yeah, I guess we'd have to plumb that through.  The idea was to replace
URL-based deletes with global ID-based ones.

[akalin, 2012-10-10 23:06:06]

Nicolas mentioned offline that we probably want to not encrypt this type.  I'll
work on that.

[akalin, 2012-10-15 18:36:53]

On 2012/10/10 23:06:06, akalin wrote:
> Nicolas mentioned offline that we probably want to not encrypt this type. 
I'll
> work on that.

Talked with Nicolas, decided that delete directives should be treated as a
normal type, except it won't work if all data types are encrypted.

Nicolas, PTAL!

[Nicolas Zea, 2012-10-15 19:03:57]

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/history_dele...
File sync/protocol/history_delete_directive_specifics.proto (right):

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/history_dele...
sync/protocol/history_delete_directive_specifics.proto:48: optional bool
delete_everything = 3 [default = false];
does this need a timestamp? Otherwise a client might receive this well after the
event has passed, and delete too much right? Although in that case, we could
just reuse the above directive and have start_time_usec == 0 right?

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/nigori_speci...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/nigori_speci...
sync/protocol/nigori_specifics.proto:112: optional bool
encrypt_history_delete_directives = 34;
see comment in nigori util. I'm not sure it makes sense to have this.

http://codereview.chromium.org/10855037/diff/24001/sync/syncable/nigori_util.cc
File sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/10855037/diff/24001/sync/syncable/nigori_util....
sync/syncable/nigori_util.cc:278: nigori->set_encrypt_history_delete_directives(
Does it make sense to have these changes? Perhaps just a comment that delete
directives are incompatible with encryption?

[akalin, 2012-10-15 19:37:09]

On 2012/10/15 19:03:57, nzea wrote:
http://codereview.chromium.org/10855037/diff/24001/sync/syncable/nigori_util....
> sync/syncable/nigori_util.cc:278:
nigori->set_encrypt_history_delete_directives(
> Does it make sense to have these changes? Perhaps just a comment that delete
> directives are incompatible with encryption?

I thought we were going to treat them as normal data types?  I.e., we'd let them
be encrypted, but then it just wouldn't work.

[albertb, 2012-10-15 19:50:33]

On 2012/10/15 19:37:09, akalin wrote:
> On 2012/10/15 19:03:57, nzea wrote:
>
http://codereview.chromium.org/10855037/diff/24001/sync/syncable/nigori_util....
> > sync/syncable/nigori_util.cc:278:
> nigori->set_encrypt_history_delete_directives(
> > Does it make sense to have these changes? Perhaps just a comment that delete
> > directives are incompatible with encryption?
> 
> I thought we were going to treat them as normal data types?  I.e., we'd let
them
> be encrypted, but then it just wouldn't work.

If you encrypt delete directives, the server won't be able to parse and and act
on them.

You could simply disable delete directives when tab sync is encrypted since in
that case the server can't even read the tab sync data and thus can't delete
anything. On the other hand, that would make deleting old, non-encrypted tab
sync data impossible.

From the server point of view, I think the best solution is to never encrypt
delete directives.

[akalin, 2012-10-15 20:21:21]

Comments addressed, PTAL!

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/history_dele...
File sync/protocol/history_delete_directive_specifics.proto (right):

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/history_dele...
sync/protocol/history_delete_directive_specifics.proto:48: optional bool
delete_everything = 3 [default = false];
On 2012/10/15 19:03:57, nzea wrote:
> does this need a timestamp? Otherwise a client might receive this well after
the
> event has passed, and delete too much right? Although in that case, we could
> just reuse the above directive and have start_time_usec == 0 right?

We'd have to make it so that the client keeps track of the directives it's acted
upon, right.

But yeah, I guess we should make 'delete everything' be a time range, just to be
save.  Removed.

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/nigori_speci...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10855037/diff/24001/sync/protocol/nigori_speci...
sync/protocol/nigori_specifics.proto:112: optional bool
encrypt_history_delete_directives = 34;
On 2012/10/15 19:03:57, nzea wrote:
> see comment in nigori util. I'm not sure it makes sense to have this.

Removed, added comment.

[Nicolas Zea, 2012-10-15 20:26:14]

lgtm

[commit-bot: I haz the power, 2012-10-15 20:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10855037/28003

[commit-bot: I haz the power, 2012-10-15 20:27:13]

Presubmit check for 10855037-28003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.4s to calculate.

[akalin, 2012-10-17 05:28:24]

Added EncryptableUserTypes() function and use that instead of UserTypes() for
encryption stuff.

TBRing jam.

[commit-bot: I haz the power, 2012-10-17 06:38:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10855037/43001

[commit-bot: I haz the power, 2012-10-17 08:21:06]

Change committed as 162323