[USS] Move GetAllNodes from backend to controller

[USS] Move GetAllNodes from backend to controller

This check in is a preparation for showing USS debug info in
sync-internals page. After this one, will Implement
NonBlockingDataTypeController::GetAllNodes(), so
sync-internals can show USS sync nodes.

BUG=641069
Committed: https://crrev.com/8fd6d2f37cb22680b7b060a5528c9d93de81cc7a
Cr-Commit-Position: refs/heads/master@{#415505}

[Gang Wu, 2016-08-25 18:28:40]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[Gang Wu, 2016-08-25 18:29:00]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-25 18:29:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 20:04:44]

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

[commit-bot: I haz the power, 2016-08-25 20:04:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Gang Wu, 2016-08-25 21:45:42]

Description was changed from

==========
git cl format


step 2


step 1

BUG=
==========

to

==========
[USS] Move GetAllNodes from backend to controller



BUG=641069
==========

[Gang Wu, 2016-08-25 22:29:45]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 22:30:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2016-08-25 22:30:47]

Patchset #1 (id:20001) has been deleted

[Gang Wu, 2016-08-25 22:36:27]

Description was changed from

==========
[USS] Move GetAllNodes from backend to controller



BUG=641069
==========

to

==========
[USS] Move GetAllNodes from backend to controller

This check in is a preparation for showing USS debug info in
sync-internals page. After this one, will Implement
NonBlockingDataTypeController::GetAllNodes(), so
sync-internals can show USS sync nodes.

BUG=641069
==========

[Gang Wu, 2016-08-25 22:36:28]

gangwu@chromium.org changed reviewers:
+ maxbogue@chromium.org, pavely@chromium.org, skym@chromium.org

[Gang Wu, 2016-08-25 22:43:37]

PTAL

[maxbogue, 2016-08-25 23:38:59]

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2204: void
OnReceivedNodesForTypes(
This should become OnReceivedNodesForType, since now we're only calling it with
one type at a time. Same signature change as in DTC:

void OnReceivedNodesForType(syncer::ModelType type,
std::unique_ptr<base::ListValue> node_list);

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2255:
type_dict->Set("nodes", node_list);
Does this take ownership of the list pointer? Can you check and comment saying
so if it does? Otherwise we're just leaking it...

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2273: ModelTypeSet
all_types = GetRegisteredDataTypes();
Should this be using GetEnabledDataTypes()? It was filtered down in the backend
before, wasn't it? Though if it was, I'm not sure how the
GetAllNodesRequestHelper was finishing...

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:810:
scoped_refptr<base::SequencedTaskRunner> task_runner,
You don't need to pass a task runner here.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/data_type_controller.h:79: typedef
base::Callback<void(const std::vector<syncer::ModelType>& type,
It would make more sense if the callback was just for the one type:

base::Callback<syncer::ModelType, std::unique_ptr<base::ListValue>>

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/data_type_controller.h:160: virtual void
GetAllNodes(scoped_refptr<base::SequencedTaskRunner> task_runner,
You don't need the task runner here, either. This is always going to be done on
the UI thread and if the DTC is going to thread hop away, it can always grab the
UI thread handle itself.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.cc:74: return nodes;
nit: you could probably just do "return base::WrapUnique(...);" instead of
having an intermediary nodes variable.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/proxy_data_type_controller.cc:76: void
ProxyDataTypeController::GetAllNodes(
I suspect this shouldn't need an implementation, should it? I'm a bit confused.

[commit-bot: I haz the power, 2016-08-26 00:47:56]

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

[commit-bot: I haz the power, 2016-08-26 00:47:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Gang Wu, 2016-08-26 06:57:46]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 06:58:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 08:21:45]

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

[commit-bot: I haz the power, 2016-08-26 08:21:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Gang Wu, 2016-08-26 16:48:05]

updated, working on android test failures.

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2204: void
OnReceivedNodesForTypes(
On 2016/08/25 23:38:59, maxbogue wrote:
> This should become OnReceivedNodesForType, since now we're only calling it
with
> one type at a time. Same signature change as in DTC:
> 
> void OnReceivedNodesForType(syncer::ModelType type,
> std::unique_ptr<base::ListValue> node_list);

Done.

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2255:
type_dict->Set("nodes", node_list);
On 2016/08/25 23:38:59, maxbogue wrote:
> Does this take ownership of the list pointer? Can you check and comment saying
> so if it does? Otherwise we're just leaking it...

Done.

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2273: ModelTypeSet
all_types = GetRegisteredDataTypes();
On 2016/08/25 23:38:59, maxbogue wrote:
> Should this be using GetEnabledDataTypes()? It was filtered down in the
backend
> before, wasn't it? Though if it was, I'm not sure how the
> GetAllNodesRequestHelper was finishing...

I did not find GetEnabledDataTypes(), so I let DTC to filter disabled data type
by return empty list for disabled data type.
backend did the same thing, it use registrar to find which data type is
disabled, and sent empty list for it.
GetAllNodesRequestHelper will ignore empty list to display.

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2276943006/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:810:
scoped_refptr<base::SequencedTaskRunner> task_runner,
On 2016/08/25 23:38:59, maxbogue wrote:
> You don't need to pass a task runner here.

Done.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/data_type_controller.h:79: typedef
base::Callback<void(const std::vector<syncer::ModelType>& type,
On 2016/08/25 23:38:59, maxbogue wrote:
> It would make more sense if the callback was just for the one type:
> 
> base::Callback<syncer::ModelType, std::unique_ptr<base::ListValue>>

Done.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/data_type_controller.h:160: virtual void
GetAllNodes(scoped_refptr<base::SequencedTaskRunner> task_runner,
On 2016/08/25 23:38:59, maxbogue wrote:
> You don't need the task runner here, either. This is always going to be done
on
> the UI thread and if the DTC is going to thread hop away, it can always grab
the
> UI thread handle itself.

Done.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.cc:74: return nodes;
On 2016/08/25 23:38:59, maxbogue wrote:
> nit: you could probably just do "return base::WrapUnique(...);" instead of
> having an intermediary nodes variable.

Done.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/proxy_data_type_controller.cc:76: void
ProxyDataTypeController::GetAllNodes(
On 2016/08/25 23:38:59, maxbogue wrote:
> I suspect this shouldn't need an implementation, should it? I'm a bit
confused.

proxy_tab is registered and enabled type, and cannot filtered out in PSS, so we
need to implement here.

[skym, 2016-08-26 18:41:39]

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/proxy_data_type_controller.cc:81:
types_vector.push_back(type());
I know Max suggested to switch to returning a single type at a time, but just
fyi

1) Use initializer lists to make 1 element vectors. Waaay more compact than
declaring the vector, adding a single element, and then passing it along. And
we're allowed to use them now, wooo!

2) Don't use ScopedVector, std::vector<T> supports movable types now

https://bugs.chromium.org/p/chromium/issues/detail?id=554289

https://cs.chromium.org/chromium/src/base/memory/scoped_vector.h?q=ScopedVect...

[pavely, 2016-08-26 23:17:09]

https://codereview.chromium.org/2276943006/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2276943006/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:342: void
GetAllNodesForTypes(
I think you only have/use GetAllNodesForTypes in implementation.

https://codereview.chromium.org/2276943006/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:954: base::Thread*
sync_thread_test_;
Cleanup.

https://codereview.chromium.org/2276943006/diff/60001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.cc (right):

https://codereview.chromium.org/2276943006/diff/60001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.cc:33: if (state() ==
DataTypeController::DISABLED) {
I think you meant (state() != DISABLED).
state == DISABLED doesn't mean that datatype is disabled in preferences, it
means that model association failed.

[Gang Wu, 2016-08-27 04:40:03]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 04:40:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 06:38:31]

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

[commit-bot: I haz the power, 2016-08-27 06:38:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Gang Wu, 2016-08-29 17:15:31]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 17:27:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2016-08-29 18:41:15]

Patchset #4 (id:100001) has been deleted

[Gang Wu, 2016-08-29 18:41:36]

The CQ bit was unchecked by gangwu@chromium.org

[Gang Wu, 2016-08-29 20:07:41]

updated base on comment, fix android test failures, working on getting enabled
data type.

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
File components/sync/driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/2276943006/diff/40001/components/sync/driver/...
components/sync/driver/proxy_data_type_controller.cc:81:
types_vector.push_back(type());
On 2016/08/26 18:41:39, skym wrote:
> I know Max suggested to switch to returning a single type at a time, but just
> fyi
> 
> 1) Use initializer lists to make 1 element vectors. Waaay more compact than
> declaring the vector, adding a single element, and then passing it along. And
> we're allowed to use them now, wooo!
> 
> 2) Don't use ScopedVector, std::vector<T> supports movable types now
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=554289
> 
>
https://cs.chromium.org/chromium/src/base/memory/scoped_vector.h?q=ScopedVect...

Done.

https://codereview.chromium.org/2276943006/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2276943006/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:342: void
GetAllNodesForTypes(
On 2016/08/26 23:17:09, pavely wrote:
> I think you only have/use GetAllNodesForTypes in implementation.

Done.

https://codereview.chromium.org/2276943006/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:954: base::Thread*
sync_thread_test_;
I dont know where this line come from, will delete in next iteration

https://codereview.chromium.org/2276943006/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:954: base::Thread*
sync_thread_test_;
On 2016/08/26 23:17:09, pavely wrote:
> Cleanup.

Done.

[maxbogue, 2016-08-29 21:21:17]

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2267:
base::WrapUnique(new base::ListValue()));
nit: in this case you could just use base::MakeUnique<base::ListValue>().

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2270:
GetAllNodesForTypes(
I'm all for modularity, but GetAllNodesForTypes doesn't have any use outside
this function. I'd prefer you just make the !backend_initialized_ case return at
the end, and then insert the GetAllNodesForTypes code here.

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2533:
DataTypeController::TypeMap::const_iterator iter =
This is really the sort of place it's great to use auto. Or const auto or
whichever works here. Also I'd call it "dtc_iter" to distinguish from "it".

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
components/sync/driver/data_type_controller.h:78: typedef
base::Callback<void(const syncer::ModelType,
nit: newline above? Or remove the one below StartCallback, I don't really care
as long as it's consistent.

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
components/sync/driver/data_type_controller.h:159: virtual void
GetAllNodes(const AllNodesCallback& callback) = 0;
Comment please (especially that it's for debug purposes).

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.h:43: // Returns a
ListValue representing all nodes for a specified type by query
"by querying the directory."

[Gang Wu, 2016-08-30 18:37:50]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 18:38:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 18:42:34]

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

[commit-bot: I haz the power, 2016-08-30 18:42:36]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Gang Wu, 2016-08-30 20:14:59]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 20:15:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gang Wu, 2016-08-30 20:56:01]

Fix the enabled data type problem.

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2267:
base::WrapUnique(new base::ListValue()));
On 2016/08/29 21:21:16, maxbogue wrote:
> nit: in this case you could just use base::MakeUnique<base::ListValue>().

Done.

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2270:
GetAllNodesForTypes(
On 2016/08/29 21:21:16, maxbogue wrote:
> I'm all for modularity, but GetAllNodesForTypes doesn't have any use outside
> this function. I'd prefer you just make the !backend_initialized_ case return
at
> the end, and then insert the GetAllNodesForTypes code here.

Done.

https://codereview.chromium.org/2276943006/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:2533:
DataTypeController::TypeMap::const_iterator iter =
On 2016/08/29 21:21:16, maxbogue wrote:
> This is really the sort of place it's great to use auto. Or const auto or
> whichever works here. Also I'd call it "dtc_iter" to distinguish from "it".

Done.

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
components/sync/driver/data_type_controller.h:78: typedef
base::Callback<void(const syncer::ModelType,
On 2016/08/29 21:21:17, maxbogue wrote:
> nit: newline above? Or remove the one below StartCallback, I don't really care
> as long as it's consistent.

Done.

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
components/sync/driver/data_type_controller.h:159: virtual void
GetAllNodes(const AllNodesCallback& callback) = 0;
On 2016/08/29 21:21:17, maxbogue wrote:
> Comment please (especially that it's for debug purposes).

Done.

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/80001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.h:43: // Returns a
ListValue representing all nodes for a specified type by query
On 2016/08/29 21:21:17, maxbogue wrote:
> "by querying the directory."

Done.

[commit-bot: I haz the power, 2016-08-30 21:22:17]

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

[commit-bot: I haz the power, 2016-08-30 21:22:18]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-08-30 21:47:44]

lgtm w/ a few more nits.

https://codereview.chromium.org/2276943006/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2276943006/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2239: // Add these
results to our list.
Perhaps DCHECK at the top of this method that |type| is in |awaiting_types_|?

https://codereview.chromium.org/2276943006/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2270: } else {
I would still prefer putting a return statement at the end of the if and
removing the else. That style is commonly used for handling error cases and
helps emphasize that the below section is the "normal" code path that will be
followed (mostly by it not being indented).

https://codereview.chromium.org/2276943006/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2271: for
(syncer::ModelTypeSet::Iterator it = all_types.First(); it.Good();
Remove "syncer::"; we do using syncer::ModelTypeSet at the top of the file.

https://codereview.chromium.org/2276943006/diff/140001/components/sync/driver...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/140001/components/sync/driver...
components/sync/driver/data_type_controller.h:161: // |callback| on this thread.
Add "Used for populating chrome://sync-internals." or "Used for debugging
purposes." or something like that at the end of the comment.

[Gang Wu, 2016-08-30 21:59:44]

The CQ bit was checked by gangwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 22:00:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 23:11:17]

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

[commit-bot: I haz the power, 2016-08-30 23:11:18]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-08-30 23:46:06]

Thanks, Max!

https://codereview.chromium.org/2276943006/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2276943006/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2270: } else {
On 2016/08/30 21:47:44, maxbogue wrote:
> I would still prefer putting a return statement at the end of the if and
> removing the else. That style is commonly used for handling error cases and
> helps emphasize that the below section is the "normal" code path that will be
> followed (mostly by it not being indented).

Done.

https://codereview.chromium.org/2276943006/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2271: for
(syncer::ModelTypeSet::Iterator it = all_types.First(); it.Good();
On 2016/08/30 21:47:43, maxbogue wrote:
> Remove "syncer::"; we do using syncer::ModelTypeSet at the top of the file.

Done.

https://codereview.chromium.org/2276943006/diff/140001/components/sync/driver...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2276943006/diff/140001/components/sync/driver...
components/sync/driver/data_type_controller.h:161: // |callback| on this thread.
On 2016/08/30 21:47:44, maxbogue wrote:
> Add "Used for populating chrome://sync-internals." or "Used for debugging
> purposes." or something like that at the end of the comment.

Done.

[pavely, 2016-08-30 23:56:13]

lgtm

[Gang Wu, 2016-08-31 00:00:22]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2016-08-31 00:00:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2276943006/#ps160001
(title: "update for Max")

[commit-bot: I haz the power, 2016-08-31 00:00:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 00:06:11]

Description was changed from

==========
[USS] Move GetAllNodes from backend to controller

This check in is a preparation for showing USS debug info in
sync-internals page. After this one, will Implement
NonBlockingDataTypeController::GetAllNodes(), so
sync-internals can show USS sync nodes.

BUG=641069
==========

to

==========
[USS] Move GetAllNodes from backend to controller

This check in is a preparation for showing USS debug info in
sync-internals page. After this one, will Implement
NonBlockingDataTypeController::GetAllNodes(), so
sync-internals can show USS sync nodes.

BUG=641069
==========

[commit-bot: I haz the power, 2016-08-31 00:06:13]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-08-31 00:08:38]

Description was changed from

==========
[USS] Move GetAllNodes from backend to controller

This check in is a preparation for showing USS debug info in
sync-internals page. After this one, will Implement
NonBlockingDataTypeController::GetAllNodes(), so
sync-internals can show USS sync nodes.

BUG=641069
==========

to

==========
[USS] Move GetAllNodes from backend to controller

This check in is a preparation for showing USS debug info in
sync-internals page. After this one, will Implement
NonBlockingDataTypeController::GetAllNodes(), so
sync-internals can show USS sync nodes.

BUG=641069

Committed: https://crrev.com/8fd6d2f37cb22680b7b060a5528c9d93de81cc7a
Cr-Commit-Position: refs/heads/master@{#415505}
==========

[commit-bot: I haz the power, 2016-08-31 00:08:39]

Patchset 6 (id:??) landed as
https://crrev.com/8fd6d2f37cb22680b7b060a5528c9d93de81cc7a
Cr-Commit-Position: refs/heads/master@{#415505}