Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2942403002:
[GRC] Coordination Unit Graph Observer (Closed)
Description[GRC] Coordination Unit Graph Observer
The CoordinationUnitGraphObserver framework provides an API to observe and respond to to interesting events that occur throughout coordination unit lifetimes’ within the resource_coordinator service.
R=*lpy@chromium.org,*oysteine@chromium.org,zhenw@chromium.org
BUG=724306
Review-Url: https://codereview.chromium.org/2942403002
Cr-Commit-Position: refs/heads/master@{#481076}
Committed: https://chromium.googlesource.com/chromium/src/+/a2c1ff6a5f04699131e02568187ad9e68b7738f0
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Change API names #Patch Set 4 : Rename API function calls #
Total comments: 74
Patch Set 5 : Address reviewer feedback #
Total comments: 1
Patch Set 6 : Fix unittest and OnCoordinationUnitWillBeDestroyed #
Total comments: 1
Patch Set 7 : Rebase #Patch Set 8 : Rebase #
Total comments: 1
Messages
Total messages: 56 (39 generated)
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Initialize CL BUG= ========== to ========== [GRC] Coordination Unit Graph Observer V2 This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: TODO R=*oysteine@chromium.org,*zhenw@chromium.org,lpy@google.com,fmeawad@chromium.org BUG=724306 ==========
matthalp@google.com changed reviewers: + oysteine@chromium.org, zhenw@chromium.org
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Description was changed from ========== [GRC] Coordination Unit Graph Observer V2 This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: TODO R=*oysteine@chromium.org,*zhenw@chromium.org,lpy@google.com,fmeawad@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Graph Observer V2 This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: TODO R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ==========
Description was changed from ========== [GRC] Coordination Unit Graph Observer V2 This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: TODO R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Graph Observer V2 This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ==========
Description was changed from ========== [GRC] Coordination Unit Graph Observer V2 This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Graph Observer: Traditional Observer Implementation This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ==========
Description was changed from ========== [GRC] Coordination Unit Graph Observer: Traditional Observer Implementation This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Graph Observer: Traditional Observer Implementation This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ==========
zhenw@ PTAL
Thanks for putting up this CL so quickly! Overall looks good. I left some comments below. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:19: // An observer API for the CoordinationUnitGraph maintained by GRC. nit: CoordinationUnitGraph is not a real class. So no need to use camel case. Just use "coordination unit graph". https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:33: // This document needs to be updated because the life of an observer can be shorter now. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:36: CoordinationUnitGraphObserver(); It is probably better to also have some comments for the default constructor to indicate that the default one does not filter anything. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:37: // The CoordinationUnitGraphObserver will only call s/CoordinationUnitGraphObserver/CoordinationUnitManager ? Actually, Maybe it is better not mention CUManager here because now the observer can be created by some other objects. Just say "CUCreated is called when ..." https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:44: // invoked on the newly created CoordinationUnit. Nit: This comment is too specific to OnCoordinationUnitCreated. Maybe add a general description at the beginning, something like: Determines whether or not this observer should observe the coordination unit. This will affect whether or not OnCoordinationUnitCreated should be invoked on the newly created CoordinationUnit. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:56: virtual void OnChildAddedEvent( I think we can remove "Event" in the API to be concise, so just |OnChildAdded|. Same for other ones. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:97: CoordinationUnitType filter_; Why does this need to be protected? I did a quick search and didn't find any subclass using it directly. Even if some subclass wants to use it, it is better to put it in private and have a protected filter() function. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:156: std::unique_ptr<TestBasicAPICoordinationUnitGraphObserver>(observer)); This is for line 150-156. In general using raw pointers are discouraged. I see you need this to check the number of invocations later. The usual way is directly creating the smart pointer and transfer ownership to manager. Later you can have a CUManager::GetObservers() function to get back the observers and test them. For example: coordination_unit_manager()->RegisterObserver(base::MakeUnique<...>()); ... auto observers = coordination_unit_manager()->GetObservers(); EXPECT_EQ(2u, observers[0]->on_xyz()); https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:187: EXPECT_EQ(2u, observer->on_coordination_unit_created_invocations()); nit: it probably helps readability to do this check first. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:194: TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedWithoutFilters) { Do you mean "With" filters here? And "WithoutFilters" for the above test? https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:195: TestFilterAPICoordinationUnitGraphObserver* observer = ditto. Do not use raw pointers here. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:256: // Note that AddParent is called within Add child. s/within Add child/as a consequence of AddChild. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:295: EXPECT_EQ(2u, observer->on_coordination_unit_created_invocations()); nit: it probably helps readability to do this check first. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:227: // after RecalcCoordinationPolicy? Is this undecided or you intend to ask Oystein for this CL? https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:32: // Collection to manage CoordinationUnitEvent listeners. s/CoordinationUnitEvent listeners/CoordinatorUnitGraphObservers https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:34: class CoordinationUnitGraphObserverRegistry { Remember to add RemoveObserver API. (This is just a reminder to myself for our discussion offline). https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:46: void AddObserver(CoordinationUnitGraphObserver* observer) { Comments needed for the above two functions. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:61: // Get listeners registered without a filter after. I think it should be s/without/with here? If you get the comment here correctly, the comment for the above |AppendObservers| on line 58 is not needed. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:69: std::vector<CoordinationUnitGraphObserver*> GetObserversWithoutAFilter() { nit: s/GetObserversWithoutAFilter/GetObserversWithoutFilter (removing "A"). https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:126: void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer, Can you also add some comments for those APIs. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:128: void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer); I forget if using default arguments are allowed in chromium style, which would lead to cleaner code here. Double check with Oystein. If not allowed, I suggest separating those with blank lines for better readability. For example: void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer); void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer, CoordinationUnitType child_filter); void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer); void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer, CoordinationUnitType child_filter); void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer); void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer, CoordinationUnitType parent_filter); ...
lpy@chromium.org changed reviewers: + lpy@chromium.org
https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:45: bool ShouldObserve(CoordinationUnitImpl* coordination_unit); defer the functionality to CUGraphObserver itself in OnCoordinationUnitCreated. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:49: virtual void OnCoordinationUnitCreatedEvent( OnCoordinationUnitCreated https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:56: virtual void OnChildAddedEvent( OnChildAdded https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:64: virtual void OnParentAddedEvent( OnParentAdded. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:72: virtual void OnPropertyChangedEvent( OnPropertyChanged Will it be useful to also pass the original value? https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:80: virtual void OnChildRemovedEvent( OnChildRemoved https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:88: virtual void OnParentRemovedEvent( OnParentRemoved https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:93: virtual void OnWillBeDestroyedEvent( OnWillBeDestroyed https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:34: class CoordinationUnitGraphObserverRegistry { Let's leave the filtering to each CUGraphObserver implementation. An observed CU should do nothing but forward changes to the CUGraphObserver that is observing it, and it's the CUGraphObserver's responsibility to select whatever messages it needs. A CU can then only hold a list of observers that are observing the graph it belongs. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_manager.cc (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:7: #include <memory> no need. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:50: coordination_unit->WillBeDestroyed(); Should we also notify observers here? https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_manager.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.h:37: void NotifyObserversCoordinationUnitCreated( OnCoordinationUnitCreated https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.h:39: void NotifyObserversCoordinationUnitWillBeDestroyed( OnCoordinationUnitWillBeDestroyed
nduca@chromium.org changed reviewers: + nduca@chromium.org
Note zhenw is ooo this week, so let us incoporate zhen's feedback but then wait only for lg's from oysteine + lpy and then land when we have those. We can always incorporate zhen's feedback afterward.
On 2017/06/20 17:20:29, nduca wrote: > Note zhenw is ooo this week, so let us incoporate zhen's feedback but then wait > only for lg's from oysteine + lpy and then land when we have those. We can > always incorporate zhen's feedback afterward. That sounds good to me. I will also give reviews whenever I can.
https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:21: // Observers are instantiated when the resource_coordinator serivce nit: s/serivce/service/ https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:227: // after RecalcCoordinationPolicy? On 2017/06/19 at 22:54:25, Zhen Wang wrote: > Is this undecided or you intend to ask Oystein for this CL? Recalc should happen after, though TBH I think we'll end up refactoring the whole RecalcCoordinationPolicy thing and replace it with observers, it's essentially a hack of the same functionality (X event happens, triggering some policy update on a callback). https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:34: class CoordinationUnitGraphObserverRegistry { On 2017/06/19 at 23:21:38, lpy wrote: > Let's leave the filtering to each CUGraphObserver implementation. An observed CU should do nothing but forward changes to the CUGraphObserver that is observing it, and it's the CUGraphObserver's responsibility to select whatever messages it needs. A CU can then only hold a list of observers that are observing the graph it belongs. Agreed. I think it's better to defer the per-event granularity until there's a real need for it for performance reasons, which should simplify this CL a lot (i.e. a single list, and no filters). i.e. if (observer->ShouldObserve(cu) { cu->AddObserver) } https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:128: void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer); On 2017/06/19 at 22:54:25, Zhen Wang wrote: > I forget if using default arguments are allowed in chromium style, which would lead to cleaner code here. Double check with Oystein. > > If not allowed, I suggest separating those with blank lines for better readability. For example: > > void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer); > void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer, > CoordinationUnitType child_filter); > > void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer); > void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer, > CoordinationUnitType child_filter); > > void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer); > void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer, > CoordinationUnitType parent_filter); > > ... AFAIK default arguments are allowed on non-virtual functions, but I don't see them used a lot. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h:29: return &coordination_unit_manager_; Just return a ref? https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_manager.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.h:24: // aspects of Coordination Units within the ResourceCoordinatorService. nit: resource_coordinator service.
Reviewer feedback from lpy, oysteine, and zhenw have all been addressed. However, the changes were made and moved to clean CL (temporary) in case some of the changes are too extreme. Once there is high-level agreement I will move them back here. (Temporary) clean CL link: https://codereview.chromium.org/2953483002 https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:19: // An observer API for the CoordinationUnitGraph maintained by GRC. On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > nit: CoordinationUnitGraph is not a real class. So no need to use camel case. Just use "coordination unit graph". Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:21: // Observers are instantiated when the resource_coordinator serivce On 2017/06/20 at 18:39:23, oystein wrote: > nit: s/serivce/service/ Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:33: // On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > This document needs to be updated because the life of an observer can be shorter now. The observer lifetime behavior has returned to being bound to the lifetime of GRC for now -- this no longer needs to be addressed. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:36: CoordinationUnitGraphObserver(); On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > It is probably better to also have some comments for the default constructor to indicate that the default one does not filter anything. Filtering has been removed for now -- this no longer needs to be addressed. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:37: // The CoordinationUnitGraphObserver will only call On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > s/CoordinationUnitGraphObserver/CoordinationUnitManager ? > > Actually, Maybe it is better not mention CUManager here because now the observer can be created by some other objects. Just say "CUCreated is called when ..." Filtering has been removed for now -- this no longer needs to be addressed. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:44: // invoked on the newly created CoordinationUnit. On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > Nit: This comment is too specific to OnCoordinationUnitCreated. Maybe add a general description at the beginning, something like: > > Determines whether or not this observer should observe the coordination unit. This will affect whether or not OnCoordinationUnitCreated should be invoked on the newly created CoordinationUnit. Filtering has been removed for now -- this no longer needs to be addressed. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:45: bool ShouldObserve(CoordinationUnitImpl* coordination_unit); On 2017/06/19 at 23:21:38, lpy wrote: > defer the functionality to CUGraphObserver itself in OnCoordinationUnitCreated. Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:49: virtual void OnCoordinationUnitCreatedEvent( On 2017/06/19 at 23:21:38, lpy wrote: > OnCoordinationUnitCreated Done. Event has been removed from On*Event. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:56: virtual void OnChildAddedEvent( On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > I think we can remove "Event" in the API to be concise, so just |OnChildAdded|. Same for other ones. Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:64: virtual void OnParentAddedEvent( On 2017/06/19 at 23:21:38, lpy wrote: > OnParentAdded. Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:72: virtual void OnPropertyChangedEvent( On 2017/06/19 at 23:21:38, lpy wrote: > OnPropertyChanged > > Will it be useful to also pass the original value? If it is determined to be useful for someone we can added it. I am willing to take on the yak shaving for that. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:80: virtual void OnChildRemovedEvent( On 2017/06/19 at 23:21:38, lpy wrote: > OnChildRemoved Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:88: virtual void OnParentRemovedEvent( On 2017/06/19 at 23:21:38, lpy wrote: > OnParentRemoved Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:93: virtual void OnWillBeDestroyedEvent( On 2017/06/19 at 23:21:38, lpy wrote: > OnWillBeDestroyed Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h:97: CoordinationUnitType filter_; On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > Why does this need to be protected? I did a quick search and didn't find any subclass using it directly. Even if some subclass wants to use it, it is better to put it in private and have a protected filter() function. Filter has been removed -- this is no longer applicable. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:156: std::unique_ptr<TestBasicAPICoordinationUnitGraphObserver>(observer)); On 2017/06/19 at 22:54:25, Zhen Wang (OOO until Jun 27) wrote: > This is for line 150-156. > > In general using raw pointers are discouraged. I see you need this to check the number of invocations later. The usual way is directly creating the smart pointer and transfer ownership to manager. Later you can have a CUManager::GetObservers() function to get back the observers and test them. > > For example: > coordination_unit_manager()->RegisterObserver(base::MakeUnique<...>()); > ... > auto observers = coordination_unit_manager()->GetObservers(); > EXPECT_EQ(2u, observers[0]->on_xyz()); Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:187: EXPECT_EQ(2u, observer->on_coordination_unit_created_invocations()); On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > nit: it probably helps readability to do this check first. Good suggestion. I moved the EXPECT_EQ directly after the code that should trigger the change being checked as well. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:194: TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedWithoutFilters) { On 2017/06/19 at 22:54:25, Zhen Wang (OOO until Jun 27) wrote: > Do you mean "With" filters here? And "WithoutFilters" for the above test? Filtering has been removed -- this is no longer applicable. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:195: TestFilterAPICoordinationUnitGraphObserver* observer = On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > ditto. Do not use raw pointers here. Filtering has been removed -- this is no longer applicable. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:256: // Note that AddParent is called within Add child. On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > s/within Add child/as a consequence of AddChild. Filtering has been removed -- this is no longer applicable. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:295: EXPECT_EQ(2u, observer->on_coordination_unit_created_invocations()); On 2017/06/19 at 22:54:24, Zhen Wang (OOO until Jun 27) wrote: > nit: it probably helps readability to do this check first. Filtering has been removed -- this is no longer applicable. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:227: // after RecalcCoordinationPolicy? On 2017/06/19 at 22:54:25, Zhen Wang (OOO until Jun 27) wrote: > Is this undecided or you intend to ask Oystein for this CL? RecalcCoordinationPolicy does not do anything right now so it can be addressed here or at a later point. +oysteine https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:227: // after RecalcCoordinationPolicy? On 2017/06/20 at 18:39:23, oystein wrote: > On 2017/06/19 at 22:54:25, Zhen Wang wrote: > > Is this undecided or you intend to ask Oystein for this CL? > > Recalc should happen after, though TBH I think we'll end up refactoring the whole RecalcCoordinationPolicy thing and replace it with observers, it's essentially a hack of the same functionality (X event happens, triggering some policy update on a callback). Sounds good. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:32: // Collection to manage CoordinationUnitEvent listeners. On 2017/06/19 at 22:54:25, Zhen Wang wrote: > s/CoordinationUnitEvent listeners/CoordinatorUnitGraphObservers Removed CoordinationGraphObserverRegistry -- no longer needs to be addressed. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:34: class CoordinationUnitGraphObserverRegistry { On 2017/06/19 at 22:54:29, Zhen Wang wrote: > Remember to add RemoveObserver API. (This is just a reminder to myself for our discussion offline). Removed CoordinationGraphObserverRegistry -- no longer needs to be addressed. However, CoordinationUnitImpl::RemoveObserver has been added. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:46: void AddObserver(CoordinationUnitGraphObserver* observer) { On 2017/06/19 at 22:54:28, Zhen Wang wrote: > Comments needed for the above two functions. Removed CoordinationGraphObserverRegistry -- no longer needs to be addressed. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:126: void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer, On 2017/06/19 at 22:54:26, Zhen Wang (OOO until Jun 27) wrote: > Can you also add some comments for those APIs. Filter has been removed -- no long applicable. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:128: void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer); On 2017/06/20 at 18:39:23, oystein wrote: > On 2017/06/19 at 22:54:25, Zhen Wang wrote: > > I forget if using default arguments are allowed in chromium style, which would lead to cleaner code here. Double check with Oystein. > > > > If not allowed, I suggest separating those with blank lines for better readability. For example: > > > > void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer); > > void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer, > > CoordinationUnitType child_filter); > > > > void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer); > > void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer, > > CoordinationUnitType child_filter); > > > > void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer); > > void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer, > > CoordinationUnitType parent_filter); > > > > ... > > AFAIK default arguments are allowed on non-virtual functions, but I don't see them used a lot. Filter has been removed -- no long applicable. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h:29: return &coordination_unit_manager_; On 2017/06/20 at 18:39:24, oystein wrote: > Just return a ref? Agreed -- Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_manager.cc (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:7: #include <memory> On 2017/06/19 at 23:21:39, lpy wrote: > no need. Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:50: coordination_unit->WillBeDestroyed(); On 2017/06/19 at 23:21:39, lpy wrote: > Should we also notify observers here? Done. I agree that removes a little bit of code complexity from CoordinationUnitImpl. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_manager.h (right): https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.h:24: // aspects of Coordination Units within the ResourceCoordinatorService. On 2017/06/20 at 18:39:24, oystein wrote: > nit: resource_coordinator service. Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.h:37: void NotifyObserversCoordinationUnitCreated( On 2017/06/19 at 23:21:39, lpy wrote: > OnCoordinationUnitCreated Done. https://codereview.chromium.org/2942403002/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.h:39: void NotifyObserversCoordinationUnitWillBeDestroyed( On 2017/06/19 at 23:21:39, lpy wrote: > OnCoordinationUnitWillBeDestroyed Done. Agree that this is more consistent naming with OnCoordinationUnitCreated.
https://codereview.chromium.org/2942403002/diff/80001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_manager.cc (right): https://codereview.chromium.org/2942403002/diff/80001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:52: observer->OnCoordinationUnitWillBeDestroyed(coordination_unit); I advocate moving the OnCoordinationUnitWillBeDestroyed logic back into the CoordinationUnit. Doing the if (observer->ShouldObserver(coordinationUnit) check here requires that ShouldObserve is pure in that it gives the same output every time that coordination_unit instance is passed into it. While I think that most of that code will be that way, there is no guarantee for that to be the case. For example an observer could decide to do bool ShouldObserver(...) { return tracked_coordination_units_ < 5; } void CoordinationUnitCreated(...) { ++tracked_coordination_units_; } In this case, if a CU was one of the first five CUs created it would be tracked, but if more than five were created when this CU is destroyed then OnCoordinationUnitWillBeDestroyed would not be invoked. I know this is a pathological example, but you never know.
On 2017/06/20 at 21:02:10, matthalp wrote: > https://codereview.chromium.org/2942403002/diff/80001/services/resource_coord... > File services/resource_coordinator/coordination_unit/coordination_unit_manager.cc (right): > > https://codereview.chromium.org/2942403002/diff/80001/services/resource_coord... > services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:52: observer->OnCoordinationUnitWillBeDestroyed(coordination_unit); > I advocate moving the OnCoordinationUnitWillBeDestroyed logic back into the CoordinationUnit. Doing the if (observer->ShouldObserver(coordinationUnit) check here requires that ShouldObserve is pure in that it gives the same output every time that coordination_unit instance is passed into it. > > While I think that most of that code will be that way, there is no guarantee for that to be the case. For example an observer could decide to do > > bool ShouldObserver(...) { > return tracked_coordination_units_ < 5; > } > > void CoordinationUnitCreated(...) { > ++tracked_coordination_units_; > } > > In this case, if a CU was one of the first five CUs created it would be tracked, but if more than five were created when this CU is destroyed then OnCoordinationUnitWillBeDestroyed would not be invoked. > > I know this is a pathological example, but you never know. oysteine, lpy Sorry for the back-to-back uploads. This is now ready for review. PTAL.
This loos great, lgtm from my side. Thanks for doing this and sorry for the back and forth
Great, thanks! :) lgtm with a nit. https://codereview.chromium.org/2942403002/diff/100001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_manager.h (right): https://codereview.chromium.org/2942403002/diff/100001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_manager.h:40: std::vector<std::unique_ptr<CoordinationUnitGraphObserver>>& observers() { nit: looks this is just for testing? maybe const std::vector<blah>& observers_for_testing()
Awesome work Matt!
nduca@chromium.org changed reviewers: - nduca@chromium.org
Description was changed from ========== [GRC] Coordination Unit Graph Observer: Traditional Observer Implementation This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Graph Observer This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ==========
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by matthalp@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by matthalp@google.com to run a CQ dry run
The CQ bit was unchecked by matthalp@google.com
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [GRC] Coordination Unit Graph Observer This is alternative API implementation for the CoordinationUnitGraphObserver framework. Its implementation is more in line with the traditional observer model used throughout the Chromium code base, in contrast to the callback approach used in the initial API proposal (https://codereview.chromium.org/2926663003). Example Usage: https://codereview.chromium.org/2946683002 R=oysteine@chromium.org,zhenw@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Graph Observer The CoordinationUnitGraphObserver framework provides an API to observe and respond to to interesting events that occur throughout coordination unit lifetimes’ within the resource_coordinator service. R=*lpy@chromium.org,*oysteine@chromium.org,zhenw@chromium.org BUG=724306 ==========
matthalp@google.com changed required reviewers: + lpy@chromium.org, oysteine@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by matthalp@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, lpy@chromium.org Link to the patchset: https://codereview.chromium.org/2942403002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498008821404120,
"parent_rev": "24c9f587451ae73f0a3d826dccb362913fb2ddc6", "commit_rev":
"a2c1ff6a5f04699131e02568187ad9e68b7738f0"}
Message was sent while issue was closed.
Description was changed from ========== [GRC] Coordination Unit Graph Observer The CoordinationUnitGraphObserver framework provides an API to observe and respond to to interesting events that occur throughout coordination unit lifetimes’ within the resource_coordinator service. R=*lpy@chromium.org,*oysteine@chromium.org,zhenw@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Graph Observer The CoordinationUnitGraphObserver framework provides an API to observe and respond to to interesting events that occur throughout coordination unit lifetimes’ within the resource_coordinator service. R=*lpy@chromium.org,*oysteine@chromium.org,zhenw@chromium.org BUG=724306 Review-Url: https://codereview.chromium.org/2942403002 Cr-Commit-Position: refs/heads/master@{#481076} Committed: https://chromium.googlesource.com/chromium/src/+/a2c1ff6a5f04699131e02568187a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a2c1ff6a5f04699131e02568187a...
Message was sent while issue was closed.
Thanks for the hard work! Late LGTM with one nit, which can be cleaned up in a follow-up CL. https://codereview.chromium.org/2942403002/diff/140001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc (right): https://codereview.chromium.org/2942403002/diff/140001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:100: TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedNoFilters) { nit: CallbacksInvokedNoFilters/CallbacksInvoked There is no concept of "filter" any more.
Message was sent while issue was closed.
On 2017/06/21 at 03:48:36, zhenw wrote: > Thanks for the hard work! Late LGTM with one nit, which can be cleaned up in a follow-up CL. > > https://codereview.chromium.org/2942403002/diff/140001/services/resource_coor... > File services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc (right): > > https://codereview.chromium.org/2942403002/diff/140001/services/resource_coor... > services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc:100: TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedNoFilters) { > nit: CallbacksInvokedNoFilters/CallbacksInvoked > > There is no concept of "filter" any more. Thank you for catching that. I will fix it in my TabCPUUsage CL. |
