sync: Support nudges from non-blocking sync types

sync: Support nudges from non-blocking sync types

Implements support for receiving nudges from the non-blocking sync
engine.  When a non-blocking sync type requests a commit, it will also
send a request to the sync scheduler asking it to schedule
a sync cycle for some time in the future.

Adds some of the code required to support refresh requests, but does not
include an interface to allow clients of the non-blocking sync API to
access it.

Adds basic support for the initial download nudge.  When a non-blocking
type starts syncing for the first time, it sends a request to the
scheduler asking it to download any data available on the server.  This
allows it to complete initial sync quickly and without putting the
scheduler into configure mode.  For now, this looks like
a refresh request in the sync protocol.  This will be changed in
a future CL.

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

[rlarocque, 2014-07-09 20:29:17]

Please review.

[stanisc, 2014-07-09 22:35:25]

https://codereview.chromium.org/375023002/diff/20001/sync/engine/nudge_handle...
File sync/engine/nudge_handler.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/engine/nudge_handle...
sync/engine/nudge_handler.cc:9: NudgeHandler::NudgeHandler() {
Since NudgeHandler is effectively an interface, why aren't these trivial
constructor and destructor inlined in the header file?

https://codereview.chromium.org/375023002/diff/20001/sync/engine/nudge_handler.h
File sync/engine/nudge_handler.h (right):

https://codereview.chromium.org/375023002/diff/20001/sync/engine/nudge_handle...
sync/engine/nudge_handler.h:14: class SYNC_EXPORT_PRIVATE NudgeHandler {
Add a description

https://codereview.chromium.org/375023002/diff/20001/sync/engine/nudge_handle...
sync/engine/nudge_handler.h:19: virtual void
NudgeForInitialDownload(syncer::ModelType type) = 0;
Nit: since this class is already in syncer namespace, is it necessary to prefix
ModelType with the same namespace?

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_c...
File sync/internal_api/sync_context_proxy_impl_unittest.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_c...
sync/internal_api/sync_context_proxy_impl_unittest.cc:24: registry_(new
ModelTypeRegistry(workers_, NULL, &nudge_handler_)),
Consider adding a comment explaining what this NULL is and why it is OK to pass
it.

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.h:199: // SyncNudgeHandler implementation.
NudgeHandler

https://codereview.chromium.org/375023002/diff/20001/sync/test/engine/mock_nu...
File sync/test/engine/mock_nudge_handler.h (right):

https://codereview.chromium.org/375023002/diff/20001/sync/test/engine/mock_nu...
sync/test/engine/mock_nudge_handler.h:26: int GetNumRefreshNudges() const;
I didn't see any usage of the refresh counter. Are you planning to use it in a
later change?

[pavely, 2014-07-09 23:13:18]

lgtm with 2 comments.

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.cc:907: RequestNudgeForDataTypes(FROM_HERE,
ModelTypeSet(type));
Could you check thread either here or in RequestNudgeForDataTypes?

https://codereview.chromium.org/375023002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:106: : directory_(NULL),
weak_ptr_factory_(this) {
Set nudge_handler_ to NULL.
This change assumes (never checks) that nudge_handler_ is valid pointer, but
this ctor breaks this assumption. Seems like you removed last reference to this
ctor, maybe you can remove ctor as well?

[rlarocque, 2014-07-10 00:12:30]

https://codereview.chromium.org/375023002/diff/20001/sync/engine/nudge_handle...
File sync/engine/nudge_handler.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/engine/nudge_handle...
sync/engine/nudge_handler.cc:9: NudgeHandler::NudgeHandler() {
On 2014/07/09 22:35:24, stanisc wrote:
> Since NudgeHandler is effectively an interface, why aren't these trivial
> constructor and destructor inlined in the header file? 

I've seen the windows trybots sometimes fail to link when SYNC_EXPORT'ed
functions are declared in the header file.  Moving them to the .cc file fixes
it, so I've started declaring even trivial functions int he .cc file as a
precaution.

Besides, I see no harm in moving these functions to the .cc file.

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_c...
File sync/internal_api/sync_context_proxy_impl_unittest.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_c...
sync/internal_api/sync_context_proxy_impl_unittest.cc:24: registry_(new
ModelTypeRegistry(workers_, NULL, &nudge_handler_)),
On 2014/07/09 22:35:25, stanisc wrote:
> Consider adding a comment explaining what this NULL is and why it is OK to
pass
> it.

It's probably better to just use a real directory.  Did that instead.

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.cc:907: RequestNudgeForDataTypes(FROM_HERE,
ModelTypeSet(type));
On 2014/07/09 23:13:18, pavely wrote:
> Could you check thread either here or in RequestNudgeForDataTypes?

Done.

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/375023002/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.h:199: // SyncNudgeHandler implementation.
On 2014/07/09 22:35:25, stanisc wrote:
> NudgeHandler

Done.

https://codereview.chromium.org/375023002/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/375023002/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.cc:106: : directory_(NULL),
weak_ptr_factory_(this) {
On 2014/07/09 23:13:18, pavely wrote:
> Set nudge_handler_ to NULL.
> This change assumes (never checks) that nudge_handler_ is valid pointer, but
> this ctor breaks this assumption. Seems like you removed last reference to
this
> ctor, maybe you can remove ctor as well?

Removed the ctor.

https://codereview.chromium.org/375023002/diff/20001/sync/test/engine/mock_nu...
File sync/test/engine/mock_nudge_handler.h (right):

https://codereview.chromium.org/375023002/diff/20001/sync/test/engine/mock_nu...
sync/test/engine/mock_nudge_handler.h:26: int GetNumRefreshNudges() const;
On 2014/07/09 22:35:25, stanisc wrote:
> I didn't see any usage of the refresh counter. Are you planning to use it in a
> later change?

That's the plan.

A refresh nudge is some type-specific event.  For example, we issue one for
SESSIONS when the user opens the new tab page on Android.

We don't have any interface to expose this functionality to the non-blocking
data types, so we can't test it yet.

[stanisc, 2014-07-10 00:16:29]

lgtm

[rlarocque, 2014-07-10 17:00:01]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:01:45]

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

[commit-bot: I haz the power, 2014-07-10 17:13:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/27075)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31509)

[commit-bot: I haz the power, 2014-07-10 17:15:15]

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

[commit-bot: I haz the power, 2014-07-10 17:15:17]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/27080)

[rlarocque, 2014-07-10 17:52:53]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:55:30]

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

[commit-bot: I haz the power, 2014-07-10 22:03:13]

Change committed as 282439