[GRC] Coordination Unit Manager Modularization

[GRC] Coordination Unit Manager Modularization

Encapsulate GRC functionalities related to coordination unit management
as this is only one aspect of GRC. There are also a few minor
refactorings included as well.

This CL depends on: https://codereview.chromium.org/2710823003

R=oysteine@chromium.org
BUG=724306
Review-Url: https://codereview.chromium.org/2914003002
Cr-Commit-Position: refs/heads/master@{#477184}
Committed: https://chromium.googlesource.com/chromium/src/+/85378cec1bed7f00a92ff1c658eed044e2518a79

[matthalp, 2017-05-31 15:54:23]

Description was changed from

==========
[GRC] Coordination Unit Manager Modularization

Encapusalte GRC functionalities related to coordination unit management
as this is only one aspect of GRC.

This CL depends on: https://codereview.chromium.org/2710823003

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] Coordination Unit Manager Modularization

Encapusalte GRC functionalities related to coordination unit management
as this is only one aspect of GRC. There are also a few minor
refactorings included as well.

This CL depends on: https://codereview.chromium.org/2710823003

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-05-31 15:54:31]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 15:55:06]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-05-31 15:56:49]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-05-31 15:58:02]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 15:58:25]

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, 2017-05-31 16:01:21]

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

[commit-bot: I haz the power, 2017-05-31 16:01:22]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[matthalp, 2017-05-31 16:12:50]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 16:13:13]

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, 2017-05-31 16:15:38]

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

[commit-bot: I haz the power, 2017-05-31 16:15:39]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[oystein (OOO til 10th of July), 2017-06-01 18:29:09]

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:39:
CoordinationUnitID& id() { return id_; }
const

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:40:
std::set<CoordinationUnitImpl*>& children() { return children_; }
Can these be const?

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_manager.h
(right):

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:19:
CoordinationUnitManager();
Since there's no class members yet, just ditch these for now (the main point of
defining default constructors/destructors in the .cc is when the class has
non-POD members).

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
File services/resource_coordinator/resource_coordinator_service.h (right):

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/resource_coordinator_service.h:5: #ifndef
SERVICES_RESOURCE_COORDINATOR_RESOURCE_COORDINATOR_SERVICE_H_
Nice catch, thanks :)

[matthalp, 2017-06-01 21:05:14]

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:39:
CoordinationUnitID& id() { return id_; }
On 2017/06/01 at 18:29:08, oystein wrote:
> const

Done.

Additionally, I made id_ itself const. However, this required changing the
constructor logic a little bit because const member variables can only be
initialized in constructor initializer lists (so the if-then logic had to be
removed), so I moved that logic into the CoordinationUnitID constructor itself.
Was that a reasonable change?

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:40:
std::set<CoordinationUnitImpl*>& children() { return children_; }
On 2017/06/01 at 18:29:08, oystein wrote:
> Can these be const?

Absolutely! Done.

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_manager.h
(right):

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/06/01 at 18:29:08, oystein wrote:
> 2017

Done.

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:19:
CoordinationUnitManager();
On 2017/06/01 at 18:29:08, oystein wrote:
> Since there's no class members yet, just ditch these for now (the main point
of defining default constructors/destructors in the .cc is when the class has
non-POD members).

Unchanged. Default constructor appears to be required because the copy and
assignment macro, but there was a chance I just went about it wrong (however
simple it may be). I have a future CL that will add non-POD members anyway.

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
File services/resource_coordinator/resource_coordinator_service.h (right):

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/resource_coordinator_service.h:5: #ifndef
SERVICES_RESOURCE_COORDINATOR_RESOURCE_COORDINATOR_SERVICE_H_
On 2017/06/01 at 18:29:08, oystein wrote:
> Nice catch, thanks :)

Np!

[oystein (OOO til 10th of July), 2017-06-01 21:57:04]

lgtm and thanks!

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:39:
CoordinationUnitID& id() { return id_; }
On 2017/06/01 at 21:05:14, matthalp wrote:
> On 2017/06/01 at 18:29:08, oystein wrote:
> > const
> 
> Done.
> 
> Additionally, I made id_ itself const. However, this required changing the
constructor logic a little bit because const member variables can only be
initialized in constructor initializer lists (so the if-then logic had to be
removed), so I moved that logic into the CoordinationUnitID constructor itself.
Was that a reasonable change?

Yep seems totally fine to me!

[oystein (OOO til 10th of July), 2017-06-01 21:57:31]

On 2017/06/01 at 21:57:04, oystein wrote:
> lgtm and thanks!
> 
>
https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
> File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):
> 
>
https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
> services/resource_coordinator/coordination_unit/coordination_unit_impl.h:39:
CoordinationUnitID& id() { return id_; }
> On 2017/06/01 at 21:05:14, matthalp wrote:
> > On 2017/06/01 at 18:29:08, oystein wrote:
> > > const
> > 
> > Done.
> > 
> > Additionally, I made id_ itself const. However, this required changing the
constructor logic a little bit because const member variables can only be
initialized in constructor initializer lists (so the if-then logic had to be
removed), so I moved that logic into the CoordinationUnitID constructor itself.
Was that a reasonable change?
> 
> Yep seems totally fine to me!

Oh, description s/Encapusalte/Encapsulate/

[matthalp, 2017-06-05 22:37:44]

Description was changed from

==========
[GRC] Coordination Unit Manager Modularization

Encapusalte GRC functionalities related to coordination unit management
as this is only one aspect of GRC. There are also a few minor
refactorings included as well.

This CL depends on: https://codereview.chromium.org/2710823003

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] Coordination Unit Manager Modularization

Encapsulate GRC functionalities related to coordination unit management
as this is only one aspect of GRC. There are also a few minor
refactorings included as well.

This CL depends on: https://codereview.chromium.org/2710823003

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-05 22:38:02]

On 2017/06/01 at 21:57:31, oysteine wrote:
> On 2017/06/01 at 21:57:04, oystein wrote:
> > lgtm and thanks!
> > 
> >
https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
> > File
services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):
> > 
> >
https://codereview.chromium.org/2914003002/diff/60001/services/resource_coord...
> > services/resource_coordinator/coordination_unit/coordination_unit_impl.h:39:
CoordinationUnitID& id() { return id_; }
> > On 2017/06/01 at 21:05:14, matthalp wrote:
> > > On 2017/06/01 at 18:29:08, oystein wrote:
> > > > const
> > > 
> > > Done.
> > > 
> > > Additionally, I made id_ itself const. However, this required changing the
constructor logic a little bit because const member variables can only be
initialized in constructor initializer lists (so the if-then logic had to be
removed), so I moved that logic into the CoordinationUnitID constructor itself.
Was that a reasonable change?
> > 
> > Yep seems totally fine to me!
> 
> Oh, description s/Encapusalte/Encapsulate/

Good catch! Done.

[matthalp, 2017-06-05 23:48:39]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 23:49:23]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-05 23:56:41]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-05 23:58:02]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 23:59:02]

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, 2017-06-06 02:27:36]

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

[commit-bot: I haz the power, 2017-06-06 02:27:37]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-06-06 03:37:04]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-06-06 03:37:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org
Link to the patchset: https://codereview.chromium.org/2914003002/#ps120001
(title: "Add CoordinationUnitManager comment")

[commit-bot: I haz the power, 2017-06-06 03:37:26]

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, 2017-06-06 03:43:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496720224251340,
"parent_rev": "57c9f0fa2ca884f44b783e3b7aaf8ce528fe4886", "commit_rev":
"fc313920006ac7f40bac4eceaa97f17b2ec7e7f1"}

[commit-bot: I haz the power, 2017-06-06 03:43:27]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496720224251340,
"parent_rev": "fa588f3388772151d966af82422a634c48b27447", "commit_rev":
"aeff4062e3f04eb0a0c4ff0333bc989a37054280"}

[commit-bot: I haz the power, 2017-06-06 03:43:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496720224251340,
"parent_rev": "c52947054f9441624c9cb7287ce41ac80024160b", "commit_rev":
"85378cec1bed7f00a92ff1c658eed044e2518a79"}

[commit-bot: I haz the power, 2017-06-06 03:43:52]

Description was changed from

==========
[GRC] Coordination Unit Manager Modularization

Encapsulate GRC functionalities related to coordination unit management
as this is only one aspect of GRC. There are also a few minor
refactorings included as well.

This CL depends on: https://codereview.chromium.org/2710823003

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] Coordination Unit Manager Modularization

Encapsulate GRC functionalities related to coordination unit management
as this is only one aspect of GRC. There are also a few minor
refactorings included as well.

This CL depends on: https://codereview.chromium.org/2710823003

R=oysteine@chromium.org
BUG=724306

Review-Url: https://codereview.chromium.org/2914003002
Cr-Commit-Position: refs/heads/master@{#477184}
Committed:
https://chromium.googlesource.com/chromium/src/+/85378cec1bed7f00a92ff1c658ee...
==========

[commit-bot: I haz the power, 2017-06-06 03:43:55]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/85378cec1bed7f00a92ff1c658ee...

[lpy, 2017-06-16 20:17:07]

lpy@chromium.org changed reviewers:
+ lpy@chromium.org

[lpy, 2017-06-16 20:17:08]

Just drive by

https://codereview.chromium.org/2914003002/diff/120001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_manager.h
(right):

https://codereview.chromium.org/2914003002/diff/120001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:30:
static void Create(
Why is it declared only?

[matthalp, 2017-06-16 20:44:43]

https://codereview.chromium.org/2914003002/diff/120001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_manager.h
(right):

https://codereview.chromium.org/2914003002/diff/120001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:30:
static void Create(
On 2017/06/16 at 20:17:07, lpy wrote:
> Why is it declared only?

Thank you for catching this. There is no need for it and I will make sure it's
removed in a future CL.