[GRC] Add Tab-scoped Coordination Unit Graph Observer

[GRC] Add Tab-scoped Coordination Unit Graph Observer

This CL adds a CoordinationUnitGraphObserver scoped to observing activities corresponding to tabs (i.e. WebContents) and enables it to be active within GRC.

A drive-by test name change was performed unrelated to the observer.

R=lpy@chromium.org,zhenw@chromium.org
TBR=oysteine@chromium.org

BUG=691886
Review-Url: https://codereview.chromium.org/2965553002
Cr-Commit-Position: refs/heads/master@{#483878}
Committed: https://chromium.googlesource.com/chromium/src/+/e0717ce2e90081c6d915dee987153f8d5f3a929c

[matthalp, 2017-06-29 17:49:20]

Description was changed from

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

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

to

==========
[GRC] Add Tab-scoped Coordination Unit Graph Observer

This CL adds a CoordinationUnitGraphObserver scoped to observing activities
corresponding to tabs (i.e. WebContents) and enables it to be active within GRC.

A drive-by test name change was performed unrelated to the observer.

R=lpy@chromium.org,zhenw@chromium.org
TBR=oysteine@chromium.org

BUG=691886
==========

[matthalp, 2017-06-29 17:49:21]

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

[matthalp, 2017-06-29 17:49:21]

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

[matthalp, 2017-06-29 17:49:32]

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

[matthalp, 2017-06-29 17:49:40]

PTAL

[commit-bot: I haz the power, 2017-06-29 17:49: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-29 19:16:06]

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

[commit-bot: I haz the power, 2017-06-29 19:16:07]

Dry run: This issue passed the CQ dry run.

[lpy, 2017-06-29 19:51:52]

lpy@chromium.org changed required reviewers:
- lpy@chromium.org, zhenw@chromium.org

[lpy, 2017-06-29 19:51:53]

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
File services/resource_coordinator/resource_coordinator_service.cc (right):

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
services/resource_coordinator/resource_coordinator_service.cc:20:
resource_coordinator_service->coordination_unit_manager()->RegisterObserver(
Should we register observers inside CUManager?

[matthalp, 2017-06-29 19:57:31]

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
File services/resource_coordinator/resource_coordinator_service.cc (right):

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
services/resource_coordinator/resource_coordinator_service.cc:20:
resource_coordinator_service->coordination_unit_manager()->RegisterObserver(
On 2017/06/29 at 19:51:52, lpy wrote:
> Should we register observers inside CUManager?

Ideally, but there is a testability issue. If they are registered within the
CoordinationUnitManager then they will be automatically be registered during
unittests. Realistically, we only want to register observers we want to test.

I'm open to alternative approaches still.

[lpy, 2017-06-29 20:02:30]

On 2017/06/29 19:57:31, matthalp wrote:
>
https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
> File services/resource_coordinator/resource_coordinator_service.cc (right):
> 
>
https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
> services/resource_coordinator/resource_coordinator_service.cc:20:
> resource_coordinator_service->coordination_unit_manager()->RegisterObserver(
> On 2017/06/29 at 19:51:52, lpy wrote:
> > Should we register observers inside CUManager?
> 
> Ideally, but there is a testability issue. If they are registered within the
> CoordinationUnitManager then they will be automatically be registered during
> unittests. Realistically, we only want to register observers we want to test.
> 
> I'm open to alternative approaches still.

I see, let's clean it up in follow-up patches.

lgtm

[Zhen Wang, 2017-06-29 20:06:21]

The test will come with the "[GRC] Tab-level CPU Attribution" CL, right?

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/tab_observer.cc (right):

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/tab_observer.cc:17: // This
observer only tracks tabs.
Maybe remove this comment. This observer only tracks tabs for now. Later, it
will need to track frames to get frame signals. For example, tab done loading
signal is computed from net idleness (from frame) and cpu usage.

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/tab_observer.h (right):

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/tab_observer.h:15: class
TabObserver : public CoordinationUnitGraphObserver {
Need comment to describe what TabObserver does.

And the name "TabObserver" can be misleading. This observer is:
1. observing the whole CU graph (input).
2. producing calculated signals for clients interested at tab scope (output).

The name "TabObserver" indicates the input is scoped to tab only, which is
incorrect. I would suggest dropping "observer" in the name, though it is a
subclass of CUGraphObserver. For example, TabPolicyMaker, TabController,
TabSignalProducer, etc. (just brain dump of some possible names)

[Zhen Wang, 2017-06-29 20:09:19]

cc'ing fmeawad.

[matthalp, 2017-06-29 23:13:56]

Feedback has been addressed. PTAL

I do plan to add unittests for GetAssociated*(), but want to make sure we agree
on this approach first.

zhenw@ I envisioned the tab attribution tests actually coming here (WIP):
https://codereview.chromium.org/2964103002

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/tab_observer.cc (right):

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/tab_observer.cc:17: // This
observer only tracks tabs.
On 2017/06/29 at 20:06:20, Zhen Wang wrote:
> Maybe remove this comment. This observer only tracks tabs for now. Later, it
will need to track frames to get frame signals. For example, tab done loading
signal is computed from net idleness (from frame) and cpu usage.

Done.

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/tab_observer.h (right):

https://codereview.chromium.org/2965553002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/tab_observer.h:15: class
TabObserver : public CoordinationUnitGraphObserver {
On 2017/06/29 at 20:06:21, Zhen Wang wrote:
> Need comment to describe what TabObserver does.
> 
> And the name "TabObserver" can be misleading. This observer is:
> 1. observing the whole CU graph (input).
> 2. producing calculated signals for clients interested at tab scope (output).
> 
> The name "TabObserver" indicates the input is scoped to tab only, which is
incorrect. I would suggest dropping "observer" in the name, though it is a
subclass of CUGraphObserver. For example, TabPolicyMaker, TabController,
TabSignalProducer, etc. (just brain dump of some possible names)

I would advocated TabController since it's consistent with the with GRC
"controller" terminology, which is what observers are meant to be. I changed the
name to "TabController" for now.

[lpy, 2017-06-29 23:31:42]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 23:31:58]

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-30 01:07:11]

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

[commit-bot: I haz the power, 2017-06-30 01:07:12]

Dry run: This issue passed the CQ dry run.

[Zhen Wang, 2017-06-30 16:34:28]

Overall looks good. Let's finalize the class name together.

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

https://codereview.chromium.org/2965553002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/tab_controller.h:15: class
TabController : public CoordinationUnitGraphObserver {
Let's also get lpy's opinion on the class name here. And then add comments.

[Zhen Wang, 2017-06-30 20:22:00]

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

https://codereview.chromium.org/2965553002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/tab_controller.h:15: class
TabController : public CoordinationUnitGraphObserver {
On 2017/06/30 16:34:28, Zhen Wang wrote:
> Let's also get lpy's opinion on the class name here. And then add comments.

Another proposal: TabSignalSynthesizer or TabSignalProcessor (coming from DSP)

[lpy, 2017-06-30 20:22:33]

On 2017/06/30 20:22:00, Zhen Wang wrote:
>
https://codereview.chromium.org/2965553002/diff/20001/services/resource_coord...
> File services/resource_coordinator/coordination_unit/tab_controller.h (right):
> 
>
https://codereview.chromium.org/2965553002/diff/20001/services/resource_coord...
> services/resource_coordinator/coordination_unit/tab_controller.h:15: class
> TabController : public CoordinationUnitGraphObserver {
> On 2017/06/30 16:34:28, Zhen Wang wrote:
> > Let's also get lpy's opinion on the class name here. And then add comments.
> 
> Another proposal: TabSignalSynthesizer or TabSignalProcessor (coming from DSP)

TabSignalCollector

[matthalp, 2017-06-30 22:14:33]

The observer name has been updated -- PTAL.

[Zhen Wang, 2017-06-30 22:53:52]

Thanks! LGTM

[matthalp, 2017-06-30 23:00:10]

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

[commit-bot: I haz the power, 2017-06-30 23:00:26]

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-07-01 00:19:14]

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

[commit-bot: I haz the power, 2017-07-01 00:19:15]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-07-01 00:44:31]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-07-01 00:44:31]

The patchset sent to the CQ was uploaded after l-g-t-m from lpy@chromium.org
Link to the patchset: https://codereview.chromium.org/2965553002/#ps40001
(title: "Change observer name")

[commit-bot: I haz the power, 2017-07-01 00:44:53]

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-07-01 00:53:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498869871179020,
"parent_rev": "30815d143e71eb731bea80ba5fbd236d628ee9f3", "commit_rev":
"e0717ce2e90081c6d915dee987153f8d5f3a929c"}

[commit-bot: I haz the power, 2017-07-01 00:53:25]

Description was changed from

==========
[GRC] Add Tab-scoped Coordination Unit Graph Observer

This CL adds a CoordinationUnitGraphObserver scoped to observing activities
corresponding to tabs (i.e. WebContents) and enables it to be active within GRC.

A drive-by test name change was performed unrelated to the observer.

R=lpy@chromium.org,zhenw@chromium.org
TBR=oysteine@chromium.org

BUG=691886
==========

to

==========
[GRC] Add Tab-scoped Coordination Unit Graph Observer

This CL adds a CoordinationUnitGraphObserver scoped to observing activities
corresponding to tabs (i.e. WebContents) and enables it to be active within GRC.

A drive-by test name change was performed unrelated to the observer.

R=lpy@chromium.org,zhenw@chromium.org
TBR=oysteine@chromium.org

BUG=691886

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

[commit-bot: I haz the power, 2017-07-01 00:53:27]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e0717ce2e90081c6d915dee98715...