Sync: Refactoring of DEVICE_INFO syncable type - Part 1

Sync: Refactoring of DEVICE_INFO syncable type - Part 1

This change introduces a new class LocalDeviceInfoProvider that is responsible
for providing the local device specific DeviceInfo/cache_guid. It initializes
the data asynchronously and allows consumers to get notified when the data 
becomes available. 

LocalDeviceInfoProvider will allow to remove these responsibilities from 
SyncedDeviceTracker / ProfileSyncService and loose coupling between
ProfileSyncService and a number of SyncableService and DataTypeController 
derived classes.

LocalDeviceInfoProvider is hosted on the frontend thread. Since it needs
cache_guid to initialize DeviceInfo, which is currently available only on the
backend, I updated SyncBackendHostCore and SyncBackendHostImpl to pass
cache_guid to the frontend via OnBackendInitialized.

For the time being SyncedDeviceTracker remains unchanged
and continues to initialize the local device info too. The entire class will be
removed in the Part 2 of the change which will move SyncedDeviceTracker
functionality to a new SyncableService.

LocalDeviceInfoProvider also replaces SessionsSyncManager::SyncInternalApiDelegate
which was previously used to decouple SessionsSyncManager from the specifics of
of the local device info implementation. 
SessionsSyncManager is changed to consume LocalDeviceInfoProvider instead of 
SessionsSyncManager::SyncInternalApiDelegate.
SessionDataTypeController now also consumes LocalDeviceInfoProvider to make sure
that SESSIONS type model doesn't start before local device info becomes available.
A very similar approach will be used in the upcoming second part of the change
for DEVICE_INFO DataTypeController and SyncableType.

The change includes new unit tests for LocalDeviceInfoProvider and
SessionDataTypeController.

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

[rlarocque, 2014-07-08 18:25:38]

https://codereview.chromium.org/367153005/diff/1/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/367153005/diff/1/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_data_type_controller_unittest.cc:123:
EXPECT_FALSE(error.IsSet());
I dislike putting assertions in functions, because it makes the line number in
the assertion failure logs less useful.

A possible alternative is:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_a_Function_That...

You could also just put the assertions inline with the test code.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/sessions/sessions_apitest.cc (left):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_apitest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
(This comment is unrelated to this file but there's nowhere else to put it.)

Your CL description won't look the way you expect it to when you commit.

The "Subject" line is ignored by the commit-bot.

The "Description" box is what gets used in your commit message.

If you want your commit message to begin with a short title, you'll need to make
it the first line in the description.  Making it the subject will have no
effect.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/sessions/sessions_apitest.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_apitest.cc:127:
ON_CALL(*factory, CreateLocalDeviceInfoProvider()).WillByDefault(
We would normally discourage the use of GMock in new code, but the way you're
using it here is OK.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.cc:47: // the fields
passed in local_info
Comments should end with a period.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.h (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.h:29: // Specifies
whether local device info is available
Comments should end with a period.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.h:32: // Registers a
callback to be called when local device info becomes available
Add a period here, too.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_unittest.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_unittest.cc:67: 
Maybe add a test for the callback registration, too?

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:59: if
(!local_device_->IsInitialized()) {
I like the way you've handled this, but I can't think of a scenario where this
can happen in practice.

Sync should never try to initialize the DataTypeController unless the backend is
initialized.  Once the backend is initialized, we should have a cache_guid
available.

[maniscalco, 2014-07-08 21:17:53]

(some drive by comments; feel free to reject if they conflict with comments from
your selected reviewers)

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:15: class
LocalDeviceInfoProvider {
This would be a good place to describe the purpose of this class.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:21: virtual const
DeviceInfo* GetLocalDeviceInfo() const = 0;
The comment mentions the returned value is a scoped_ptr, but the return type is
a const DeviceInfo*.  Is that intentional?

Also, if you do plan to return a dumb pointer be sure to document the ownership
and lifetime semantics (caller is/is-not responsible for free'ing; pointer is
invalid at the next method call, etc.).  This applies to anywhere a dumb pointer
is passed or returned.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:24: virtual std::string
GetLocalSyncCacheGUID() const = 0;
Will this method return a non-empty string?  Even when IsInitialized() returns
false?

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:27: virtual bool
IsInitialized() const = 0;
Is this method necessary?  Given the description of GetLocalDeviceInfo, I would
expect callers to check the return value of GetLocalDeviceInfo instead.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:30: virtual
scoped_ptr<Subscription> RegisterOnInitializedCallback(
What's the return value for?  Does the lifetime of the returned Subscription
object affect whether callback is invoked?

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.cc (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.cc:36: void
LocalDeviceInfoProviderImpl::Initialize(const std::string& cache_guid) {
Can cache_guid be an empty string?  If not, consider adding
DCHECK(!cache_guid.empty());

It's probably worth documenting that cache_guids can (or cannot) be empty
strings.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.h:20: // Return value
is an empty scoped_ptr if the device info is unavailable.
Since these methods are documented in the base class (LocalDeviceInfoProvider),
it's common to omit the docs here.  That way the two won't get out of sync. 
It's also common to put a single comment above the block of methods indicating
that they are the implementation of, in this case, LocalDeviceInfoProvider. 
E.g.

// LocalDeviceInfoProvider implementation.
virtual const DeviceInfo* GetLocalDeviceInfo() const OVERRIDE;
virtual std::string GetLocalSyncCacheGUID() const OVERRIDE;
virtual bool IsInitialized() const OVERRIDE;
virtual scoped_ptr<Subscription> RegisterOnInitializedCallback(
    const base::Closure& callback) OVERRIDE;

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_mock.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_mock.h:13: class
LocalDeviceInfoProviderMock : public LocalDeviceInfoProvider {
Most of the sync code I've seen prefixes mocks with Mock (e.g.
MockLocalDeviceProvider).

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/sessions_sync_manager.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/sessions_sync_manager.h:87:
LocalDeviceInfoProvider* local_device_info_provider,
local_device_info_provider must outlive this SessionsSyncManager, right?  Can
you add a comment mentioning that.

[stanisc, 2014-07-09 21:58:37]

https://codereview.chromium.org/367153005/diff/1/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/367153005/diff/1/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_data_type_controller_unittest.cc:123:
EXPECT_FALSE(error.IsSet());
On 2014/07/08 18:25:37, rlarocque wrote:
> I dislike putting assertions in functions, because it makes the line number in
> the assertion failure logs less useful.
> 
> A possible alternative is:
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_a_Function_That...
> 
> You could also just put the assertions inline with the test code.
OK, I've rewritten these tests to avoid doing assertions in member functions.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:15: class
LocalDeviceInfoProvider {
On 2014/07/08 21:17:52, maniscalco wrote:
> This would be a good place to describe the purpose of this class.

Done.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:21: virtual const
DeviceInfo* GetLocalDeviceInfo() const = 0;
On 2014/07/08 21:17:52, maniscalco wrote:
> The comment mentions the returned value is a scoped_ptr, but the return type
is
> a const DeviceInfo*.  Is that intentional?
> 
> Also, if you do plan to return a dumb pointer be sure to document the
ownership
> and lifetime semantics (caller is/is-not responsible for free'ing; pointer is
> invalid at the next method call, etc.).  This applies to anywhere a dumb
pointer
> is passed or returned.
Good catch. This comment was copied from the previous implementation. I updated
the comment and added the ownership/lifetime information.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:24: virtual std::string
GetLocalSyncCacheGUID() const = 0;
On 2014/07/08 21:17:52, maniscalco wrote:
> Will this method return a non-empty string?  Even when IsInitialized() returns
> false?
Added comment.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:27: virtual bool
IsInitialized() const = 0;
On 2014/07/08 21:17:52, maniscalco wrote:
> Is this method necessary?  Given the description of GetLocalDeviceInfo, I
would
> expect callers to check the return value of GetLocalDeviceInfo instead.
I guess you are right. It is somewhat redundant and could be removed.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider.h:30: virtual
scoped_ptr<Subscription> RegisterOnInitializedCallback(
On 2014/07/08 21:17:52, maniscalco wrote:
> What's the return value for?  Does the lifetime of the returned Subscription
> object affect whether callback is invoked?
This follows the callback subscription pattern described in callback_list.h.
I've added a comment.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.cc (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.cc:36: void
LocalDeviceInfoProviderImpl::Initialize(const std::string& cache_guid) {
On 2014/07/08 21:17:53, maniscalco wrote:
> Can cache_guid be an empty string?  If not, consider adding
> DCHECK(!cache_guid.empty());
> 
> It's probably worth documenting that cache_guids can (or cannot) be empty
> strings.
> 

Done.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.h:20: // Return value
is an empty scoped_ptr if the device info is unavailable.
On 2014/07/08 21:17:53, maniscalco wrote:
> Since these methods are documented in the base class
(LocalDeviceInfoProvider),
> it's common to omit the docs here.  That way the two won't get out of sync. 
> It's also common to put a single comment above the block of methods indicating
> that they are the implementation of, in this case, LocalDeviceInfoProvider. 
> E.g.
> 
> // LocalDeviceInfoProvider implementation.
> virtual const DeviceInfo* GetLocalDeviceInfo() const OVERRIDE;
> virtual std::string GetLocalSyncCacheGUID() const OVERRIDE;
> virtual bool IsInitialized() const OVERRIDE;
> virtual scoped_ptr<Subscription> RegisterOnInitializedCallback(
>     const base::Closure& callback) OVERRIDE;

Done.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_mock.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_mock.h:13: class
LocalDeviceInfoProviderMock : public LocalDeviceInfoProvider {
On 2014/07/08 21:17:53, maniscalco wrote:
> Most of the sync code I've seen prefixes mocks with Mock (e.g.
> MockLocalDeviceProvider).
There are 26 instances (including this one) of files with _mock suffix. Many of
them are in Sync code including ProfileSyncServiceMock and
ProfileSyncComponentsFactoryMock. I agree that having Mock as a prefix would
make a better name, but having it as a suffix makes it easier to group related
files together and it kind of follows the pattern of adding other suffixes like
Impl or Test. I'd prefer to leave it this way unless you think we should start
renaming all of these mock classes.

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/sessions_sync_manager.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/sessions_sync_manager.h:87:
LocalDeviceInfoProvider* local_device_info_provider,
On 2014/07/08 21:17:53, maniscalco wrote:
> local_device_info_provider must outlive this SessionsSyncManager, right?  Can
> you add a comment mentioning that.

Done.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/sessions/sessions_apitest.cc (left):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_apitest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2014/07/08 18:25:37, rlarocque wrote:
> (This comment is unrelated to this file but there's nowhere else to put it.)
> 
> Your CL description won't look the way you expect it to when you commit.
> 
> The "Subject" line is ignored by the commit-bot.
> 
> The "Description" box is what gets used in your commit message.
> 
> If you want your commit message to begin with a short title, you'll need to
make
> it the first line in the description.  Making it the subject will have no
> effect.

Done.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/sessions/sessions_apitest.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_apitest.cc:127:
ON_CALL(*factory, CreateLocalDeviceInfoProvider()).WillByDefault(
On 2014/07/08 18:25:37, rlarocque wrote:
> We would normally discourage the use of GMock in new code, but the way you're
> using it here is OK.

Done.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.cc:47: // the fields
passed in local_info
On 2014/07/08 18:25:37, rlarocque wrote:
> Comments should end with a period.

Done.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_impl.h (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.h:29: // Specifies
whether local device info is available
On 2014/07/08 18:25:38, rlarocque wrote:
> Comments should end with a period.

Done.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_impl.h:32: // Registers a
callback to be called when local device info becomes available
On 2014/07/08 18:25:38, rlarocque wrote:
> Add a period here, too.

Done.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_unittest.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_unittest.cc:67: 
On 2014/07/08 18:25:38, rlarocque wrote:
> Maybe add a test for the callback registration, too?

I had a test for this in InitializeProvider method that would hang and cause the
test to time out if the callback wasn't called. I've changed the test to be more
obvious.

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/sess...
File chrome/browser/sync/sessions/session_data_type_controller.cc (right):

https://codereview.chromium.org/367153005/diff/40001/chrome/browser/sync/sess...
chrome/browser/sync/sessions/session_data_type_controller.cc:59: if
(!local_device_->IsInitialized()) {
On 2014/07/08 18:25:38, rlarocque wrote:
> I like the way you've handled this, but I can't think of a scenario where this
> can happen in practice.
> 
> Sync should never try to initialize the DataTypeController unless the backend
is
> initialized.  Once the backend is initialized, we should have a cache_guid
> available.

Local Device Info initialization is asynchronous. When backend is initialized we
get cache_guid and start local device info initialization but it isn't
immediately ready and becomes available a bit later.

[maniscalco, 2014-07-09 22:47:34]

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/local_device_info_provider_mock.h (right):

https://codereview.chromium.org/367153005/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/local_device_info_provider_mock.h:13: class
LocalDeviceInfoProviderMock : public LocalDeviceInfoProvider {
You're right, it's mixed.  All of the files in c/b/sync end with _mock whereas
(almost all of) those in src/sync start with mock_:

maniscalco-z620 ~/chromium/src(master) $ find chrome/browser/sync | grep mock
chrome/browser/sync/profile_sync_service_mock.cc
chrome/browser/sync/glue/sync_backend_host_mock.cc
chrome/browser/sync/glue/sync_backend_host_mock.h
chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h
chrome/browser/sync/glue/frontend_data_type_controller_mock.cc
chrome/browser/sync/glue/non_frontend_data_type_controller_mock.cc
chrome/browser/sync/glue/frontend_data_type_controller_mock.h
chrome/browser/sync/profile_sync_components_factory_mock.cc
chrome/browser/sync/profile_sync_service_mock.h
chrome/browser/sync/profile_sync_components_factory_mock.h

maniscalco-z620 ~/chromium/src(master) $ find sync | grep mock
sync/notifier/mock_ack_handler.h
sync/notifier/mock_ack_handler.cc
sync/test/mock_invalidation.h
sync/test/engine/mock_connection_manager.h
sync/test/engine/mock_model_type_sync_proxy.cc
sync/test/engine/mock_update_handler.cc
sync/test/engine/mock_model_type_sync_proxy.h
sync/test/engine/mock_update_handler.h
sync/test/engine/single_type_mock_server.cc
sync/test/engine/single_type_mock_server.h
sync/test/engine/mock_model_type_sync_worker.cc
sync/test/engine/mock_connection_manager.cc
sync/test/engine/mock_model_type_sync_worker.h
sync/test/sessions/mock_debug_info_getter.h
sync/test/sessions/mock_debug_info_getter.cc
sync/test/trackable_mock_invalidation.cc
sync/test/trackable_mock_invalidation.h
sync/test/mock_invalidation.cc
sync/test/mock_invalidation_tracker.cc
sync/test/mock_invalidation_tracker.h
sync/api/sync_error_factory_mock.h
sync/api/sync_error_factory_mock.cc

I agree on leaving as is.

On 2014/07/09 21:58:36, stanisc wrote:
> On 2014/07/08 21:17:53, maniscalco wrote:
> > Most of the sync code I've seen prefixes mocks with Mock (e.g.
> > MockLocalDeviceProvider).
> There are 26 instances (including this one) of files with _mock suffix. Many
of
> them are in Sync code including ProfileSyncServiceMock and
> ProfileSyncComponentsFactoryMock. I agree that having Mock as a prefix would
> make a better name, but having it as a suffix makes it easier to group related
> files together and it kind of follows the pattern of adding other suffixes
like
> Impl or Test. I'd prefer to leave it this way unless you think we should start
> renaming all of these mock classes.
>

[rlarocque, 2014-07-09 23:01:50]

lgtm

[stanisc, 2014-07-10 21:22:24]

I had a couple more small patches to address an issue with one  Mac specific .mm
file which I'd initially overlooked and issues with some ProfileSyncServiceTest
tests which had failures due to using a strict mock of
ProfileSyncComponentsFactory. With the latest patch all try bots are green with
an exception of telemetry_unittests that seem to be flaky and unrelated to my
changes.

[stanisc, 2014-07-10 21:38:07]

The CQ bit was checked by stanisc@chromium.org

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

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

[commit-bot: I haz the power, 2014-07-10 23:25:46]

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...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 23:29:42]

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

[commit-bot: I haz the power, 2014-07-10 23:29:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[stanisc, 2014-07-10 23:52:34]

Need additional OWNER reviewers for a few files.

Nick, please review:
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc
chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc 

Scott, please review:
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
chrome/browser/extensions/api/sessions/sessions_apitest.cc
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc
chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

[maniscalco, 2014-07-11 00:00:00]

c/b/ui/sync lgtm

[sky, 2014-07-14 19:33:34]

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
(right):

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc:46:
new ProfileSyncComponentsFactoryMock(),
Can the constructor take a scoped_ptr so that I don't have to wonder if you're
leaking ProfileSyncComponentsFactoryMock?

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm:64:
local_device_.reset(new browser_sync::LocalDeviceInfoProviderMock(
nit: make part of member initialize list.

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right):

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:117:
local_device_.reset(new browser_sync::LocalDeviceInfoProviderMock(
move to member initializer list.

[stanisc, 2014-07-14 22:49:05]

Addressed the latest CR feedback including
changing ProfileSyncService to take scoped_ptr<ProfileSyncComponentsFactory>
rather than ProfileSyncComponentsFactory*.

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
(right):

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc:46:
new ProfileSyncComponentsFactoryMock(),
On 2014/07/14 19:33:34, sky wrote:
> Can the constructor take a scoped_ptr so that I don't have to wonder if you're
> leaking ProfileSyncComponentsFactoryMock?
This is a pre-existing issue, but I agree it makes sense to convert it to
scoped_ptr. Done.

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm:64:
local_device_.reset(new browser_sync::LocalDeviceInfoProviderMock(
On 2014/07/14 19:33:34, sky wrote:
> nit: make part of member initialize list.

Done.

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right):

https://codereview.chromium.org/367153005/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:117:
local_device_.reset(new browser_sync::LocalDeviceInfoProviderMock(
On 2014/07/14 19:33:34, sky wrote:
> move to member initializer list.

Done.

[rlarocque, 2014-07-15 00:04:31]

+CC Pavel, since he's working on changes near this code.

[sky, 2014-07-15 04:32:43]

Set of files you asked me to review LGTM

[stanisc, 2014-07-16 17:56:44]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2014-07-16 17:58:59]

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

[commit-bot: I haz the power, 2014-07-16 18:52:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-07-16 19:03:56]

Change committed as 283461

[kuan, 2014-07-16 19:57:50]

A revert of this CL has been created in
https://codereview.chromium.org/401433003/ by kuan@chromium.org.

The reason for reverting is: this broke unit_tests" on "Linux ASan LSan Tests
(2):
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....