[GRC] Coordination Unit Graph Observer V3

[GRC] Coordination Unit Graph Observer V3*

This is a temporary landing place for: https://codereview.chromium.org/2942403002

R=*oysteine@chromium.org,*lpy@chromium.org,zhenw@chromium.org
BUG=724306

[matthalp, 2017-06-20 18:58:50]

Description was changed from

==========
Initialize CL

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

to

==========
[GRC] Coordination Unit Graph Observer V3

The CoordinationUnitGraphObserver is a framework for observing and responding to
“interesting events” that occur throughout coordination unit lifetimes’ within
GRC. It utilizes an event listener model similar in spirit to JavaScript’s
event-driven programming model. 

Design Doc Link:
TODO

Example Usage:
TODO

Example Usage:
https://codereview.chromium.org/2946683002

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

[matthalp, 2017-06-20 18:58:50]

matthalp@google.com changed reviewers:
+ lpy@chromium.org, oysteine@chromium.org, zhenw@chromium.org

[matthalp, 2017-06-20 18:58:50]

matthalp@google.com changed required reviewers:
+ oysteine@chromium.org

[matthalp, 2017-06-20 18:59:22]

Description was changed from

==========
[GRC] Coordination Unit Graph Observer V3

The CoordinationUnitGraphObserver is a framework for observing and responding to
“interesting events” that occur throughout coordination unit lifetimes’ within
GRC. It utilizes an event listener model similar in spirit to JavaScript’s
event-driven programming model. 

Design Doc Link:
TODO

Example Usage:
TODO

Example Usage:
https://codereview.chromium.org/2946683002

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

to

==========
[GRC] Coordination Unit Graph Observer V3

The CoordinationUnitGraphObserver is a framework for observing and responding to
“interesting events” that occur throughout coordination unit lifetimes’ within
GRC. It utilizes an event listener model similar in spirit to JavaScript’s
event-driven programming model. 

Design Doc Link:
TODO

Example Usage:
TODO

Example Usage:
https://codereview.chromium.org/2946683002

R=*oysteine@chromium.org,*lpy@chromium.org,zhenw@chromium.org
BUG=724306
==========

[matthalp, 2017-06-20 18:59:22]

matthalp@google.com changed required reviewers:
+ lpy@chromium.org

[matthalp, 2017-06-20 18:59:51]

Description was changed from

==========
[GRC] Coordination Unit Graph Observer V3

The CoordinationUnitGraphObserver is a framework for observing and responding to
“interesting events” that occur throughout coordination unit lifetimes’ within
GRC. It utilizes an event listener model similar in spirit to JavaScript’s
event-driven programming model. 

Design Doc Link:
TODO

Example Usage:
TODO

Example Usage:
https://codereview.chromium.org/2946683002

R=*oysteine@chromium.org,*lpy@chromium.org,zhenw@chromium.org
BUG=724306
==========

to

==========
[GRC] Coordination Unit Graph Observer V3*

*V3 will be removed after lgtms and before commit

The CoordinationUnitGraphObserver is a framework for observing and responding to
“interesting events” that occur throughout coordination unit lifetimes’ within
GRC. It utilizes an event listener model similar in spirit to JavaScript’s
event-driven programming model. 

Design Doc Link:
TODO

Example Usage:
TODO

Example Usage:
https://codereview.chromium.org/2946683002

R=*oysteine@chromium.org,*lpy@chromium.org,zhenw@chromium.org
BUG=724306
==========

[matthalp, 2017-06-20 19:03:22]

Description was changed from

==========
[GRC] Coordination Unit Graph Observer V3*

*V3 will be removed after lgtms and before commit

The CoordinationUnitGraphObserver is a framework for observing and responding to
“interesting events” that occur throughout coordination unit lifetimes’ within
GRC. It utilizes an event listener model similar in spirit to JavaScript’s
event-driven programming model. 

Design Doc Link:
TODO

Example Usage:
TODO

Example Usage:
https://codereview.chromium.org/2946683002

R=*oysteine@chromium.org,*lpy@chromium.org,zhenw@chromium.org
BUG=724306
==========

to

==========
[GRC] Coordination Unit Graph Observer V3*

This is a temporary landing place for:
https://codereview.chromium.org/2942403002

R=*oysteine@chromium.org,*lpy@chromium.org,zhenw@chromium.org
BUG=724306
==========

[lpy, 2017-06-20 19:53:46]

awesome! Thanks for cleaning it up! Overall it l-g to me, just a nit and a
comment :)

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
(right):

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:96:
size_t on_child_added_listener_invocations_;
IMHO, Let's use
child_added_count_/parent_added_count_/coodination_unit_created_count_/property_changed_count_/child_removed_count_/parent_removed_count/will_be_destroyed_count_

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
(right):

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:47:
AddObserversToCoordinationUnit(coordination_unit);
Should we leave this to CUGraphObserver to decide whether or not it wants to add
itself to a CU? I imagine that in a CUGraphObserver implementation, it would
have code like:

void ConcreteCoordinationUnitObserver::OnCoordinationUnitCreated(cu) {
  if (cu is the type of CUs I want to observe) {
    cu->AddObserver(this);
  }
}

WDYT?

[oystein (OOO til 10th of July), 2017-06-20 20:21:07]

lg2m with minors

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
(right):

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:108:
EXPECT_EQ(0u, coordination_unit_manager().observers().size());
nit: EXPECT_TRUE(.empty());

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
(right):

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:47:
AddObserversToCoordinationUnit(coordination_unit);
On 2017/06/20 at 19:53:46, lpy wrote:
> Should we leave this to CUGraphObserver to decide whether or not it wants to
add itself to a CU? I imagine that in a CUGraphObserver implementation, it would
have code like:
> 
> void ConcreteCoordinationUnitObserver::OnCoordinationUnitCreated(cu) {
>   if (cu is the type of CUs I want to observe) {
>     cu->AddObserver(this);
>   }
> }
> 
> WDYT?

Minor API nit: As discussed in the meeting, I think it's slightly cleaner to
separate the event callbacks from the "should I observe X" virtual which always
gets called.

so:
virtual bool ShouldObserve(cu) = 0;
virtual void OnCoordinationUnitCreated(cu) {};

[lpy, 2017-06-20 20:47:25]

> Minor API nit: As discussed in the meeting, I think it's slightly cleaner to
> separate the event callbacks from the "should I observe X" virtual which
always
> gets called.
> 
> so:
> virtual bool ShouldObserve(cu) = 0;
> virtual void OnCoordinationUnitCreated(cu) {};

It's not clear to me it's cleaner. Will ShouldObserve(cu) be called by
CUManager? If yes we will end up with code like:

CUManager::OnCoordinationUnitCreated(cu) {
  for (auto& observer : observers_) {
    if (observer->ShouldObserve(cu)) {
      observer->OnCoordinationUnitCreated(cu);
    }
  }
}

CUManager::OnCoordinationUnitWillBeDestroyed(cu) {
  for (auto& observer : observers_) {
    if (observer->ShouldObserve(cu)) {
      observer->OnCoordinationUnitWillBeDestroyed(cu);
    }
  }
}

Do I understand correctly?

[matthalp, 2017-06-20 20:56:09]

I have addressed lpy and oysteine's feedback. However, I have pushed the code
back to the original CL now that we have reached an agreement. Let's continue
the ShouldObserve conversation there: https://codereview.chromium.org/2942403002

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
(right):

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:96:
size_t on_child_added_listener_invocations_;
On 2017/06/20 at 19:53:46, lpy wrote:
> IMHO, Let's use
child_added_count_/parent_added_count_/coodination_unit_created_count_/property_changed_count_/child_removed_count_/parent_removed_count/will_be_destroyed_count_

Done.

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:108:
EXPECT_EQ(0u, coordination_unit_manager().observers().size());
On 2017/06/20 at 20:21:07, oystein wrote:
> nit: EXPECT_TRUE(.empty());

Agreed -- Done. Always better to use .empty() than size() == 0.

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
File
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
(right):

https://codereview.chromium.org/2953483002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:47:
AddObserversToCoordinationUnit(coordination_unit);
On 2017/06/20 at 20:21:07, oystein wrote:
> On 2017/06/20 at 19:53:46, lpy wrote:
> > Should we leave this to CUGraphObserver to decide whether or not it wants to
add itself to a CU? I imagine that in a CUGraphObserver implementation, it would
have code like:
> > 
> > void ConcreteCoordinationUnitObserver::OnCoordinationUnitCreated(cu) {
> >   if (cu is the type of CUs I want to observe) {
> >     cu->AddObserver(this);
> >   }
> > }
> > 
> > WDYT?
> 
> Minor API nit: As discussed in the meeting, I think it's slightly cleaner to
separate the event callbacks from the "should I observe X" virtual which always
gets called.
> 
> so:
> virtual bool ShouldObserve(cu) = 0;
> virtual void OnCoordinationUnitCreated(cu) {};

I agree with oysteine  that ShouldObserve should be used in place of deciding
within CoordinationUnitCreated. This decoupling allows us to make the
CoordinationUnitCreated(const CoordinationUnitImpl*) which is consistent with
the parameters for the rest of the API.

[oystein (OOO til 10th of July), 2017-06-20 21:06:36]

On 2017/06/20 at 20:47:25, lpy wrote:
> > Minor API nit: As discussed in the meeting, I think it's slightly cleaner to
> > separate the event callbacks from the "should I observe X" virtual which
always
> > gets called.
> > 
> > so:
> > virtual bool ShouldObserve(cu) = 0;
> > virtual void OnCoordinationUnitCreated(cu) {};
> 
> It's not clear to me it's cleaner. Will ShouldObserve(cu) be called by
CUManager? If yes we will end up with code like:
> 
> CUManager::OnCoordinationUnitCreated(cu) {
>   for (auto& observer : observers_) {
>     if (observer->ShouldObserve(cu)) {
>       observer->OnCoordinationUnitCreated(cu);
>     }
>   }
> }
> 
> CUManager::OnCoordinationUnitWillBeDestroyed(cu) {
>   for (auto& observer : observers_) {
>     if (observer->ShouldObserve(cu)) {
>       observer->OnCoordinationUnitWillBeDestroyed(cu);
>     }
>   }
> }
> 
> Do I understand correctly?

Not quite:
CUManager::AddObserversToCoordinationUnit(cu) {
   for (auto& observer : observers_) {
      if (observer->ShouldObserve(cu))
         cu->AddObserver(observer);
   }

   cu->OnCoordinationUnitCreated();
}

CoordinationUnitImpl::OnCoordinationUnitCreated() {
  for (auto& observer : observers_) {
    observer.OnCoordinationUnitCreated(this);
  }
}

CoordinationUnitImpl::~CoordinationUnitImpl() {
  // This could be done on an OnCoordinationUnitDestroyed step prior to
destruction if we want to avoid destructor ordering issues.
  for (auto& observer : observers_) {
    observer.OnCoordinationUnitWillBeDestroyed(this);
  }
}


The benefit is that the observers are already all set up when
OnCoordinationUnitCreated() is called, so it separates construction from events.

Not a big deal either way though, it just also feels like a slightly more easily
readable API for observer implementors rather than:
// The observer needs to implement this to register itself as an observer with
the CU to receive any further events.
virtual void AddObserversToCoordinationUnit() = 0;