Enable GRC

Enable GRC

(landing for zhenw@; original CL: https://codereview.chromium.org/2943563002)

This CL enables GlobalResourceCoordinator by default. Several projects are
depending on GRC service. For features using GRC, they should be guarded
by their own feature flags and run their own finch trials.

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

[matthalp, 2017-06-28 17:25:30]

Description was changed from

==========
Add ServiceManager check


Enable GRC

This CL enables GlobalResourceCoordinator by default. Several projects are
depending on GRC service. For features using GRC, they should be guarded
by their own feature flags and run their own finch trials.

BUG=691886

patch from issue 2943563002 at patchset 1 (http://crrev.com/2943563002#ps1)
==========

to

==========
Enable GRC

(landing for zhenw@; original CL: https://codereview.chromium.org/2943563002)

This CL enables GlobalResourceCoordinator by default. Several projects are
depending on GRC service. For features using GRC, they should be guarded
by their own feature flags and run their own finch trials.

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

[matthalp, 2017-06-28 17:25:30]

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

[matthalp, 2017-06-28 17:25:31]

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

[matthalp, 2017-06-28 17:25:42]

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

[commit-bot: I haz the power, 2017-06-28 17:26:10]

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-28 17:57:21]

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

[commit-bot: I haz the power, 2017-06-28 17:57:22]

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

[lpy, 2017-06-28 17:57:32]

lgtm
Does it mean IsEnable always returns false for unittest? Why did it fail before?

[zhenw, 2017-06-28 18:06:56]

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

[zhenw, 2017-06-28 18:06:57]

Are currently failing ones still related to enabling GRC?

https://codereview.chromium.org/2958133003/diff/1/chrome/browser/resource_coo...
File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):

https://codereview.chromium.org/2958133003/diff/1/chrome/browser/resource_coo...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:80:
content::ServiceManagerConnection* service_manager_connection =
content::ServiceManagerConnection::GetForProcess();
nit: the line is too long.

[matthalp, 2017-06-28 18:22:58]

On 2017/06/28 at 18:06:57, zhenw wrote:
> Are currently failing ones still related to enabling GRC?
> 
>
https://codereview.chromium.org/2958133003/diff/1/chrome/browser/resource_coo...
> File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2958133003/diff/1/chrome/browser/resource_coo...
>
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:80:
content::ServiceManagerConnection* service_manager_connection =
content::ServiceManagerConnection::GetForProcess();
> nit: the line is too long.

The tests are failing back the ResourceCoordinatorWebContentsObserver is being
created when the resource_coordinator service has not been created. The
|content::ServiceManagerConnection::GetForProcess()| check I added is not
conservative enough as the |service_manager| can be created without the
|resource_coordinator| service being there.

[matthalp, 2017-06-29 04:01:38]

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

[matthalp, 2017-06-29 04:01:52]

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

[commit-bot: I haz the power, 2017-06-29 04:01:56]

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 05:28:59]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[matthalp, 2017-06-29 15:30:07]

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

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

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-29 16:19:28]

PTAL: I found a simple way to check if the content_browser service is active.
This is the service that the ResourceCoordinatorWebContentsObserver is meant to
live in, so I think it's a reasonable check. If you two agree, we can confirm
with rockot.

[zhenw, 2017-06-29 16:28:30]

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

[zhenw, 2017-06-29 16:28:30]

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

https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:84:
content::mojom::kBrowserServiceName &&
Just to confirm my understanding. So this indicates that the child identity name
is empty string in the test?
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...

Are there other cases where a WebContents can be associated with other types of
services?
https://cs.chromium.org/chromium/src/out/Debug/gen/content/public/common/serv...

[matthalp, 2017-06-29 16:35:09]

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

https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:84:
content::mojom::kBrowserServiceName &&
On 2017/06/29 at 16:28:30, zhenw wrote:
> Just to confirm my understanding. So this indicates that the child identity
name is empty string in the test?
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...

Yes. The mocks leave this as an empty string for now.

> Are there other cases where a WebContents can be associated with other types
of services?
>
https://cs.chromium.org/chromium/src/out/Debug/gen/content/public/common/serv...

The check is not checking what service the WebContents is in. It is checking to
make sure that the ResourceCoordinatorWebContents observer is being created in
the content_browser service. Currently, content_browser is the only service that
utilizes resource_coordinator service's capabilities:
https://cs.chromium.org/chromium/src/content/public/app/mojo/content_browser_...

[commit-bot: I haz the power, 2017-06-29 16:36:04]

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

[commit-bot: I haz the power, 2017-06-29 16:36:05]

Dry run: This issue passed the CQ dry run.

[Zhen Wang, 2017-06-29 16:43:17]

zhenw@chromium.org changed reviewers:
+ rockot@chromium.org

[Zhen Wang, 2017-06-29 16:43:19]

lgtm

+rockot

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

https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h:24:
static bool ShouldCreate(content::WebContents* web_contents);
nit: The name does not sound correct to me in grammar sense. I see people
usually use ShouldCreateXYZ instead of ShouldCreate. In this case, maybe
ShouldBeCreated is better?

[Ken Rockot(use gerrit already), 2017-06-29 16:51:24]

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

https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:84:
content::mojom::kBrowserServiceName &&
On 2017/06/29 at 16:35:08, matthalp wrote:
> On 2017/06/29 at 16:28:30, zhenw wrote:
> > Just to confirm my understanding. So this indicates that the child identity
name is empty string in the test?
> >
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
> 
> Yes. The mocks leave this as an empty string for now.
> 
> > Are there other cases where a WebContents can be associated with other types
of services?
> >
https://cs.chromium.org/chromium/src/out/Debug/gen/content/public/common/serv...
> 
> The check is not checking what service the WebContents is in. It is checking
to make sure that the ResourceCoordinatorWebContents observer is being created
in the content_browser service. Currently, content_browser is the only service
that utilizes resource_coordinator service's capabilities:
https://cs.chromium.org/chromium/src/content/public/app/mojo/content_browser_...

This is not correct -- a child's identity is never content_browser, so this
condition will always evaluate to false (if you have a case where it doesn't,
please file a bug, that's definitely wrong.) You are thus effectively never
creating this WebContentsObserver, which may explain why you don't see test
failures despite not fixing the MockRenderProcessHost behavior.

Furthermore RPH::GetChildIdentity must always return a content_renderer
identity, so the check is redundant anyway.

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

On 2017/06/29 at 16:51:24, rockot wrote:
>
https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
> File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
>
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:84:
content::mojom::kBrowserServiceName &&
> On 2017/06/29 at 16:35:08, matthalp wrote:
> > On 2017/06/29 at 16:28:30, zhenw wrote:
> > > Just to confirm my understanding. So this indicates that the child
identity name is empty string in the test?
> > >
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
> > 
> > Yes. The mocks leave this as an empty string for now.
> > 
> > > Are there other cases where a WebContents can be associated with other
types of services?
> > >
https://cs.chromium.org/chromium/src/out/Debug/gen/content/public/common/serv...
> > 
> > The check is not checking what service the WebContents is in. It is checking
to make sure that the ResourceCoordinatorWebContents observer is being created
in the content_browser service. Currently, content_browser is the only service
that utilizes resource_coordinator service's capabilities:
https://cs.chromium.org/chromium/src/content/public/app/mojo/content_browser_...
> 
> This is not correct -- a child's identity is never content_browser, so this
condition will always evaluate to false (if you have a case where it doesn't,
please file a bug, that's definitely wrong.) You are thus effectively never
creating this WebContentsObserver, which may explain why you don't see test
failures despite not fixing the MockRenderProcessHost behavior.
> 
> Furthermore RPH::GetChildIdentity must always return a content_renderer
identity, so the check is redundant anyway.

My mistake, I had kBrowserServiceName instead of kRendererService name in the
check. That has been fixed. Sorry about that.

Maybe this is not the desired behavior but the
MockRenderProcessHost::GetIdentity does not return a content_renderer identity.
It is empty:
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...

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

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

[commit-bot: I haz the power, 2017-06-29 17:14:58]

Dry run: CQ is trying da patch.

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

[Ken Rockot(use gerrit already), 2017-06-29 17:26:09]

On 2017/06/29 at 17:14:20, matthalp wrote:
> On 2017/06/29 at 16:51:24, rockot wrote:
> >
https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
> > File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):
> > 
> >
https://codereview.chromium.org/2958133003/diff/40001/chrome/browser/resource...
> >
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:84:
content::mojom::kBrowserServiceName &&
> > On 2017/06/29 at 16:35:08, matthalp wrote:
> > > On 2017/06/29 at 16:28:30, zhenw wrote:
> > > > Just to confirm my understanding. So this indicates that the child
identity name is empty string in the test?
> > > >
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
> > > 
> > > Yes. The mocks leave this as an empty string for now.
> > > 
> > > > Are there other cases where a WebContents can be associated with other
types of services?
> > > >
https://cs.chromium.org/chromium/src/out/Debug/gen/content/public/common/serv...
> > > 
> > > The check is not checking what service the WebContents is in. It is
checking to make sure that the ResourceCoordinatorWebContents observer is being
created in the content_browser service. Currently, content_browser is the only
service that utilizes resource_coordinator service's capabilities:
https://cs.chromium.org/chromium/src/content/public/app/mojo/content_browser_...
> > 
> > This is not correct -- a child's identity is never content_browser, so this
condition will always evaluate to false (if you have a case where it doesn't,
please file a bug, that's definitely wrong.) You are thus effectively never
creating this WebContentsObserver, which may explain why you don't see test
failures despite not fixing the MockRenderProcessHost behavior.
> > 
> > Furthermore RPH::GetChildIdentity must always return a content_renderer
identity, so the check is redundant anyway.
> 
> My mistake, I had kBrowserServiceName instead of kRendererService name in the
check. That has been fixed. Sorry about that.
> 
> Maybe this is not the desired behavior but the
MockRenderProcessHost::GetIdentity does not return a content_renderer identity.
It is empty:
>
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...

I see. OK, but I still don't think this is a great idea. Now you have a weird
subtle dependency on a detail of MockRPH that nobody can change without fixing
your code. What's wrong with the solution we discussed yesterday?

[chromium-reviews, 2017-06-29 17:33:57]

I agree that this introduces an implicit dependency on the MockRPH, but
just wanted to gauge this before the other approach. The advantage here is
that we don't run the code at all. It's useless computation and, while not
the case now, someone's future code here has to be extra careful about
inducing side-effects like what was run into here.

Nonetheless I switch to the empty CoordinationUnitPtr for now and revisit
this if and when it becomes an issue.

On Thu, Jun 29, 2017 at 10:26 AM, <rockot@chromium.org> wrote:

> On 2017/06/29 at 17:14:20, matthalp wrote:
> > On 2017/06/29 at 16:51:24, rockot wrote:
> > >
> https://codereview.chromium.org/2958133003/diff/40001/
> chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> observer.cc
> > > File
> chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> observer.cc
> (right):
> > >
> > >
> https://codereview.chromium.org/2958133003/diff/40001/
> chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> observer.cc#newcode84
> > >
> chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> observer.cc:84:
> content::mojom::kBrowserServiceName &&
> > > On 2017/06/29 at 16:35:08, matthalp wrote:
> > > > On 2017/06/29 at 16:28:30, zhenw wrote:
> > > > > Just to confirm my understanding. So this indicates that the child
> identity name is empty string in the test?
> > > > >
> https://cs.chromium.org/chromium/src/content/public/
> test/mock_render_process_host.h?l=184
> > > >
> > > > Yes. The mocks leave this as an empty string for now.
> > > >
> > > > > Are there other cases where a WebContents can be associated with
> other
> types of services?
> > > > >
> https://cs.chromium.org/chromium/src/out/Debug/gen/
> content/public/common/service_names.mojom.cc
> > > >
> > > > The check is not checking what service the WebContents is in. It is
> checking to make sure that the ResourceCoordinatorWebContents observer is
> being
> created in the content_browser service. Currently, content_browser is the
> only
> service that utilizes resource_coordinator service's capabilities:
> https://cs.chromium.org/chromium/src/content/public/
> app/mojo/content_browser_manifest.json?q=content_browser_ma&l=86
> > >
> > > This is not correct -- a child's identity is never content_browser, so
> this
> condition will always evaluate to false (if you have a case where it
> doesn't,
> please file a bug, that's definitely wrong.) You are thus effectively never
> creating this WebContentsObserver, which may explain why you don't see test
> failures despite not fixing the MockRenderProcessHost behavior.
> > >
> > > Furthermore RPH::GetChildIdentity must always return a content_renderer
> identity, so the check is redundant anyway.
> >
> > My mistake, I had kBrowserServiceName instead of kRendererService name
> in the
> check. That has been fixed. Sorry about that.
> >
> > Maybe this is not the desired behavior but the
> MockRenderProcessHost::GetIdentity does not return a content_renderer
> identity.
> It is empty:
> >
> https://cs.chromium.org/chromium/src/content/public/
> test/mock_render_process_host.h?dr=C&q=mock_render_process_host&l=184
>
> I see. OK, but I still don't think this is a great idea. Now you have a
> weird
> subtle dependency on a detail of MockRPH that nobody can change without
> fixing
> your code. What's wrong with the solution we discussed yesterday?
>
>
> https://codereview.chromium.org/2958133003/
>



-- 

Matthew Halpern
Intern, Chrome Speed Infrastructure

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[matthalp, 2017-06-29 18:30:04]

On 2017/06/29 at 17:33:57, chromium-reviews wrote:
> I agree that this introduces an implicit dependency on the MockRPH, but
> just wanted to gauge this before the other approach. The advantage here is
> that we don't run the code at all. It's useless computation and, while not
> the case now, someone's future code here has to be extra careful about
> inducing side-effects like what was run into here.
> 
> Nonetheless I switch to the empty CoordinationUnitPtr for now and revisit
> this if and when it becomes an issue.
> 
> On Thu, Jun 29, 2017 at 10:26 AM, <rockot@chromium.org> wrote:
> 
> > On 2017/06/29 at 17:14:20, matthalp wrote:
> > > On 2017/06/29 at 16:51:24, rockot wrote:
> > > >
> > https://codereview.chromium.org/2958133003/diff/40001/
> > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > observer.cc
> > > > File
> > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > observer.cc
> > (right):
> > > >
> > > >
> > https://codereview.chromium.org/2958133003/diff/40001/
> > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > observer.cc#newcode84
> > > >
> > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > observer.cc:84:
> > content::mojom::kBrowserServiceName &&
> > > > On 2017/06/29 at 16:35:08, matthalp wrote:
> > > > > On 2017/06/29 at 16:28:30, zhenw wrote:
> > > > > > Just to confirm my understanding. So this indicates that the child
> > identity name is empty string in the test?
> > > > > >
> > https://cs.chromium.org/chromium/src/content/public/
> > test/mock_render_process_host.h?l=184
> > > > >
> > > > > Yes. The mocks leave this as an empty string for now.
> > > > >
> > > > > > Are there other cases where a WebContents can be associated with
> > other
> > types of services?
> > > > > >
> > https://cs.chromium.org/chromium/src/out/Debug/gen/
> > content/public/common/service_names.mojom.cc
> > > > >
> > > > > The check is not checking what service the WebContents is in. It is
> > checking to make sure that the ResourceCoordinatorWebContents observer is
> > being
> > created in the content_browser service. Currently, content_browser is the
> > only
> > service that utilizes resource_coordinator service's capabilities:
> > https://cs.chromium.org/chromium/src/content/public/
> > app/mojo/content_browser_manifest.json?q=content_browser_ma&l=86
> > > >
> > > > This is not correct -- a child's identity is never content_browser, so
> > this
> > condition will always evaluate to false (if you have a case where it
> > doesn't,
> > please file a bug, that's definitely wrong.) You are thus effectively never
> > creating this WebContentsObserver, which may explain why you don't see test
> > failures despite not fixing the MockRenderProcessHost behavior.
> > > >
> > > > Furthermore RPH::GetChildIdentity must always return a content_renderer
> > identity, so the check is redundant anyway.
> > >
> > > My mistake, I had kBrowserServiceName instead of kRendererService name
> > in the
> > check. That has been fixed. Sorry about that.
> > >
> > > Maybe this is not the desired behavior but the
> > MockRenderProcessHost::GetIdentity does not return a content_renderer
> > identity.
> > It is empty:
> > >
> > https://cs.chromium.org/chromium/src/content/public/
> > test/mock_render_process_host.h?dr=C&q=mock_render_process_host&l=184
> >
> > I see. OK, but I still don't think this is a great idea. Now you have a
> > weird
> > subtle dependency on a detail of MockRPH that nobody can change without
> > fixing
> > your code. What's wrong with the solution we discussed yesterday?
> >
> >
> > https://codereview.chromium.org/2958133003/
> >
> 
> 
> 
> -- 
> 
> Matthew Halpern
> Intern, Chrome Speed Infrastructure
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

On second glance, it does not seem like the empty interface pointer approach
will work due to software layering issues. All of the resource_coordinator
service code is in services/* while the MockRenderProcessHost is in content/*.

As a reminder, the delinquent code is here:
https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
which is called from:
https://cs.chromium.org/chromium/src/chrome/browser/resource_coordinator/reso...

What are your thoughts?

[Ken Rockot(use gerrit already), 2017-06-29 19:39:46]

I'm not understanding something: what's wrong with content depending on the
resource_coordinator client library? Isn't that all that's required here?

On Thu, Jun 29, 2017 at 11:30 AM, <matthalp@google.com> wrote:

> On 2017/06/29 at 17:33:57, chromium-reviews wrote:
> > I agree that this introduces an implicit dependency on the MockRPH, but
> > just wanted to gauge this before the other approach. The advantage here
> is
> > that we don't run the code at all. It's useless computation and, while
> not
> > the case now, someone's future code here has to be extra careful about
> > inducing side-effects like what was run into here.
> >
> > Nonetheless I switch to the empty CoordinationUnitPtr for now and revisit
> > this if and when it becomes an issue.
> >
> > On Thu, Jun 29, 2017 at 10:26 AM, <rockot@chromium.org&gt; wrote:
> >
> > > On 2017/06/29 at 17:14:20, matthalp wrote:
> > > > On 2017/06/29 at 16:51:24, rockot wrote:
> > > > >
> > > https://codereview.chromium.org/2958133003/diff/40001/
> > > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > > observer.cc
> > > > > File
> > > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > > observer.cc
> > > (right):
> > > > >
> > > > >
> > > https://codereview.chromium.org/2958133003/diff/40001/
> > > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > > observer.cc#newcode84
> > > > >
> > > chrome/browser/resource_coordinator/resource_coordinator_web_contents_
> > > observer.cc:84:
> > > content::mojom::kBrowserServiceName &&
> > > > > On 2017/06/29 at 16:35:08, matthalp wrote:
> > > > > > On 2017/06/29 at 16:28:30, zhenw wrote:
> > > > > > > Just to confirm my understanding. So this indicates that the
> child
> > > identity name is empty string in the test?
> > > > > > >
> > > https://cs.chromium.org/chromium/src/content/public/
> > > test/mock_render_process_host.h?l=184
> > > > > >
> > > > > > Yes. The mocks leave this as an empty string for now.
> > > > > >
> > > > > > > Are there other cases where a WebContents can be associated
> with
> > > other
> > > types of services?
> > > > > > >
> > > https://cs.chromium.org/chromium/src/out/Debug/gen/
> > > content/public/common/service_names.mojom.cc
> > > > > >
> > > > > > The check is not checking what service the WebContents is in. It
> is
> > > checking to make sure that the ResourceCoordinatorWebContents observer
> is
> > > being
> > > created in the content_browser service. Currently, content_browser is
> the
> > > only
> > > service that utilizes resource_coordinator service's capabilities:
> > > https://cs.chromium.org/chromium/src/content/public/
> > > app/mojo/content_browser_manifest.json?q=content_browser_ma&l=86
> > > > >
> > > > > This is not correct -- a child's identity is never
> content_browser, so
> > > this
> > > condition will always evaluate to false (if you have a case where it
> > > doesn't,
> > > please file a bug, that's definitely wrong.) You are thus effectively
> never
> > > creating this WebContentsObserver, which may explain why you don't see
> test
> > > failures despite not fixing the MockRenderProcessHost behavior.
> > > > >
> > > > > Furthermore RPH::GetChildIdentity must always return a
> content_renderer
> > > identity, so the check is redundant anyway.
> > > >
> > > > My mistake, I had kBrowserServiceName instead of kRendererService
> name
> > > in the
> > > check. That has been fixed. Sorry about that.
> > > >
> > > > Maybe this is not the desired behavior but the
> > > MockRenderProcessHost::GetIdentity does not return a content_renderer
> > > identity.
> > > It is empty:
> > > >
> > > https://cs.chromium.org/chromium/src/content/public/
> > > test/mock_render_process_host.h?dr=C&q=mock_render_process_host&l=184
> > >
> > > I see. OK, but I still don't think this is a great idea. Now you have a
> > > weird
> > > subtle dependency on a detail of MockRPH that nobody can change without
> > > fixing
> > > your code. What's wrong with the solution we discussed yesterday?
> > >
> > >
> > > https://codereview.chromium.org/2958133003/
> > >
> >
> >
> >
> > --
> >
> > Matthew Halpern
> > Intern, Chrome Speed Infrastructure
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
> >
>
> On second glance, it does not seem like the empty interface pointer
> approach
> will work due to software layering issues. All of the resource_coordinator
> service code is in services/* while the MockRenderProcessHost is in
> content/*.
>
> As a reminder, the delinquent code is here:
> https://cs.chromium.org/chromium/src/content/public/
> test/mock_render_process_host.cc?type=cs&l=349
> which is called from:
> https://cs.chromium.org/chromium/src/chrome/browser/
> resource_coordinator/resource_coordinator_web_contents_
> observer.cc?type=cs&q=ResourceCoordinatorWebCon&l=108
>
> What are your thoughts?
>
> https://codereview.chromium.org/2958133003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-06-29 20:46:27]

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

[commit-bot: I haz the power, 2017-06-29 20:46:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[matthalp, 2017-06-30 03:22:37]

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

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

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 04:05:37]

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

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

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_...)

[matthalp, 2017-06-30 15:49:09]

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

[commit-bot: I haz the power, 2017-06-30 15:49:19]

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 19:31:55]

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

[commit-bot: I haz the power, 2017-06-30 19:31:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

rockot:

  (1) Adding services/* code to content/* was not a problem. Could you check
that the changes to MockRenderProcessHost::GetProcessResourceCoordinator are
consistent with what you were thinking?

  (2) I forgot to mention in our meeting, but it seems like there are cases
where content::ServiceManagerConnection::GetForProcess returns a nullptr. I'm
not too familiar with the LazyInstance class, but I was surprised that nullptr
was returned. This is really where much of the unittest breakage is occuring
from. Are you okay with the nullptr check in
ResourceCoordinatorWebContentsObserver::IsEnabled? It does have a hackiness
associated with it.

Once you feel comfortable with what is going on here, I'll add a reviewer for
the changes to MockRenderProcessHost, etc. I just want to get us in sync first.

[Ken Rockot(use gerrit already), 2017-06-30 22:37:11]

lgtm

[Ken Rockot(use gerrit already), 2017-06-30 22:37:25]

On 2017/06/30 at 22:37:11, Ken Rockot(use gerrit already) wrote:
> lgtm

(All seems fine to me)

[Zhen Wang, 2017-06-30 22:51:54]

Thanks, Matt! lgtm stamp

[matthalp, 2017-07-01 02:02:47]

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

[commit-bot: I haz the power, 2017-07-01 02:02:57]

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 02:32:25]

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

[commit-bot: I haz the power, 2017-07-01 02:32:26]

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

[matthalp, 2017-07-01 17:28:10]

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

[commit-bot: I haz the power, 2017-07-01 17:28:17]

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 17:55:29]

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

[commit-bot: I haz the power, 2017-07-01 17:55:30]

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

[matthalp, 2017-07-01 19:05:19]

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

[commit-bot: I haz the power, 2017-07-01 19:05:28]

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 19:37:18]

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

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

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

[matthalp, 2017-07-05 18:18:58]

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

[commit-bot: I haz the power, 2017-07-05 18:19:32]

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-05 18:52:23]

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

[commit-bot: I haz the power, 2017-07-05 18:52:24]

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

[matthalp, 2017-07-05 21:02:58]

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

[commit-bot: I haz the power, 2017-07-05 21:03:19]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-05 22:03:04]

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

[commit-bot: I haz the power, 2017-07-05 22:03:23]

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-06 00:04:08]

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

[commit-bot: I haz the power, 2017-07-06 00:04:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[matthalp, 2017-07-06 15:31:03]

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

[commit-bot: I haz the power, 2017-07-06 15:31:20]

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-06 16:17:45]

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

[commit-bot: I haz the power, 2017-07-06 16:17:46]

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_...)

[matthalp, 2017-07-06 18:22:38]

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

[commit-bot: I haz the power, 2017-07-06 18:22:51]

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

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

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

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_...)

[matthalp, 2017-07-06 20:46:27]

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

[commit-bot: I haz the power, 2017-07-06 20:46:43]

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

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

[commit-bot: I haz the power, 2017-07-06 20:49:39]

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

[matthalp, 2017-07-06 20:51:26]

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

[commit-bot: I haz the power, 2017-07-06 20:51:43]

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

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

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

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

[matthalp, 2017-07-06 21:10:22]

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

[commit-bot: I haz the power, 2017-07-06 21:10:36]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-06 21:20:04]

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

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

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-06 21:46:52]

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

[commit-bot: I haz the power, 2017-07-06 21:46:54]

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

[matthalp, 2017-07-11 17:36:11]

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

[commit-bot: I haz the power, 2017-07-11 17:36:33]

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-11 18:45:38]

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

[commit-bot: I haz the power, 2017-07-11 18:45:39]

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

[matthalp, 2017-07-12 02:00:35]

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

[commit-bot: I haz the power, 2017-07-12 02:00:56]

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-12 03:20:48]

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

[commit-bot: I haz the power, 2017-07-12 03:20:51]

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_...)

[matthalp, 2017-07-12 04:19:01]

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

[commit-bot: I haz the power, 2017-07-12 04:19:09]

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-12 05:38:04]

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

[commit-bot: I haz the power, 2017-07-12 05:38:05]

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...)

[matthalp, 2017-07-12 22:15:06]

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

[commit-bot: I haz the power, 2017-07-12 22:15:28]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-12 22:36:25]

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

[commit-bot: I haz the power, 2017-07-12 22:36:41]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-12 22:38:08]

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

[commit-bot: I haz the power, 2017-07-12 22:38:29]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-12 22:51:27]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-07-12 23:30:31]

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

[commit-bot: I haz the power, 2017-07-12 23:31:06]

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-13 01:17:10]

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

[commit-bot: I haz the power, 2017-07-13 01:17:11]

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

[matthalp, 2017-07-13 22:22:20]

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

[commit-bot: I haz the power, 2017-07-13 22: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-07-13 23:56:26]

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

[commit-bot: I haz the power, 2017-07-13 23:56:46]

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-14 02:17:25]

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

[commit-bot: I haz the power, 2017-07-14 02:17:27]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-07-14 02:40:03]

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

[commit-bot: I haz the power, 2017-07-14 02:40:19]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-14 03:40:47]

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

[commit-bot: I haz the power, 2017-07-14 03:40:56]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-14 03:56:15]

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

[commit-bot: I haz the power, 2017-07-14 03:56:33]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-14 03:59:18]

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

[commit-bot: I haz the power, 2017-07-14 03:59:32]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-14 04:20:25]

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

[commit-bot: I haz the power, 2017-07-14 04:20: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-07-14 06:09:03]

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

[commit-bot: I haz the power, 2017-07-14 06:09:04]

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

[matthalp, 2017-07-14 14:19:44]

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

[commit-bot: I haz the power, 2017-07-14 14:19:48]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-14 15:21:31]

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

[commit-bot: I haz the power, 2017-07-14 15:21:43]

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-14 17:21:37]

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

[commit-bot: I haz the power, 2017-07-14 17:21:39]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[matthalp, 2017-07-14 18:38:54]

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

[commit-bot: I haz the power, 2017-07-14 18:39:08]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-14 19:47:28]

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

[commit-bot: I haz the power, 2017-07-14 19:47:39]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-07-14 19:58:45]

matthalp@google.com changed reviewers:
+ nasko@chromium.org
- zhenw@google.com

[matthalp, 2017-07-14 19:59:01]

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

[commit-bot: I haz the power, 2017-07-14 19:59:09]

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-14 21:12:12]

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

[commit-bot: I haz the power, 2017-07-14 21:12:14]

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...)