sync: Implement per-type update processing

sync: Implement per-type update processing

Introduces a new class that represents a syncable::Directory's udpate
requesting and processing capabilities.  The intention is that this will
eventually be expanded to cover commits as well.  Eventually, this will
be used as the basis for an interface between sync and 

Some update logic has been moved into this SyncDirectoryUpdateHandler
class or download.cc.  The rest of it can be found in
process_update_util.cc, the successor to the old ProcessUpdatesCommand.
The StoreTimestampsCommand has been entirely removed.  The unit tests
associated with these SyncerCommands have been ported to
sync_directory_update_handler_unittest.cc.

Except for a few error scenarios that are now handled differently, the
observable behavior of the client should not be changed by this CL.

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

[rlarocque, 2013-10-24 20:47:27]

Please review.

This should look similar to the per-type commit CL I committed recently
(r228810).

https://codereview.chromium.org/38803003/diff/60001/sync/engine/download.h
File sync/engine/download.h (right):

https://codereview.chromium.org/38803003/diff/60001/sync/engine/download.h#ne...
sync/engine/download.h:58: ExecuteDownloadUpdates(ModelTypeSet request_types,
Some of the issues I ran into in this change made me question whether or not we
should allow ProxyTypes to be sent down to this layer.  Maybe we should filter
them out at the SyncScheduler, or even earlier.

Thoughts?

[Nicolas Zea, 2013-10-25 21:04:41]

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:151: DLOG(WARNING) << "Unknown field number " <<
field_number;
do NOTREACHED instead? (here and below)

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:161: std::make_pair(model_type,
&gu_response.new_progress_marker(i)));
It seems risky to take the pointer of a possibly temp variable with unknown
scope. The progress markers aren't themselves particularly large right? Why not
just pass by value? Or alternatively store the index within the gu_response

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:186: TypeProgressMarkerMap::iterator pm_it =
progress_by_type.begin();
naming nit: pm_it, up_it, and uh_it are all fairly ambiguous names, perhaps
progress_marker_iter, update_iter, and update_handler_iter?

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:328: return result;
interesting, we would attempt to process updates and store timestamps even in
cases where the result was not SYNCER_OK before? Did that accomplish anything?

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
File sync/engine/process_updates_util.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
sync/engine/process_updates_util.cc:99: << ModelTypeToString(type);
nit: align <<

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
sync/engine/process_updates_util.cc:103: it->second.push_back(&update);
Same comment applies here. I think you can get the same benefit by just storing
the entry index within the updates list and therefore not run the risk that you
keep a pointer to an object that has been de-alloc'd.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
File sync/engine/process_updates_util.h (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
sync/engine/process_updates_util.h:34: // TODO: Move these definitions somewhere
else?
TODO(rlarocque)

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
File sync/engine/sync_directory_update_handler.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.cc:29: const
sync_pb::DataTypeProgressMarker& progress_marker,
fix indent

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.cc:45: const
sync_pb::DataTypeProgressMarker& progress_marker) {
fix indent

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
File sync/engine/sync_directory_update_handler.h (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:11: #include
"base/gtest_prod_util.h"
this isn't needed is it?

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:37: class SYNC_EXPORT_PRIVATE
SyncDirectoryUpdateHandler {
Out of curiosity, is this eventually going to implement some interface shared
with non-directory types?

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:43: void GetDownloadProgress(
Return rather than fill output parameter? Also, perhaps return by const ref?

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:50: // this update processor's type,
and the set of SyncEntities must include all
s/update processor/update handler/ ?

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:69: void UpdateProgressMarkers(
Given that this only updates a single progress marker, rename
UpdateProgressMarker?

https://codereview.chromium.org/38803003/diff/120001/sync/test/engine/mock_co...
File sync/test/engine/mock_connection_manager.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/test/engine/mock_co...
sync/test/engine/mock_connection_manager.cc:536:
sync_pb::DataTypeProgressMarker* new_marker =
what is this change for?

[rlarocque, 2013-10-25 22:29:46]

I've uploaded a new match that addresses most of your comments.  PTAL.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:151: DLOG(WARNING) << "Unknown field number " <<
field_number;
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> do NOTREACHED instead? (here and below)

If we ever get here, I expect the program will end up crashing on the
NOTREACHED() at line 181.  Allowing this loop to continue might result in
slightly more informative messages.  For example, I'd like to know if there was
more than one unknown field number present.

Normally I wouldn't resort to this level of cleverness just for nicer debug
messages, but the NOTREACEHD at 181 had to be there anyway, so we'd might as
well take advantage of it.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:161: std::make_pair(model_type,
&gu_response.new_progress_marker(i)));
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> It seems risky to take the pointer of a possibly temp variable with unknown
> scope. The progress markers aren't themselves particularly large right? Why
not
> just pass by value? Or alternatively store the index within the gu_response

The progress markers are moderately large.  I checked on my regular browser and
found that they range in size from 177 to 349 bytes.  There ~20 of them.  The
cost of copying isn't huge, but it would be nice to avoid if possible.

I wanted to avoid giving the update processing functions direct access to the
protobuf message.  The less functions that have access to it, the better. 
Changing that protobuf is hard enough as is.

I guess I could use indices.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:186: TypeProgressMarkerMap::iterator pm_it =
progress_by_type.begin();
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> naming nit: pm_it, up_it, and uh_it are all fairly ambiguous names, perhaps
> progress_marker_iter, update_iter, and update_handler_iter?

Done.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/download.cc#...
sync/engine/download.cc:328: return result;
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> interesting, we would attempt to process updates and store timestamps even in
> cases where the result was not SYNCER_OK before? Did that accomplish anything?

AFAIK, no.  This behavior was in the state machine (I think) because we needed
to make it all the way to the APPLY_UPDATES before we exited the loop.  I tried
to keep it intact when I refactored the syncer, even though I was pretty sure it
had no purpose.

We clear the StatusController's updates response two lines above, so there would
be no progress markers to save and no updates to process when those
SyncerCommands were invoked.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
File sync/engine/process_updates_util.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
sync/engine/process_updates_util.cc:99: << ModelTypeToString(type);
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> nit: align <<

Done.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
sync/engine/process_updates_util.cc:103: it->second.push_back(&update);
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> Same comment applies here. I think you can get the same benefit by just
storing
> the entry index within the updates list and therefore not run the risk that
you
> keep a pointer to an object that has been de-alloc'd.

The trade off here is not so clear.  We'd need to pass the callee a list of
indices and the entire vector.  Then the callee has the opportunity to make the
more subtle and dangerous error of processing items it's not supposed to.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
File sync/engine/process_updates_util.h (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/process_upda...
sync/engine/process_updates_util.h:34: // TODO: Move these definitions somewhere
else?
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> TODO(rlarocque)

Done.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
File sync/engine/sync_directory_update_handler.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.cc:29: const
sync_pb::DataTypeProgressMarker& progress_marker,
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> fix indent

Done.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.cc:45: const
sync_pb::DataTypeProgressMarker& progress_marker) {
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> fix indent

Done.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
File sync/engine/sync_directory_update_handler.h (right):

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:11: #include
"base/gtest_prod_util.h"
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> this isn't needed is it?

Nope.  Removed.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:37: class SYNC_EXPORT_PRIVATE
SyncDirectoryUpdateHandler {
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> Out of curiosity, is this eventually going to implement some interface shared
> with non-directory types?

Mostly yes.

The final interface might need to be a bit more complicated, to support features
like encryption.

The final interface will need to include functions from
SyncDirectoryUpdateHandler, SyncDirectoryCommitContributor, and the functions
for committing updates, so this class is only a piece of its implementation.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:43: void GetDownloadProgress(
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> Return rather than fill output parameter? Also, perhaps return by const ref?

Fixed the comment.

The syncable::Directory's function that implements this uses a pointer.  I don't
think there's any reasonable way to translate between the two styles in this
function.

I could return a const ref to a temporary, but that's kind of ugly.

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:50: // this update processor's type,
and the set of SyncEntities must include all
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> s/update processor/update handler/ ?

Done.  (Here and elsewhere.)

https://codereview.chromium.org/38803003/diff/120001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler.h:69: void UpdateProgressMarkers(
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> Given that this only updates a single progress marker, rename
> UpdateProgressMarker?

Done.

https://codereview.chromium.org/38803003/diff/120001/sync/test/engine/mock_co...
File sync/test/engine/mock_connection_manager.cc (right):

https://codereview.chromium.org/38803003/diff/120001/sync/test/engine/mock_co...
sync/test/engine/mock_connection_manager.cc:536:
sync_pb::DataTypeProgressMarker* new_marker =
On 2013/10/25 21:04:41, Nicolas Zea wrote:
> what is this change for?

This uses a little-known feature in the sync protocol.  Apparently, if the
progress markers for a given type are not present in the response, the client is
supposed to just accept that and leave its current progress markers as is.

This feature doesn't make a lot of sense these days, and our real server never
uses it, so I've decided to deprecate it.

This code implements a sort of optimization where the progress marker is not
sent in the response if it is equal to what came up in the request.  I presume
some old servers used to do this.

[rlarocque, 2013-10-28 23:12:11]

While working on the next patch in the series, I noticed an oversight in this
one.  This CL should use UpdateHandlers rather than the directory to fetch the
progress markers.  

That change could have been split out into a separate CL, but I think it should
be included in this one.  It wouldn't make a lot of sense to define the
UpdateHandlers' progress marker fetching functions but not use them.

The additional change is included in patch set #5.

[Nicolas Zea, 2013-10-29 22:50:17]

LGTM

https://codereview.chromium.org/38803003/diff/290001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/38803003/diff/290001/sync/engine/download.cc#...
sync/engine/download.cc:133: UpdateHandlerMap::iterator handler_it =
handler_map->find(it.Get());
DCHECK that handler_it != handler_map->end()?

https://codereview.chromium.org/38803003/diff/290001/sync/sessions/sync_sessi...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/38803003/diff/290001/sync/sessions/sync_sessi...
sync/sessions/sync_session_context.h:171: // Deleter for the
|commit_contributor_map_|.
update comment

[rlarocque, 2013-10-30 00:32:21]

https://codereview.chromium.org/38803003/diff/290001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/38803003/diff/290001/sync/engine/download.cc#...
sync/engine/download.cc:133: UpdateHandlerMap::iterator handler_it =
handler_map->find(it.Get());
On 2013/10/29 22:50:18, Nicolas Zea wrote:
> DCHECK that handler_it != handler_map->end()?

Done.

https://codereview.chromium.org/38803003/diff/290001/sync/sessions/sync_sessi...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/38803003/diff/290001/sync/sessions/sync_sessi...
sync/sessions/sync_session_context.h:171: // Deleter for the
|commit_contributor_map_|.
On 2013/10/29 22:50:18, Nicolas Zea wrote:
> update comment

Done.

[commit-bot: I haz the power, 2013-10-30 16:48:55]

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

[commit-bot: I haz the power, 2013-10-30 18:21:20]

Change committed as 231878