[GRC] Process Coordination Unit support for Global Resource Controller

[GRC] Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

R=oysteine@chromium.org, rockot@chromium.org
BUG=724306
Review-Url: https://codereview.chromium.org/2892443002
Cr-Commit-Position: refs/heads/master@{#473620}
Committed: https://chromium.googlesource.com/chromium/src/+/e97f707738a7c04418efe00cd4c68d5ef2fea3c8

[oystein (OOO til 10th of July), 2017-05-17 23:06:07]

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_factory.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.cc:5:
#include
"services/resource_coordinator/coordination_unit/coordination_unit_factory.h"
nit: empty line between the associated header file for the .cc, and the rest of
them.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:11: 
nit: remove blank line

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:13: 
nit: remove blank line

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:14:
}  // service_manager
no need for the // service_manager comment when you're just using namespaces for
forward declarations.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:20:
class CoordinationUnitFactory {
Since we're wrapped in a namespace here anyway and just have a single static
function, I wouldn't bother with the class at all (though a nested
CoordinationUnitFactory namespace is probably good, to keep the naming still).

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:27:
DISALLOW_COPY_AND_ASSIGN(CoordinationUnitFactory);
nit: should include "base/macros.h" for this, for completeness. Same with
<memory> for the unique_ptr usage (i.e. shouldn't rely on the indirect headers
from coordination_unit_impl.h.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:221:
return -1;
-1.0 to match types.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:5:
#ifndef
SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_COORDINATION_UNIT_BASE_TEST_UNITTEST_H_
The name is a little confusing; maybe coordination_unit_impl_unittest_util.h
instead, that's a name pattern I've seen elsewhere for these kinds of helper
classes.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:15:
class CoordinationUnitImplBaseTest : public testing::Test {
bikeshedding naming nit: CoordinationUnitImplTestBase instead maybe, since it's
a base for testing classes, not a test class for the base class?

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:18:
~CoordinationUnitImplBaseTest() override {}
put the destructor body in the .cc, since it's a non-trivial destructor (and =
default; instead of {}).

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:24:
service_manager::ServiceContextRefFactory* ServiceContextRefFactory() {
service_context_ref_factory() since it's a simple inlined getter function.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:16:

nit: ditch empty lines here

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:19:
}  // namespace service_manager
nit: ditto for ditching comment; no need when there's just one forward declare.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:50:
void ProcessCoordinationUnitImpl::ScheduleProcessCPUProfiling(
I would just ditch this function and just start the timer directly in both
calling locations since it just contains the single call; it makes it a little
bit clearer that MeasureProcessCPUUsage() is just calling itself.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:13:

nit: ditch the blank lines here and the //namespace comment.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:33:
std::unique_ptr<base::ProcessMetrics> process_metrics_;
nit: #include <memory> for this.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:37:
DISALLOW_COPY_AND_ASSIGN(ProcessCoordinationUnitImpl);
nit: #include "base/macros.h"

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:28:
cu_id, ServiceContextRefFactory());
Do we know what happens to the created ServiceContextRefs stored in the
CoordinationUnitImpls after the ServiceContextRefFactory gets destroyed after
this call? (I honestly don't know; I'd double-check or make it a member of
ProcessCoordinationUnitImplTest).

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:32:
EXPECT_LE(0.0, coordination_unit_->GetCPUUsageForTesting());
Shouldn't this be EXPECT_GE?

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File services/resource_coordinator/public/cpp/coordination_unit_id.h (right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/public/cpp/coordination_unit_id.h:21:
CoordinationUnitID(const CoordinationUnitType& type, uint64_t new_id);
Slight yak shave, sorry: Can you make the id type a typedef in this class,
rather than a hardcoded uint64_t? I suspect it may be something that may change
again later and something we'll potentially refer to in a lot of other places.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:15:
LOG(ERROR) << "OnConnectionError";
Leftover log line?

[matthalp, 2017-05-18 16:13:46]

Your feedback/comments/suggests were all reasonable and I took your advice for
all of them.

There is one I want to discuss further though:

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...

https://codereview.chromium.org/2892443002/diff/20001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h
(right):

https://codereview.chromium.org/2892443002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:28:
CoordinationUnitImplBaseTest()
Is this good here or should the relevant parts be moved to the .cc?

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_factory.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.cc:5:
#include
"services/resource_coordinator/coordination_unit/coordination_unit_factory.h"
On 2017/05/17 at 23:06:05, oystein wrote:
> nit: empty line between the associated header file for the .cc, and the rest
of them.

Done.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:11: 
On 2017/05/17 at 23:06:06, oystein wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:13: 
On 2017/05/17 at 23:06:06, oystein wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:14:
}  // service_manager
On 2017/05/17 at 23:06:05, oystein wrote:
> no need for the // service_manager comment when you're just using namespaces
for forward declarations.

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:20:
class CoordinationUnitFactory {
On 2017/05/17 at 23:06:05, oystein wrote:
> Since we're wrapped in a namespace here anyway and just have a single static
function, I wouldn't bother with the class at all (though a nested
CoordinationUnitFactory namespace is probably good, to keep the naming still).

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:27:
DISALLOW_COPY_AND_ASSIGN(CoordinationUnitFactory);
On 2017/05/17 at 23:06:05, oystein wrote:
> nit: should include "base/macros.h" for this, for completeness. Same with
<memory> for the unique_ptr usage (i.e. shouldn't rely on the indirect headers
from coordination_unit_impl.h.

N/A since this is no longer a class as the code is now a namespaced function.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:221:
return -1;
On 2017/05/17 at 23:06:06, oystein wrote:
> -1.0 to match types.

Done.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:5:
#ifndef
SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_COORDINATION_UNIT_BASE_TEST_UNITTEST_H_
On 2017/05/17 at 23:06:06, oystein wrote:
> The name is a little confusing; maybe coordination_unit_impl_unittest_util.h
instead, that's a name pattern I've seen elsewhere for these kinds of helper
classes.

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:15:
class CoordinationUnitImplBaseTest : public testing::Test {
On 2017/05/17 at 23:06:06, oystein wrote:
> bikeshedding naming nit: CoordinationUnitImplTestBase instead maybe, since
it's a base for testing classes, not a test class for the base class?

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:18:
~CoordinationUnitImplBaseTest() override {}
On 2017/05/17 at 23:06:06, oystein wrote:
> put the destructor body in the .cc, since it's a non-trivial destructor (and =
default; instead of {}).

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl_base_test_unittest.h:24:
service_manager::ServiceContextRefFactory* ServiceContextRefFactory() {
On 2017/05/17 at 23:06:06, oystein wrote:
> service_context_ref_factory() since it's a simple inlined getter function.

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:16:

On 2017/05/17 at 23:06:06, oystein wrote:
> nit: ditch empty lines here

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:19:
}  // namespace service_manager
On 2017/05/17 at 23:06:06, oystein wrote:
> nit: ditto for ditching comment; no need when there's just one forward
declare.

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:50:
void ProcessCoordinationUnitImpl::ScheduleProcessCPUProfiling(
On 2017/05/17 at 23:06:06, oystein wrote:
> I would just ditch this function and just start the timer directly in both
calling locations since it just contains the single call; it makes it a little
bit clearer that MeasureProcessCPUUsage() is just calling itself.

Done.

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

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:13:

On 2017/05/17 at 23:06:06, oystein wrote:
> nit: ditch the blank lines here and the //namespace comment.

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:33:
std::unique_ptr<base::ProcessMetrics> process_metrics_;
On 2017/05/17 at 23:06:07, oystein wrote:
> nit: #include <memory> for this.

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:37:
DISALLOW_COPY_AND_ASSIGN(ProcessCoordinationUnitImpl);
On 2017/05/17 at 23:06:07, oystein wrote:
> nit: #include "base/macros.h"

Done.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:28:
cu_id, ServiceContextRefFactory());
On 2017/05/17 at 23:06:07, oystein wrote:
> Do we know what happens to the created ServiceContextRefs stored in the
CoordinationUnitImpls after the ServiceContextRefFactory gets destroyed after
this call? (I honestly don't know; I'd double-check or make it a member of
ProcessCoordinationUnitImplTest).

Maybe I am misunderstanding, but I think this has already been taken care of
since it service_ref_ is already a member of the CoordinationUnitImpl that
ProcessCoordinationUnitImpl is derived from.

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:32:
EXPECT_LE(0.0, coordination_unit_->GetCPUUsageForTesting());
On 2017/05/17 at 23:06:07, oystein wrote:
> Shouldn't this be EXPECT_GE?

EXPECT_LE(val1, val2) verifies val1 <= val2 [1] and we want to ensure CPU
utilization is non-negative.

[1] https://github.com/google/googletest/blob/master/googletest/docs/Primer.md

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File services/resource_coordinator/public/cpp/coordination_unit_id.h (right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/public/cpp/coordination_unit_id.h:21:
CoordinationUnitID(const CoordinationUnitType& type, uint64_t new_id);
On 2017/05/17 at 23:06:07, oystein wrote:
> Slight yak shave, sorry: Can you make the id type a typedef in this class,
rather than a hardcoded uint64_t? I suspect it may be something that may change
again later and something we'll potentially refer to in a lot of other places.

Done, but with a caveat:

The most straight-forward typedef would have been CoordinationUnitI(d|D) which
would be consistent with the CoordinationUnitType. However, CoordinationUnitID
is the name of the class itself, so there is some ambiguity.

What I did (but am not especially fond of):

  typedef CoordinationUnitTypeId uint64_t

  This rationale is that id field is essentially uniquely identifying the CU of
the type. However reading that type could also be interpreted as the
CoordinationUnitType to the uninformed developer

More Pragramatic, but bigger yak shave:

  Change the CoordinationUnitID struct to CoordinationUnitUID (a
CoordinationUnit Unique Identifier) and then make the typedef
CoordinationUnitId. This is because the <type, id> together create a unique
identifier for an instance of CoordinationUnitImpl (and its derived classes)

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:15:
LOG(ERROR) << "OnConnectionError";
On 2017/05/17 at 23:06:07, oystein wrote:
> Leftover log line?

Done.

[oystein (OOO til 10th of July), 2017-05-18 21:24:32]

lgtm w/a few last comments

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc
(right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:28:
cu_id, ServiceContextRefFactory());
On 2017/05/18 at 16:13:46, matthalp wrote:
> On 2017/05/17 at 23:06:07, oystein wrote:
> > Do we know what happens to the created ServiceContextRefs stored in the
CoordinationUnitImpls after the ServiceContextRefFactory gets destroyed after
this call? (I honestly don't know; I'd double-check or make it a member of
ProcessCoordinationUnitImplTest).
> 
> Maybe I am misunderstanding, but I think this has already been taken care of
since it service_ref_ is already a member of the CoordinationUnitImpl that
ProcessCoordinationUnitImpl is derived from.

Yeah but it's being created from the ServiceContextRefFactory that dies right
after this call, i.e. the service_ref_ outlives the factory. Note:  haven't
looked into whether this is actually a problem or not, but I would guess that
service_ref_ would call back into the factory when it gets destructed?

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:32:
EXPECT_LE(0.0, coordination_unit_->GetCPUUsageForTesting());
On 2017/05/18 at 16:13:46, matthalp wrote:
> On 2017/05/17 at 23:06:07, oystein wrote:
> > Shouldn't this be EXPECT_GE?
> 
> EXPECT_LE(val1, val2) verifies val1 <= val2 [1] and we want to ensure CPU
utilization is non-negative.
> 
> [1] https://github.com/google/googletest/blob/master/googletest/docs/Primer.md

Makes sense!

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
File services/resource_coordinator/public/cpp/coordination_unit_id.h (right):

https://codereview.chromium.org/2892443002/diff/60001/services/resource_coord...
services/resource_coordinator/public/cpp/coordination_unit_id.h:21:
CoordinationUnitID(const CoordinationUnitType& type, uint64_t new_id);
On 2017/05/18 at 16:13:46, matthalp wrote:
> On 2017/05/17 at 23:06:07, oystein wrote:
> > Slight yak shave, sorry: Can you make the id type a typedef in this class,
rather than a hardcoded uint64_t? I suspect it may be something that may change
again later and something we'll potentially refer to in a lot of other places.
> 
> Done, but with a caveat:
> 
> The most straight-forward typedef would have been CoordinationUnitI(d|D) which
would be consistent with the CoordinationUnitType. However, CoordinationUnitID
is the name of the class itself, so there is some ambiguity.
> 
> What I did (but am not especially fond of):
> 
>   typedef CoordinationUnitTypeId uint64_t
> 
>   This rationale is that id field is essentially uniquely identifying the CU
of the type. However reading that type could also be interpreted as the
CoordinationUnitType to the uninformed developer
> 
> More Pragramatic, but bigger yak shave:
> 
>   Change the CoordinationUnitID struct to CoordinationUnitUID (a
CoordinationUnit Unique Identifier) and then make the typedef
CoordinationUnitId. This is because the <type, id> together create a unique
identifier for an instance of CoordinationUnitImpl (and its derived classes)


Yep that makes sense to me, including the rename: Can you add a comment
regarding that, above the class? '// TODO(oysteine): Rename to
CoordinationUnitGUID to better differentiate the class from the internal id', or
something.

https://codereview.chromium.org/2892443002/diff/80001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_factory.h
(right):

https://codereview.chromium.org/2892443002/diff/80001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_factory.h:20:
namespace CoordinationUnitFactory {
Actually, my fault, this should be "coordination_unit_factory" instead.

https://codereview.chromium.org/2892443002/diff/80001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2892443002/diff/80001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:16:

nit: some redundant empty lines+comment here still.

https://codereview.chromium.org/2892443002/diff/80001/services/resource_coord...
services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:49:
ProcessCoordinationUnitImpl::~ProcessCoordinationUnitImpl() {}
nit: = default

[matthalp, 2017-05-18 22:43:16]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-05-18 22:43:16]

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/2892443002/#ps100001
(title: "Address reviewer feedback")

[commit-bot: I haz the power, 2017-05-18 22:45:19]

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-18 22:56:02]

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

[commit-bot: I haz the power, 2017-05-18 22:56:04]

Try jobs failed on following builders:
  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-18 22:58:39]

Description was changed from

==========
Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

to

==========
Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

[matthalp, 2017-05-18 23:04:47]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-05-18 23:04:47]

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/2892443002/#ps120001
(title: "Include Mac OS X dependency")

[matthalp, 2017-05-18 23:05:29]

Description was changed from

==========
Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

to

==========
[GRC] Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

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

CQ is trying da patch.

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

[matthalp, 2017-05-18 23:10:27]

On 2017/05/18 at 23:06:19, commit-bot wrote:
> CQ is trying da patch.
> 
> Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

+rockot For DEPS on services/service_manager/public

[matthalp, 2017-05-18 23:11:24]

Description was changed from

==========
[GRC] Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

to

==========
[GRC] Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

[matthalp, 2017-05-18 23:11:24]

matthalp@google.com changed reviewers:
+ rockot@chromium.org

[matthalp, 2017-05-18 23:11:48]

On 2017/05/18 at 23:10:27, matthalp wrote:
> On 2017/05/18 at 23:06:19, commit-bot wrote:
> > CQ is trying da patch.
> > 
> > Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> +rockot For DEPS on services/service_manager/public

+rockot For DEPS on services/service_manager/public

[commit-bot: I haz the power, 2017-05-18 23:13:46]

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

[commit-bot: I haz the power, 2017-05-18 23:13:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ken Rockot(use gerrit already), 2017-05-18 23:15:29]

lgtm

[matthalp, 2017-05-18 23:19:13]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-05-18 23:42:21]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2889273002/ by matthalp@google.com.

The reason for reverting is: CQed before having final LGTM.

[matthalp, 2017-05-18 23:45:33]

The CQ bit was checked by matthalp@google.com

[commit-bot: I haz the power, 2017-05-18 23:47:37]

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-18 23:57:51]

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

[commit-bot: I haz the power, 2017-05-18 23:57:53]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[matthalp, 2017-05-19 16:24:39]

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

[commit-bot: I haz the power, 2017-05-19 16:25: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, 2017-05-19 16:33:24]

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

[commit-bot: I haz the power, 2017-05-19 16:33:25]

Dry run: Try jobs failed on following builders:
  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-19 16:45:29]

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

[commit-bot: I haz the power, 2017-05-19 16:46:24]

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-19 17:25:01]

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

[commit-bot: I haz the power, 2017-05-19 17:25:02]

Dry run: Try jobs failed on following builders:
  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-20 17:04:57]

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

[commit-bot: I haz the power, 2017-05-20 17:05:09]

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-20 18:10:25]

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

[commit-bot: I haz the power, 2017-05-20 18:10: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, 2017-05-20 18:19:42]

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

[commit-bot: I haz the power, 2017-05-20 18:19:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[matthalp, 2017-05-20 18:23:24]

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

[commit-bot: I haz the power, 2017-05-20 18:23: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, 2017-05-20 19:47:56]

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

[commit-bot: I haz the power, 2017-05-20 19:47:57]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-05-22 03:59:00]

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

[commit-bot: I haz the power, 2017-05-22 03:59: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, 2017-05-22 05:15:06]

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

[commit-bot: I haz the power, 2017-05-22 05:15:07]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-05-22 15:16:08]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-05-22 15:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2892443002/#ps220001
(title: "Correct OS-specific Process constructor calls")

[commit-bot: I haz the power, 2017-05-22 15:16: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, 2017-05-22 15:24:08]

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

[commit-bot: I haz the power, 2017-05-22 15:24:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[matthalp, 2017-05-22 16:17:31]

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

[commit-bot: I haz the power, 2017-05-22 16:17:54]

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-22 17:49:56]

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

[commit-bot: I haz the power, 2017-05-22 17:49:57]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-05-22 17:50:28]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-05-22 17:50:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2892443002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-22 17:51:01]

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-22 17:58:13]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495475428690590,
"parent_rev": "af124f87be0846b2541f69d306c0bb705b5e9e9f", "commit_rev":
"e97f707738a7c04418efe00cd4c68d5ef2fea3c8"}

[commit-bot: I haz the power, 2017-05-22 17:58:32]

Description was changed from

==========
[GRC] Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

to

==========
[GRC] Process Coordination Unit support for Global Resource Controller

Quick design tl;dr:

* ProcessCoordinationUnitImpl
  Specific implementation of CoordinationUnitImpl for tracking
  processes within GRC of which it is derived from

* CoordinationUnitFactory
  Currently supports making ProcessCoordinationUnitImpl for
  CoordinationUnitType:kProcess and then a vanilla
  CoordinationUnitImpl for any ohter CoordinationUnitType

* CoordinationUnitImplBaseTest
  A base class for unit testing CoordinationUnitImpl and
  its derived classes

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

Review-Url: https://codereview.chromium.org/2892443002
Cr-Commit-Position: refs/heads/master@{#473620}
Committed:
https://chromium.googlesource.com/chromium/src/+/e97f707738a7c04418efe00cd4c6...
==========

[commit-bot: I haz the power, 2017-05-22 17:58:34]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/e97f707738a7c04418efe00cd4c6...