[USS] Show USS counters in Types tab

[USS] Show USS counters in Types tab

Update the commit/update counters for USS

BUG=328606
Committed: https://crrev.com/79b625d97774cda6ebf7fe914236bd985cbd002f
Cr-Commit-Position: refs/heads/master@{#427156}

[Gang Wu, 2016-10-14 21:47:59]

Description was changed from

==========
remove FOR_EACH_OBSERVER


Add comments


need to add comments


first checkin

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

to

==========
[USS] Show USS counters in Types tab

Update the commit/update counters for USS

BUG=328606
==========

[Gang Wu, 2016-10-14 21:47:59]

gangwu@chromium.org changed reviewers:
+ pavely@chromium.org

[Gang Wu, 2016-10-14 21:58:57]

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

[commit-bot: I haz the power, 2016-10-14 21:59:29]

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-10-14 22:00:09]

Patchset #2 (id:20001) has been deleted

[Gang Wu, 2016-10-14 22:00:15]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-14 22:03:20]

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

[commit-bot: I haz the power, 2016-10-14 22:03:21]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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-10-16 18:50:09]

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

[commit-bot: I haz the power, 2016-10-16 18:50:21]

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-10-16 18:52:29]

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

[commit-bot: I haz the power, 2016-10-16 18:52:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Gang Wu, 2016-10-17 17:21:54]

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

[commit-bot: I haz the power, 2016-10-17 17:22:19]

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-10-17 17:22:42]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-10-17 18:15:17]

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

[commit-bot: I haz the power, 2016-10-17 18:15:18]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-10-17 19:26:31]

PTAL

BTW: change from FOR_EACH_OBSERVER to for-loop because of 
https://cs.chromium.org/chromium/src/PRESUBMIT.py?q=range-based+for+loops&sq=...

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:138: 
same as
https://cs.chromium.org/chromium/src/components/sync/engine_impl/process_upda...

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/non_blocking_type_commit_contribution.cc
(right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/non_blocking_type_commit_contribution.cc:53: int
successes = 0;
same as DirectoryCommitContribution::ProcessCommitResponse to collect counters.

[pavely, 2016-10-19 18:18:59]

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_registry.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.cc:107: emitter =
static_cast<DirectoryTypeDebugInfoEmitter*>(it->second);
Could you check if it will suffice to declare emitter as
DataTypeDebugInfoEmitter*?
This way you can avoid awkward static cast and exposing tunnecessary details of
implementation to update handler and commit contributor.
The same applies to creating emitter for non blocking type in ConnectType.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_registry.h (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.h:40: class
DirectoryTypeDebugInfoEmitter;
This forward declaration is not needed anymore.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.h:101:
DataTypeDebugInfoEmitterMap* directory_type_debug_info_emitter_map();
I don't think this accessor is used anywhere. Could you check if we can remove
it?

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.h:133:
DataTypeDebugInfoEmitterMap data_type_debug_info_emitter_map_;
Could you update comment?

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:200:
counters->num_updates_applied += pending_updates_.size();
Should there be Emit... call for this counter change?

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.h (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.h:62:
NonBlockingTypeDebugInfoEmitter* debug_info_emitter,
Could you move emitter parameter to the end of parameter list similar how you
did in non_blocking_type_commit_contribution? And move corresponding field
towards the end of this class.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.h:62:
NonBlockingTypeDebugInfoEmitter* debug_info_emitter,
Does ModelTypeWorker call any methods that are specific to
NonBlockingTypeDebugInfoEmitter? If not then it is better to pass base class
pointer.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker_unittest.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:194: worker_.reset(new
ModelTypeWorker(
The purpose of the changes in this file is to just make existing tests work.
Meanwhile model_type_worker.cc has some logic changes, there are some counters
updated and some events fired under some conditions. It would be useful to add
tests here to ensure that the counters are updated correctly and events fired
under correct conditions.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/worker_entity_tracker.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/worker_entity_tracker.cc:149: if
(update.response_version <= highest_gu_response_version_)
It is easier to read if condition follows name/semantic of function:
if (response_version > hughest_gu_response_version)
  return true;

Or even:
return (response_version > hughest_gu_response_version);

[Gang Wu, 2016-10-20 21:34:23]

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

[commit-bot: I haz the power, 2016-10-20 21:34:45]

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-10-20 21:35:36]

updated.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_registry.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.cc:107: emitter =
static_cast<DirectoryTypeDebugInfoEmitter*>(it->second);
On 2016/10/19 18:18:59, pavely wrote:
> Could you check if it will suffice to declare emitter as
> DataTypeDebugInfoEmitter*?
> This way you can avoid awkward static cast and exposing tunnecessary details
of
> implementation to update handler and commit contributor.
> The same applies to creating emitter for non blocking type in ConnectType.

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_registry.h (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.h:40: class
DirectoryTypeDebugInfoEmitter;
On 2016/10/19 18:18:59, pavely wrote:
> This forward declaration is not needed anymore.

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.h:101:
DataTypeDebugInfoEmitterMap* directory_type_debug_info_emitter_map();
On 2016/10/19 18:18:59, pavely wrote:
> I don't think this accessor is used anywhere. Could you check if we can remove
> it?

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_registry.h:133:
DataTypeDebugInfoEmitterMap data_type_debug_info_emitter_map_;
On 2016/10/19 18:18:59, pavely wrote:
> Could you update comment?

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:200:
counters->num_updates_applied += pending_updates_.size();
On 2016/10/19 18:18:59, pavely wrote:
> Should there be Emit... call for this counter change?

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.h (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.h:62:
NonBlockingTypeDebugInfoEmitter* debug_info_emitter,
On 2016/10/19 18:18:59, pavely wrote:
> Could you move emitter parameter to the end of parameter list similar how you
> did in non_blocking_type_commit_contribution? And move corresponding field
> towards the end of this class.

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.h:62:
NonBlockingTypeDebugInfoEmitter* debug_info_emitter,
On 2016/10/19 18:18:59, pavely wrote:
> Does ModelTypeWorker call any methods that are specific to
> NonBlockingTypeDebugInfoEmitter? If not then it is better to pass base class
> pointer.

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker_unittest.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:194: worker_.reset(new
ModelTypeWorker(
On 2016/10/19 18:18:59, pavely wrote:
> The purpose of the changes in this file is to just make existing tests work.
> Meanwhile model_type_worker.cc has some logic changes, there are some counters
> updated and some events fired under some conditions. It would be useful to add
> tests here to ensure that the counters are updated correctly and events fired
> under correct conditions.

Done.

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/worker_entity_tracker.cc (right):

https://codereview.chromium.org/2420213002/diff/80001/components/sync/engine_...
components/sync/engine_impl/worker_entity_tracker.cc:149: if
(update.response_version <= highest_gu_response_version_)
On 2016/10/19 18:18:59, pavely wrote:
> It is easier to read if condition follows name/semantic of function:
> if (response_version > hughest_gu_response_version)
>   return true;
> 
> Or even:
> return (response_version > hughest_gu_response_version);

Done.

[commit-bot: I haz the power, 2016-10-20 21:37:09]

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

[commit-bot: I haz the power, 2016-10-20 21:37:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Gang Wu, 2016-10-21 00:29:35]

Patchset #4 (id:120001) has been deleted

[Gang Wu, 2016-10-21 00:29:53]

Patchset #4 (id:140001) has been deleted

[Gang Wu, 2016-10-21 00:30:31]

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

[commit-bot: I haz the power, 2016-10-21 00:31:07]

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-10-21 00:33:40]

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

[Gang Wu, 2016-10-21 00:33:42]

Patchset #4 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-10-21 00:34:23]

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-10-21 02:29:56]

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

[commit-bot: I haz the power, 2016-10-21 02:29:56]

Dry run: This issue passed the CQ dry run.

[pavely, 2016-10-24 04:53:22]

lgtm

[Gang Wu, 2016-10-24 18:08:05]

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

[commit-bot: I haz the power, 2016-10-24 18:08:40]

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-10-24 19:30:42]

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

[commit-bot: I haz the power, 2016-10-24 19:30:43]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-10-24 21:04:48]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2016-10-24 21:04:48]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2420213002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-24 21:05:32]

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-10-24 21:24:50]

Description was changed from

==========
[USS] Show USS counters in Types tab

Update the commit/update counters for USS

BUG=328606
==========

to

==========
[USS] Show USS counters in Types tab

Update the commit/update counters for USS

BUG=328606
==========

[commit-bot: I haz the power, 2016-10-24 21:24:52]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2016-10-24 21:26:32]

Description was changed from

==========
[USS] Show USS counters in Types tab

Update the commit/update counters for USS

BUG=328606
==========

to

==========
[USS] Show USS counters in Types tab

Update the commit/update counters for USS

BUG=328606

Committed: https://crrev.com/79b625d97774cda6ebf7fe914236bd985cbd002f
Cr-Commit-Position: refs/heads/master@{#427156}
==========

[commit-bot: I haz the power, 2016-10-24 21:26:33]

Patchset 5 (id:??) landed as
https://crrev.com/79b625d97774cda6ebf7fe914236bd985cbd002f
Cr-Commit-Position: refs/heads/master@{#427156}