[Sync] Add history delete directive type

sync/protocol/history_delete_directive_specifics.proto

+// Copyright (c) 2012 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.
+//
+// Sync protocol datatype extension for history delete directives.
+
+// Update proto_value_conversions{.h,.cc,_unittest.cc} if you change
+// any fields in this file.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+option retain_unknown_fields = true;
+
+package sync_pb;
+
+// Properties of history delete directive sync objects.
+message HistoryDeleteDirectiveSpecifics {
+  // The full URL to delete.

[Nicolas Zea, 2012/08/08 01:26:19]

I think something like "If present, only those history entries with the
specified url are deleted" might be clearer.

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

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

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

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

[akalin, 2012/10/10 01:22:55]

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

+  optional string url = 1;

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

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.

[akalin, 2012/10/10 01:22:55]

Done.  Define an EverythingDirective.

+  // The ID of the client from which to delete the URL above.

[Nicolas Zea, 2012/08/08 01:26:19]

mention that if empty, the server consumes this delete.

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

hmm, i see.

+  optional string client_id = 2;
+
+  // All timestamps below are based on the clock for the client with the ID
+  // above.

[Nicolas Zea, 2012/08/08 01:26:19]

or the server timestamp if message is going to server.

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

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]

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.

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

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?

[akalin, 2012/10/10 01:22:55]

I think using sane time handles this.

+
+  // The time at which this directive was created.
+  optional int64 creation_time = 3;

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

What is this for? And since (afaiu) HistoryDeleteDirectives are immutable, how
is this different from SyncEntity.mtime?

[Raz Mathias, 2012/10/02 19:26:23]

Please postfix _usec or _millis

[akalin, 2012/10/10 01:22:55]

Removed.

+  // The time on or after which entries with the URL above should be deleted.

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

The unit should be specified. I'm assuming this is all microseconds.

[Raz Mathias, 2012/10/02 19:26:23]

Yes, seconded.  Please name the field start_time_usec or start_time_millis (it
gets a little confusing without the units server-side :)

[akalin, 2012/10/10 01:22:55]

Done.

+  optional int64 start_time = 4;
+  // The time on or before which entries with the URL above should be deleted.
+  optional int64 end_time = 5;

[Raz Mathias, 2012/10/02 19:26:23]

Please postfix _usec or _millis

[akalin, 2012/10/10 01:22:55]

Done.

+}