[GRC] Coordination Unit Graph Observer

[GRC] Coordination Unit Graph Observer

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: https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2f2FVzP0te6YtCyk/edit?usp=sharing

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

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

[matthalp, 2017-06-07 21:58:30]

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

[matthalp, 2017-06-07 21:58:47]

Adding reviewers

[matthalp, 2017-06-12 20:24:40]

Description was changed from

==========
NOCOMMIT PROTOTYPE [GRC] Coordination Unit Manager Observer

Design Doc Link:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

to

==========
NOCOMMIT PROTOTYPE [GRC] Coordination Unit Manager Observer

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

[matthalp, 2017-06-12 20:24:40]

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

[matthalp, 2017-06-12 20:50:46]

Description was changed from

==========
NOCOMMIT PROTOTYPE [GRC] Coordination Unit Manager Observer

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

to

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

[matthalp, 2017-06-12 20:52:06]

Description was changed from

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

to

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

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

[zhenw, 2017-06-13 18:38:40]

zhenw@google.com changed reviewers:
+ zhenw@google.com

[zhenw, 2017-06-13 18:38:41]

I have some high level comments.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
(right):

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:27:
class CoordinationUnitGraphObserver {
Do observers outlive all CUs?

The real question is whether we should support removing listeners. Currently it
seems only AddListener is supported. If an observer dies before some CU, it
becomes a problem when that CU later wants to notify the already-dead observer.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:33:
explicit CoordinationUnitGraphObserver(CoordinationUnitType filter);
I am not entirely sure the reason of using filters. Currently there 2 types of
filters:
1. CoordinationUnitGraphObserver(CoordinationUnitType filter)
2. AddListener(listener, filter)

For the 1st one, when a CU is created, manager can just broadcast to all
observers. The observer itself can check the CU type and decide whether to add
listeners.

For the 2nd one, when a CU adds a child, the CU can just broadcast to all
listeners. The listener can just check the CU type and decide whether to
proceed.

The down side of using filter is that the filtering functionality is redundant,
because it can always be achieved by the observers do some checking internally.

The benefit I see is mostly performance, i.e., save work to talk to some
observer/listener. If that is the main reason, map should be used instead of
vector.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
(right):

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:69:
void AddChildListenerInvoked(
These callback functions should also follow the On*() style.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:66:
std::vector<Listener> GetListeners() { return GetListeners(kWildcardFilter); }
There can be 2 ways to interpret this:
1. Get the listeners that do not have filter (the intended implementation).
2. Get all listeners. (My 1st impression was this one).

Maybe rename this function to be more specific.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:69:
std::vector<std::pair<Filter, Listener>> listeners_;
A map of vectors is probably more efficient.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:78:
AddChildEventListener;
Those should follow On*() style.

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

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:45:
std::vector<CoordinationUnitGraphObserver*> observers_;
Any reason not to use unique_ptr here?

If filter is used, using a map is probably more efficient.

[matthalp, 2017-06-13 20:57:11]

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
(right):

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:27:
class CoordinationUnitGraphObserver {
On 2017/06/13 at 18:38:41, zhenw wrote:
> Do observers outlive all CUs?
> 
> The real question is whether we should support removing listeners. Currently
it seems only AddListener is supported. If an observer dies before some CU, it
becomes a problem when that CU later wants to notify the already-dead observer.

(1)  All observers will outlive any CU created within GRC.

  Furthermore, the observers will be alive as long as the 
  ResourceCoordinatorService is alive. Observers are be 
  registered in the CoordinationUnitManager constructor, 
  which is instantiated when the ResourceCoordinatorService
  is created. Observers are not intended to be added to the
  CoordinationUnitManager dynamically.

(2) Future support for RemoveListener

  I have no problem adding a RemoveListener capability once
  we have a definitive use case for it (which I do believe
  there are). It will also add some additional complexity as
  now there has to be a way to identify uniquely identify the
  listener to be removed.

  If you feel like we should add RemoveListener support now or
  at least converge on an implementation for it, I am more than
  happy to do so.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:33:
explicit CoordinationUnitGraphObserver(CoordinationUnitType filter);
On 2017/06/13 at 18:38:41, zhenw wrote:
> I am not entirely sure the reason of using filters. Currently there 2 types of
filters:
> 1. CoordinationUnitGraphObserver(CoordinationUnitType filter)
> 2. AddListener(listener, filter)
> 
> For the 1st one, when a CU is created, manager can just broadcast to all
observers. The observer itself can check the CU type and decide whether to add
listeners.
> 
> For the 2nd one, when a CU adds a child, the CU can just broadcast to all
listeners. The listener can just check the CU type and decide whether to
proceed.
> 
> The down side of using filter is that the filtering functionality is
redundant, because it can always be achieved by the observers do some checking
internally.
> 
> The benefit I see is mostly performance, i.e., save work to talk to some
observer/listener. If that is the main reason, map should be used instead of
vector.

The main intention for filtering was for code health, but also
agree there is a performance benefit with the map approach:

  (1) Readability
    The if-/switch-statement adds an extra level of indentation

  (2) Modularity
    The callback methods can focus on a particular event type.
    I think the TabCPUUsageObserver does a good job demonstrating this:
    https://codereview.chromium.org/2925123002
    Nonetheless, I'm open to removing this capability or figuring out
    an alternative approach. I was motivated by the webRequest WebExtension
    API which uses filtering to isolate traffic from certain domains:
    https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
(right):

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:69:
void AddChildListenerInvoked(
On 2017/06/13 at 18:38:41, zhenw wrote:
> These callback functions should also follow the On*() style.

I will do that.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:66:
std::vector<Listener> GetListeners() { return GetListeners(kWildcardFilter); }
On 2017/06/13 at 18:38:41, zhenw wrote:
> There can be 2 ways to interpret this:
> 1. Get the listeners that do not have filter (the intended implementation).
> 2. Get all listeners. (My 1st impression was this one).
> 
> Maybe rename this function to be more specific.

I see the ambiguity. I don't believe this method is used,
so it might just be a good idea to get rid of it and add
a comment to say that kWildcardFilter (or a better-named
variable) that gets all of the listeners without a filter.

Otherwise I can do GetListenersWithoutFilter.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:69:
std::vector<std::pair<Filter, Listener>> listeners_;
On 2017/06/13 at 18:38:41, zhenw wrote:
> A map of vectors is probably more efficient.

I can do this.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:78:
AddChildEventListener;
On 2017/06/13 at 18:38:41, zhenw wrote:
> Those should follow On*() style.

I can do this.

[Zhen Wang, 2017-06-14 00:33:03]

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
(right):

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:27:
class CoordinationUnitGraphObserver {
OK. As long as observers outlive any CU, it is fine. Please also document this.

https://codereview.chromium.org/2926663003/diff/260001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:33:
explicit CoordinationUnitGraphObserver(CoordinationUnitType filter);
OK. Let's keep filtering.

[matthalp, 2017-06-15 02:39:22]

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

[commit-bot: I haz the power, 2017-06-15 02:39:35]

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-15 02:40:09]

matthalp@google.com changed reviewers:
- zhenw@google.com

[commit-bot: I haz the power, 2017-06-15 04:37:14]

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

[commit-bot: I haz the power, 2017-06-15 04:37:15]

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

[matthalp, 2017-06-15 14:50:37]

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

[commit-bot: I haz the power, 2017-06-15 14:50:53]

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-15 15:26:32]

Description was changed from

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

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

to

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

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

[matthalp, 2017-06-15 15:26:36]

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

[commit-bot: I haz the power, 2017-06-15 15:26:58]

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-15 17:00:03]

Description was changed from

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

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

to

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

R=*oysteine@chromium.org,*zhenw@chromium.org,lpy@google.com,fmeawad@chromium.org

BUG=724306
==========

[matthalp, 2017-06-15 17:00:03]

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

[commit-bot: I haz the power, 2017-06-15 17:15:17]

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

[commit-bot: I haz the power, 2017-06-15 17:15:18]

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

[matthalp, 2017-06-15 17:22:41]

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

[commit-bot: I haz the power, 2017-06-15 17:23:00]

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-15 17:23:15]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-15 17:23:19]

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

[commit-bot: I haz the power, 2017-06-15 17:23: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, 2017-06-15 19:33:25]

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

[commit-bot: I haz the power, 2017-06-15 19:33:26]

Dry run: This issue passed the CQ dry run.

[Zhen Wang, 2017-06-15 20:07:53]

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:38:
// The CoordinationUnitmanagerObserver will only call
s/CoordinationUnitmanagerObserver/CoordinationUnitManager ?

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:44:
CoordinationUnitType filter() const { return filter_; }
Instead of exposing filter type, how about using something like ShouldObserve()?
And inside the function, it actually checks the filter type.

The reason is that if the filter type is invalid, it indicates that we do not
filter anything. When the manager does the check, it is not intuitive that
"invalid" actually means "do not filter".

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:46:
// Called whenever any new CoordinationUnit of any type is created. Note that
This is only called when it matches the filter, right. And if the filter is
kInvalidType, it is called for any type. If we switch to use ShouldObserve() or
something similar, we can just say this function is only called when it should
observe this CU.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:30:
CoordinationUnitType filter)
If this does not test filter, this class should not have constructor taking in
filter.

If this is only used for subclass purpose, put it in protected.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:69:
void AddChildListenerInvoked(
Should these callbacks be put into protected section?

And they need to be renamed to On*() style.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:122:
unsigned int add_child_listener_invocations_;
Use size_t for these:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:179:
TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedNoFilters) {
nit: s/CallbacksInvokedNoFilters/CallbacksInvokedWithoutFilters

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:182:
TestBasicAPICoordinationUnitGraphObserver* observer =
Use smart pointers and pass it to manager with std::move. With that, you do not
even need the comment above because people will understand manager "owns"
observers and will do the corresponding cleanup.
https://www.chromium.org/developers/smart-pointer-guidelines

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:223:
TestFilterAPICoordinationUnitGraphObserver* observer =
ditto: use smart pointer.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:256:
// Should not invoke the CoordinationUnitCreated listener
Combine the comments here to one: Shoudl invoke the CoordinationUnitCreated
listener for frame CU only.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:267:
frame_coordination_unit.get());
The comment says it does not invoke the listener. Why not?

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:278:
// Should invoke the observer's AddParent listener.
Why AddParent? Typo?

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:280:
// Should not invoke the observer's AddParent listener.
Why AddParent? Typo?

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:289:
// Should not invoke the observer's RemoveParent listener.
Why RemoveParent? Typo?

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:291:
// Should invoke the observer's RemoveParent listener.
Why RemoveParent? Typo?

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:75: }
I think you can just use std::vector::insert to append a vector directly instead
of looping.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.cc
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.cc:26:
coordination_unit_manager_.UnregisterAllObserversForTesting();
Is there another test helper class that can include the default ones? I am not
sure if we need those for integration test or something similar.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:47:
observer->filter() == CoordinationUnitType::kInvalidType) {
This is not intuitive. See comments in CUGraphObserver.

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

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:45:
std::vector<CoordinationUnitGraphObserver*> observers_;
Use smart pointer instead of raw pointers:
https://www.chromium.org/developers/smart-pointer-guidelines

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc:42:
coordination_unit_manager_->NotifyObserversCoordinationUnitCreated(
Why not call the notify function in CoordinationUnitImpl's constructor? Does it
require binding setup?

[lpy, 2017-06-15 20:08:55]

Description was changed from

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

R=*oysteine@chromium.org,*zhenw@chromium.org,lpy@google.com,fmeawad@chromium.org

BUG=724306
==========

to

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

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:
https://docs.google.com/a/google.com/document/d/1LbtpGa3pu9o4qAIC6VMWrdq3Yvd2...

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

R=*oysteine@chromium.org,*zhenw@chromium.org,lpy@google.com,fmeawad@chromium.org

BUG=724306
==========

[lpy, 2017-06-15 20:08:55]

lpy@chromium.org changed reviewers:
+ lpy@chromium.org
- lpy@google.com

[matthalp, 2017-06-16 00:01:20]

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

[matthalp, 2017-06-16 00:01:27]

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:38:
// The CoordinationUnitmanagerObserver will only call
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> s/CoordinationUnitmanagerObserver/CoordinationUnitManager ?

Yes. Thank you for catching this.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:44:
CoordinationUnitType filter() const { return filter_; }
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> Instead of exposing filter type, how about using something like
ShouldObserve()? And inside the function, it actually checks the filter type.
> 
> The reason is that if the filter type is invalid, it indicates that we do not
filter anything. When the manager does the check, it is not intuitive that
"invalid" actually means "do not filter".

That is definitely cleaner -- done.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:46:
// Called whenever any new CoordinationUnit of any type is created. Note that
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> This is only called when it matches the filter, right. And if the filter is
kInvalidType, it is called for any type. If we switch to use ShouldObserve() or
something similar, we can just say this function is only called when it should
observe this CU.

Done. Please let me know if the new comments are sufficient.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:30:
CoordinationUnitType filter)
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> If this does not test filter, this class should not have constructor taking in
filter.
> 
> If this is only used for subclass purpose, put it in protected.

Done. CoordinationUnitGraphObserver::filter_ is now a protected member.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:69:
void AddChildListenerInvoked(
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> Should these callbacks be put into protected section?
> 
> And they need to be renamed to On*() style.

I attempted to make the callbacks protected, but the compiler did not like it
(anonymous namespace issues).

I will also rename the callbacks to use the On*() style.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:122:
unsigned int add_child_listener_invocations_;
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> Use size_t for these:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:179:
TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedNoFilters) {
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> nit: s/CallbacksInvokedNoFilters/CallbacksInvokedWithoutFilters

Done.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:182:
TestBasicAPICoordinationUnitGraphObserver* observer =
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> Use smart pointers and pass it to manager with std::move. With that, you do
not even need the comment above because people will understand manager "owns"
observers and will do the corresponding cleanup.
> https://www.chromium.org/developers/smart-pointer-guidelines

Done.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:223:
TestFilterAPICoordinationUnitGraphObserver* observer =
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> ditto: use smart pointer.

Done.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:256:
// Should not invoke the CoordinationUnitCreated listener
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> Combine the comments here to one: Shoudl invoke the CoordinationUnitCreated
listener for frame CU only.

Done.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:267:
frame_coordination_unit.get());
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> The comment says it does not invoke the listener. Why not?

Done -- added comment at the top of this set of function invocations.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:278:
// Should invoke the observer's AddParent listener.
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> Why AddParent? Typo?

This is not a typo, but the comment has been improved to make it more clear:

By calling AddChild on the process CU, the AddParent is called on the frame CU,
whose filter is set to only look for CoordinationUnitType::kProcess parents.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:280:
// Should not invoke the observer's AddParent listener.
On 2017/06/15 at 20:07:52, Zhen Wang wrote:
> Why AddParent? Typo?

See comment above.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:289:
// Should not invoke the observer's RemoveParent listener.
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> Why RemoveParent? Typo?

See comment above.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:291:
// Should invoke the observer's RemoveParent listener.
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> Why RemoveParent? Typo?

See comment above.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:75: }
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> I think you can just use std::vector::insert to append a vector directly
instead of looping.

Thank you for pointing this out -- done.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.cc
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.cc:26:
coordination_unit_manager_.UnregisterAllObserversForTesting();
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> Is there another test helper class that can include the default ones? I am not
sure if we need those for integration test or something similar.

I have gone ahead and removed this function. Instead,
CoordinationUnitManager::Register can be called within the
ResourceCoordinatorService constructor, which owns CoordinationUnitManager
throughout it's lifetime (i.e. the ResourceCoordinatorService's lifetime).

The comment about registering an observer in CoordinationUnitGraphObserver has
been updated.

[commit-bot: I haz the power, 2017-06-16 00:01:34]

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-16 00:09:39]

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

[commit-bot: I haz the power, 2017-06-16 00:09:40]

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

[matthalp, 2017-06-16 01:28:44]

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

[commit-bot: I haz the power, 2017-06-16 01:29: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, 2017-06-16 02:43:30]

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

[commit-bot: I haz the power, 2017-06-16 02:43:32]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-06-16 16:14:25]

I realized I left one comment unaddressed (which now has been!). zhenw@ it's
ready to be reviewed.

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc
(right):

https://codereview.chromium.org/2926663003/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc:42:
coordination_unit_manager_->NotifyObserversCoordinationUnitCreated(
On 2017/06/15 at 20:07:53, Zhen Wang wrote:
> Why not call the notify function in CoordinationUnitImpl's constructor? Does
it require binding setup?

oysteine@ and I discussed whether or not to put the NotifyObservers* in the
constructor or destructor. Additionally, the CoordinationUnitImpls can be
derived so this ensures that any derived constructors can make their
modifications before the observers are called.

[Zhen Wang, 2017-06-16 19:38:20]

Hi Matt,

While reviewing the CL, I noticed one difference between CUGraphObserver and
many other observers in Chromium. The observer pattern in Chromium is usually by
subclassing a base observer class and override the event APIs.
WebContentsObserver is a typical example.

The CUGraphObserver is somewhat different. Currently the only public event API
is OnCoordinationUnitCreated(). And a user needs to register callback functions
in it in order to get the events. This is not very intuitive. For example,
people cannot just skim the public APIs of CUGraphObserver to get an idea of
what events are available.

So I am proposing to follow the general observer pattern most other observers
use. For example:

class CUGraphObserver {
  virtual void OnCUCreated(cu);
  virtual void OnAddChildToCU(cu);
  virtual void OnAddParentToCU(cu);
  ...
}

class XYZObserver : public CUGraphObserver {
  void OnCuCreated(cu) override {
    cu->ObserveAddChildEvent(this, filter);
    cu->ObserveAddParentEvent(this, filter);
  }

  void OnAddChildToCU(cu) {
    // Do whatever.
  }
}

CoordinationUnitImpl {
 public:
  void ObserveAddChildEvent(CUGraphObserver*, filter);
  void ObserveAddParentEvent(CUGraphObserver*, filter);
  ...

 private:
  // Those are raw pointers.
  // The observers are still owned by the manager.
  map<filter, vector<CUGraphObserver*>> on_add_child_event_observers_;
  map<filter, vector<CUGraphObserver*>> on_add_parent_event_observers_;
  ...
}


This requires a lot of modifications to the CL. I am really sorry that I didn't
point this out earlier. I started to notice this when I tried to ask you add
more comments and usage to CUGraphObserver. But then I found it's probably
better to use a different pattern that most Chromium developers are familiar
with.

Do let me know if there are cases where this proposal does not work or there are
problems. :)

[Zhen Wang, 2017-06-16 19:45:53]

> class CUGraphObserver {
>   virtual void OnCUCreated(cu);
>   virtual void OnAddChildToCU(cu);
>   virtual void OnAddParentToCU(cu);
>   ...
> }

And I guess those should actually be:

class CUGraphObserver {
  virtual void OnCUCreated(cu);
  virtual void OnAddChildToCU(cu, child);
  virtual void OnAddParentToCU(cu, parent);
  ...
}

[matthalp, 2017-06-19 01:44:42]

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

[commit-bot: I haz the power, 2017-06-19 01:44:55]

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-19 03:22:40]

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

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

Dry run: This issue passed the CQ dry run.

[ducbui, 2017-06-19 04:09:14]

ducbui@google.com changed reviewers:
+ ducbui@google.com

[ducbui, 2017-06-19 04:09:15]

https://codereview.chromium.org/2926663003/diff/360001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2926663003/diff/360001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:70:
std::map<Filter, std::vector<Listener>> registry_;
Is there any reason for not using std::unordered_map in this version?

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

https://codereview.chromium.org/2926663003/diff/360001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:35:
std::unique_ptr<CoordinationUnitGraphObserver> observers);
Maybe a typo? I think "observers" should be "observer".

[matthalp, 2017-06-19 04:18:11]

matthalp@google.com changed reviewers:
- ducbui@google.com, fmeawad@chromium.org, lpy@chromium.org

[matthalp, 2017-06-19 04:18:11]

matthalp@google.com changed required reviewers:
- oysteine@chromium.org, zhenw@chromium.org

[matthalp, 2017-06-19 04:18:12]

https://codereview.chromium.org/2926663003/diff/360001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2926663003/diff/360001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:70:
std::map<Filter, std::vector<Listener>> registry_;
On 2017/06/19 at 04:09:15, ducbui wrote:
> Is there any reason for not using std::unordered_map in this version?

std::unordered_map requires a std::hash implementation. C++11 does not support
std::hash for the enum classes that will be used as filter (but will be in
C++14). See "2148. Hashing enums should be supported directly by std::hash" in
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2148

The plan is to eventually use std::unordered_map, but I wanted to talk to zhenw
and oysteine to figure out the best way to proceed.

The easiest way is to pass in a hash via an struct-based operator(), which was
used inside of
services/resource_coordinator/public/cpp/coordination_unit_property_types.h for
https://codereview.chromium.org/2940983002.

Given how many places this is useful within services/resource_coordinator/ I
wanted to see if it made sense to have some sort of utils file to be included
where necessary.

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

https://codereview.chromium.org/2926663003/diff/360001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:35:
std::unique_ptr<CoordinationUnitGraphObserver> observers);
On 2017/06/19 at 04:09:15, ducbui wrote:
> Maybe a typo? I think "observers" should be "observer".

Yes -- thank you for catching this.