GRC: Hook up the frame-level CoordinationUnit

GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer process on request.

It's created on demand, and currently won't be instantiated without the GlobalResourceCoordinator feature flag.

An example of full GRC usage can be seen in https://codereview.chromium.org/2710823003

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2913253002
Cr-Commit-Position: refs/heads/master@{#477157}
Committed: https://chromium.googlesource.com/chromium/src/+/2b2a1ee8bdcc6ff7e3d31e613964bd883141a5d8

[oystein (OOO til 10th of July), 2017-05-31 18:59:06]

Description was changed from

==========
GRC: Hook up the frame-level CoordinationUnit in the browser and make it
available to the renderer process

R=nasko@chromium.org
BUG=691886
==========

to

==========
GRC: Hook up the frame-level CoordinationUnit in the browser and make it
available to the renderer process

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[oystein (OOO til 10th of July), 2017-05-31 20:33:05]

Description was changed from

==========
GRC: Hook up the frame-level CoordinationUnit in the browser and make it
available to the renderer process

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer
process on request.

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[oystein (OOO til 10th of July), 2017-05-31 20:33:21]

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

[commit-bot: I haz the power, 2017-05-31 20:34:37]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-31 20:53:01]

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

[commit-bot: I haz the power, 2017-05-31 20:53:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[oystein (OOO til 10th of July), 2017-05-31 23:47:14]

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

[commit-bot: I haz the power, 2017-05-31 23:48:05]

Dry run: CQ is trying da patch.

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

[oystein (OOO til 10th of July), 2017-06-01 00:23:36]

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

[commit-bot: I haz the power, 2017-06-01 00:24:29]

Dry run: CQ is trying da patch.

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

[oystein (OOO til 10th of July), 2017-06-01 00:25:04]

Description was changed from

==========
GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer
process on request.

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer
process on request.

It's created on demand, and currently won't be instantiated without the
GlobalResourceCoordinator feature flag.

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[oystein (OOO til 10th of July), 2017-06-01 00:26:27]

nasko: ptal; I couldn't find a good alternative to going through the
content/public API here, since I'd like as much of the GRC hookup functionality
to live in the ResourceCoordinatorWebContentsObserver so we can gate it all
together under one flag.

[oystein (OOO til 10th of July), 2017-06-01 00:29:33]

Description was changed from

==========
GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer
process on request.

It's created on demand, and currently won't be instantiated without the
GlobalResourceCoordinator feature flag.

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer
process on request.

It's created on demand, and currently won't be instantiated without the
GlobalResourceCoordinator feature flag.

An example of full GRC usage can be seen in
https://codereview.chromium.org/2710823003

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-06-01 03:34:21]

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

[commit-bot: I haz the power, 2017-06-01 03:34:22]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[oystein (OOO til 10th of July), 2017-06-02 20:49:24]

On 2017/06/01 at 00:26:27, oystein wrote:
> nasko: ptal; I couldn't find a good alternative to going through the
content/public API here, since I'd like as much of the GRC hookup functionality
to live in the ResourceCoordinatorWebContentsObserver so we can gate it all
together under one flag.

Gentle ping; the CL dependent on this one
(https://codereview.chromium.org/2914243003/) is ready to land.

[nasko, 2017-06-02 23:29:16]

https://codereview.chromium.org/2913253002/diff/40001/chrome/browser/resource...
File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):

https://codereview.chromium.org/2913253002/diff/40001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:47:
resource_coordinator::EventType::kOnNavigationCommit);
The frame hasn't committed yet, so it seems premature to send a commit event
here. You are better off doing it in DidFinishNavigation and ensuring that it
did actually successful commit a document.

In addition, this fires on each navigation, even though the RenderFrameHost
stays the same, so you will be adding duplicate child nodes.

https://codereview.chromium.org/2913253002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2913253002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:274:
render_frame_host->GetFrameResourceCoordinator()->service()->Duplicate(
This code looks strange as it is written. Creating a new coordinator interface
through a Duplicate method just doesn't read well. Also, the Duplicate method on
the CoordinationUnit interface doesn't have any comment about what it does.

[oystein (OOO til 10th of July), 2017-06-05 20:02:40]

Thanks!

https://codereview.chromium.org/2913253002/diff/40001/chrome/browser/resource...
File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):

https://codereview.chromium.org/2913253002/diff/40001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:47:
resource_coordinator::EventType::kOnNavigationCommit);
On 2017/06/02 at 23:29:16, nasko (slow) wrote:
> The frame hasn't committed yet, so it seems premature to send a commit event
here. You are better off doing it in DidFinishNavigation and ensuring that it
did actually successful commit a document.
> 
> In addition, this fires on each navigation, even though the RenderFrameHost
stays the same, so you will be adding duplicate child nodes.

Changed to DidFinishNavigation w/ error checks and removed the
kOnNavigationCommit event since it's not being used yet regardless, the thing we
need right now is just the parent/child relationship. CoordinationUnit::AddChild
will return early if there's an existing parent/child relationship, so firing
this off multiple times for the same RenderFrameHost shouldn't be an issue.

https://codereview.chromium.org/2913253002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2913253002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:274:
render_frame_host->GetFrameResourceCoordinator()->service()->Duplicate(
On 2017/06/02 at 23:29:16, nasko (slow) wrote:
> This code looks strange as it is written. Creating a new coordinator interface
through a Duplicate method just doesn't read well. Also, the Duplicate method on
the CoordinationUnit interface doesn't have any comment about what it does.

Renamed to AddBinding and added a comment to better reflect what this does.

[oystein (OOO til 10th of July), 2017-06-05 20:02:43]

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

[commit-bot: I haz the power, 2017-06-05 20:03:25]

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-05 20:06:20]

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

[commit-bot: I haz the power, 2017-06-05 20:06:21]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[nasko, 2017-06-05 23:56:51]

LGTM, assuming the comment for IsSameDocument is addressed either way.

https://codereview.chromium.org/2913253002/diff/80001/chrome/browser/resource...
File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):

https://codereview.chromium.org/2913253002/diff/80001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:43:
if (!navigation_handle->HasCommitted() || navigation_handle->IsErrorPage()) {
I think you will likely also want to check navigation_handle->IsSameDocument(),
which when true means we didn't change the document, but only did a fragment
navigation or a pushState type one.

https://codereview.chromium.org/2913253002/diff/80001/services/resource_coord...
File services/resource_coordinator/public/interfaces/coordination_unit.mojom
(right):

https://codereview.chromium.org/2913253002/diff/80001/services/resource_coord...
services/resource_coordinator/public/interfaces/coordination_unit.mojom:41:
AddBinding(CoordinationUnit& request);
nit: An empty line after AddBinding makes it a bit more readable and existed in
the original code.

[oystein (OOO til 10th of July), 2017-06-06 00:02:47]

Thanks!

https://codereview.chromium.org/2913253002/diff/80001/chrome/browser/resource...
File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):

https://codereview.chromium.org/2913253002/diff/80001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:43:
if (!navigation_handle->HasCommitted() || navigation_handle->IsErrorPage()) {
On 2017/06/05 at 23:56:51, nasko (slow) wrote:
> I think you will likely also want to check
navigation_handle->IsSameDocument(), which when true means we didn't change the
document, but only did a fragment navigation or a pushState type one.

Done

https://codereview.chromium.org/2913253002/diff/80001/services/resource_coord...
File services/resource_coordinator/public/interfaces/coordination_unit.mojom
(right):

https://codereview.chromium.org/2913253002/diff/80001/services/resource_coord...
services/resource_coordinator/public/interfaces/coordination_unit.mojom:41:
AddBinding(CoordinationUnit& request);
On 2017/06/05 at 23:56:51, nasko (slow) wrote:
> nit: An empty line after AddBinding makes it a bit more readable and existed
in the original code.

Done.

[oystein (OOO til 10th of July), 2017-06-06 00:02:51]

The CQ bit was checked by oysteine@chromium.org

[oystein (OOO til 10th of July), 2017-06-06 00:02:51]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2913253002/#ps100001
(title: "Review fixes and rebase")

[commit-bot: I haz the power, 2017-06-06 00:03:48]

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-06 02:29:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496707371032280,
"parent_rev": "8352a837598c133cbba5343cf38d3ccb8f830481", "commit_rev":
"2b2a1ee8bdcc6ff7e3d31e613964bd883141a5d8"}

[commit-bot: I haz the power, 2017-06-06 02:30:03]

Description was changed from

==========
GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer
process on request.

It's created on demand, and currently won't be instantiated without the
GlobalResourceCoordinator feature flag.

An example of full GRC usage can be seen in
https://codereview.chromium.org/2710823003

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
GRC: Hook up the frame-level CoordinationUnit

This gets hooked up in the browser process and made available to the renderer
process on request.

It's created on demand, and currently won't be instantiated without the
GlobalResourceCoordinator feature flag.

An example of full GRC usage can be seen in
https://codereview.chromium.org/2710823003

R=nasko@chromium.org
BUG=691886
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2913253002
Cr-Commit-Position: refs/heads/master@{#477157}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b2a1ee8bdcc6ff7e3d31e613964...
==========

[commit-bot: I haz the power, 2017-06-06 02:30:04]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2b2a1ee8bdcc6ff7e3d31e613964...