Move FrameMsg_NewFrame/NewFrameProxy to mojom

Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where one of
these interface requests could be lazily acquired during Channel
pause, blocking a subsequent message on that interface which
should be sent immediately while the Channel is unpaused.
See https://goo.gl/ot0S32 for some explanation of this behavior.

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/53be7caf3a84b07c4246c5c250ccac629113db9c
Cr-Commit-Position: refs/heads/master@{#422900}

[Ken Rockot(use gerrit already), 2016-09-30 22:57:26]

Description was changed from

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

BUG=612500
==========

to

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-09-30 22:57:28]

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

[commit-bot: I haz the power, 2016-09-30 22:58:03]

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), 2016-09-30 23:15:01]

rockot@chromium.org changed reviewers:
+ nick@chromium.org, tsepez@chromium.org

[Ken Rockot(use gerrit already), 2016-09-30 23:15:02]

Please take a look. Thanks!

[commit-bot: I haz the power, 2016-09-30 23:31:29]

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

[commit-bot: I haz the power, 2016-09-30 23:31:29]

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...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ncarter (slow), 2016-09-30 23:35:14]

https://codereview.chromium.org/2383853003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2383853003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.cc:189:
static_cast<RenderProcessHostImpl*>(GetProcess())->GetRendererInterface()
RPH->RPHI is not a safe cast; very similar to the RenderThread/MockRenderThread
situation, MockRenderProcessHost is not a RenderProcessHostImpl. So, code that
does this kind of cast will crash -- or worse -- if executed from inside a
content_unittest. If this works today, it's just because we don't have
content_unittest coverage for this case.

Which is possibly not too surprising if it is the case for RenderFrameProxyHost,
which is pretty hard to exercise outside of a browsertest, but I suspect we are
going to need a solution to this pattern in general that avoids the bad  cast.

https://codereview.chromium.org/2383853003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2383853003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6078:
static_cast<RenderProcessHostImpl*>(process_a)->GetRendererInterface()
Note that this cast is safe (the only place where we can reliably cast from RPH
to RPHI is in _browsertest.cc code).

[Ken Rockot(use gerrit already), 2016-09-30 23:39:13]

On 2016/09/30 at 23:35:14, nick wrote:
>
https://codereview.chromium.org/2383853003/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/render_frame_proxy_host.cc (right):
> 
>
https://codereview.chromium.org/2383853003/diff/1/content/browser/frame_host/...
> content/browser/frame_host/render_frame_proxy_host.cc:189:
static_cast<RenderProcessHostImpl*>(GetProcess())->GetRendererInterface()
> RPH->RPHI is not a safe cast; very similar to the
RenderThread/MockRenderThread situation, MockRenderProcessHost is not a
RenderProcessHostImpl. So, code that does this kind of cast will crash -- or
worse -- if executed from inside a content_unittest. If this works today, it's
just because we don't have content_unittest coverage for this case.
> 
> Which is possibly not too surprising if it is the case for
RenderFrameProxyHost, which is pretty hard to exercise outside of a browsertest,
but I suspect we are going to need a solution to this pattern in general that
avoids the bad  cast.

Ah, for some reason I believed there was no Mock RPH. Clearly there is. I'll
think about how to do this better.

> 
>
https://codereview.chromium.org/2383853003/diff/1/content/browser/site_per_pr...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2383853003/diff/1/content/browser/site_per_pr...
> content/browser/site_per_process_browsertest.cc:6078:
static_cast<RenderProcessHostImpl*>(process_a)->GetRendererInterface()
> Note that this cast is safe (the only place where we can reliably cast from
RPH to RPHI is in _browsertest.cc code).

[Ken Rockot(use gerrit already), 2016-10-03 17:38:20]

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

[commit-bot: I haz the power, 2016-10-03 17:38:44]

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), 2016-10-03 17:44:23]

OK - I've changed this to take advantage of SupportsUserData in RPHI and added a
static accessor on RPHI to get at the mojom::Renderer interface. It doesn't do
any unsafe casts, and if one attempts to use a MockRPH with
GetRendererInterface, they'll hit a reliable DCHECK.

I'm not crazy about using SupportsUserData in general, but the only reasonable
alternative I can suggest is to have that static accessor branch on whether or
not GetChannel() is null, and then conditionally do a "safe" unsafe cast. That
feels like a bit of a fragile hack to me though.

Finally, another variant of the latter approach would be to add some explicit
public interface to ask the impl if it's a real impl or a mock, which is less
fragile than testing GetChannel() but far more offensive to me all the same.


WDYT?

[commit-bot: I haz the power, 2016-10-03 18:04:05]

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

[commit-bot: I haz the power, 2016-10-03 18:04:06]

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

[Ken Rockot(use gerrit already), 2016-10-03 19:28:08]

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

[commit-bot: I haz the power, 2016-10-03 19:28:34]

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), 2016-10-03 19:29:55]

On 2016/10/03 at 17:44:23, Ken Rockot wrote:
> OK - I've changed this to take advantage of SupportsUserData in RPHI and added
a static accessor on RPHI to get at the mojom::Renderer interface. It doesn't do
any unsafe casts, and if one attempts to use a MockRPH with
GetRendererInterface, they'll hit a reliable DCHECK.
> 
> I'm not crazy about using SupportsUserData in general, but the only reasonable
alternative I can suggest is to have that static accessor branch on whether or
not GetChannel() is null, and then conditionally do a "safe" unsafe cast. That
feels like a bit of a fragile hack to me though.
> 
> Finally, another variant of the latter approach would be to add some explicit
public interface to ask the impl if it's a real impl or a mock, which is less
fragile than testing GetChannel() but far more offensive to me all the same.
> 
> 
> WDYT?

I've reworked it a bit in light of the fact that quite a few unit tests end up
sending these messages indirectly, though they don't care about the side
effects.

Now RPHI::GetRendererInterface always returns non-null. If there's no IPC
channel, the proxy it returns fires its messages into nothingness.

Still using SupportsUserData to stash the proxy.

[Ken Rockot(use gerrit already), 2016-10-03 19:31:23]

On 2016/10/03 at 19:29:55, Ken Rockot wrote:
> On 2016/10/03 at 17:44:23, Ken Rockot wrote:
> > OK - I've changed this to take advantage of SupportsUserData in RPHI and
added a static accessor on RPHI to get at the mojom::Renderer interface. It
doesn't do any unsafe casts, and if one attempts to use a MockRPH with
GetRendererInterface, they'll hit a reliable DCHECK.
> > 
> > I'm not crazy about using SupportsUserData in general, but the only
reasonable alternative I can suggest is to have that static accessor branch on
whether or not GetChannel() is null, and then conditionally do a "safe" unsafe
cast. That feels like a bit of a fragile hack to me though.
> > 
> > Finally, another variant of the latter approach would be to add some
explicit public interface to ask the impl if it's a real impl or a mock, which
is less fragile than testing GetChannel() but far more offensive to me all the
same.
> > 
> > 
> > WDYT?
> 
> I've reworked it a bit in light of the fact that quite a few unit tests end up
sending these messages indirectly, though they don't care about the side
effects.
> 
> Now RPHI::GetRendererInterface always returns non-null. If there's no IPC
channel, the proxy it returns fires its messages into nothingness.
> 
> Still using SupportsUserData to stash the proxy.

Also note that the existing usage of GetRemoteRouteProvider also incorrectly
relies on a potentially bad cast downcast to RPHI. I can reuse the strategy in
this CL to address that either here or in a follow-up CL.

[commit-bot: I haz the power, 2016-10-03 20:15:09]

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

[commit-bot: I haz the power, 2016-10-03 20:15:10]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-10-03 22:45:09]

lgtm with this approach; it seems like the right way to work inside of the
existing design.

https://codereview.chromium.org/2383853003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2383853003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:181: static
mojom::Renderer* GetRendererInterface(RenderProcessHost* host);
I'm okay with this approach. Putting a static method in RPHI that accepts an
RPH* seems to be an established pattern -- RegisterHost, IsSuitableHost, and a
few others work that way.

Really the existence of this pattern is a sign that something is lacking in the
RenderProcessHost/RenderProcessHostImpl/MockRenderProcessHost design, which I
would characterize as some combination of, "RenderProcessHost is doing
double-duty as both the public interface of RenderProcessHostImpl and as the
base class for a Mock version of RenderProcessHostImpl," "RenderProcessHostImpl
is too big to mock."

Historically, MockRenderProcessHost used to inherit from RenderProcessHostImpl
(this changed as part of the content api extraction:
https://chromium.googlesource.com/chromium/src/+/f3b1a084a01ab82caf998daefcb9...
)

One fix would be for us to extract a content-private class
{RenderProcessHostBase : public RenderProcessHost}, and move methods there from
both the public interface (like RenderProcessHost::Init) and static methods from
RenderProcessHostImpl. RPH -> RPHB would be a safe cast.

A more extreme fix, would be to make RPH->RPHI a safe cast, ideally by moving
the child-process-launcher behaviors into an interface that's mockable, and
making RPHI a final class. MockRPH would really only exist as a
RenderProcessHostFactory that passes a mock process-laucher-delegate or whatever
instance to the RPHI ctor, plus some easy way to get at the IPC::TestSink and
friends. This actually sounds pretty good to me as an end state, though the
reality of what the code needs to get done seems like it could throw any number
of wrenches in the feasibility of this idea.

I've put the above griping into a bug: 652474

[Ken Rockot(use gerrit already), 2016-10-03 22:54:23]

On 2016/10/03 at 22:45:09, nick wrote:
> lgtm with this approach; it seems like the right way to work inside of the
existing design.
> 
>
https://codereview.chromium.org/2383853003/diff/40001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.h (right):
> 
>
https://codereview.chromium.org/2383853003/diff/40001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.h:181: static
mojom::Renderer* GetRendererInterface(RenderProcessHost* host);
> I'm okay with this approach. Putting a static method in RPHI that accepts an
RPH* seems to be an established pattern -- RegisterHost, IsSuitableHost, and a
few others work that way.
> 
> Really the existence of this pattern is a sign that something is lacking in
the RenderProcessHost/RenderProcessHostImpl/MockRenderProcessHost design, which
I would characterize as some combination of, "RenderProcessHost is doing
double-duty as both the public interface of RenderProcessHostImpl and as the
base class for a Mock version of RenderProcessHostImpl," "RenderProcessHostImpl
is too big to mock."
> 
> Historically, MockRenderProcessHost used to inherit from RenderProcessHostImpl
(this changed as part of the content api extraction:
https://chromium.googlesource.com/chromium/src/+/f3b1a084a01ab82caf998daefcb9...
)
> 
> One fix would be for us to extract a content-private class
{RenderProcessHostBase : public RenderProcessHost}, and move methods there from
both the public interface (like RenderProcessHost::Init) and static methods from
RenderProcessHostImpl. RPH -> RPHB would be a safe cast.
> 
> A more extreme fix, would be to make RPH->RPHI a safe cast, ideally by moving
the child-process-launcher behaviors into an interface that's mockable, and
making RPHI a final class. MockRPH would really only exist as a
RenderProcessHostFactory that passes a mock process-laucher-delegate or whatever
instance to the RPHI ctor, plus some easy way to get at the IPC::TestSink and
friends. This actually sounds pretty good to me as an end state, though the
reality of what the code needs to get done seems like it could throw any number
of wrenches in the feasibility of this idea.
> 
> I've put the above griping into a bug: 652474

Cool, thanks for filing the bug. Ideally as some of this conversion work
progresses we can also just make RPH smaller.

[Ken Rockot(use gerrit already), 2016-10-03 22:54:33]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-10-03 22:55:24]

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), 2016-10-03 23:08:20]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-10-03 23:30:34]

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

[Ken Rockot(use gerrit already), 2016-10-03 23:31:12]

Friendly ping tsepez@ for mojom

[commit-bot: I haz the power, 2016-10-03 23:31:22]

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, 2016-10-04 00:38:27]

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

[commit-bot: I haz the power, 2016-10-04 00:38:28]

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_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-10-04 01:54:42]

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

[commit-bot: I haz the power, 2016-10-04 01:55:39]

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), 2016-10-04 01:56:09]

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

[commit-bot: I haz the power, 2016-10-04 01:56:23]

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), 2016-10-04 01:59:22]

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

[commit-bot: I haz the power, 2016-10-04 01:59:52]

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), 2016-10-04 02:02:03]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-10-04 03:12:09]

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

[commit-bot: I haz the power, 2016-10-04 03:12:10]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-04 15:21:42]

Description was changed from

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

1. The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where the
interface request could be lazily acquired during Channel pause,
blocking a subsequence message on that interface which should be
sent immediately while the Channel is unpaused. (confusing, yes.)

2. Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-10-04 15:21:59]

Description was changed from

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

1. The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where the
interface request could be lazily acquired during Channel pause,
blocking a subsequence message on that interface which should be
sent immediately while the Channel is unpaused. (confusing, yes.)

2. Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where the
interface request could be lazily acquired during Channel pause,
blocking a subsequence message on that interface which should be
sent immediately while the Channel is unpaused. (confusing, yes.)

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-10-04 15:22:57]

Description was changed from

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where the
interface request could be lazily acquired during Channel pause,
blocking a subsequence message on that interface which should be
sent immediately while the Channel is unpaused. (confusing, yes.)

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where one of
these interface requests could be lazily acquired during Channel
pause, blocking a subsequent message on that interface which
should be sent immediately while the Channel is unpaused.

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-10-04 15:43:34]

Description was changed from

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where one of
these interface requests could be lazily acquired during Channel
pause, blocking a subsequent message on that interface which
should be sent immediately while the Channel is unpaused.

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where one of
these interface requests could be lazily acquired during Channel
pause, blocking a subsequent message on that interface which
should be sent immediately while the Channel is unpaused.
See https://goo.gl/ot0S32 for some explanation of this behavior.

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Tom Sepez, 2016-10-04 20:09:38]

messages/mojom LGTM.

[Ken Rockot(use gerrit already), 2016-10-04 20:10:09]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-10-04 20:10:09]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2383853003/#ps100001
(title: "Move FrameMsg_NewFrame/NewFrameProxy to mojom")

[commit-bot: I haz the power, 2016-10-04 20:10:49]

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, 2016-10-04 20:17:30]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-04 20:20:43]

Description was changed from

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where one of
these interface requests could be lazily acquired during Channel
pause, blocking a subsequent message on that interface which
should be sent immediately while the Channel is unpaused.
See https://goo.gl/ot0S32 for some explanation of this behavior.

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move FrameMsg_NewFrame/NewFrameProxy to mojom

Straightforward conversion, no surprises.

Also changes some (re-)initialization details of RPHI. Namely:

* The mojom::RouteProvider and mojom::Renderer associated
interface proxies are acquired immediately upon Channel creation,
before pausing the Channel. This avoids an issue where one of
these interface requests could be lazily acquired during Channel
pause, blocking a subsequent message on that interface which
should be sent immediately while the Channel is unpaused.
See https://goo.gl/ot0S32 for some explanation of this behavior.

* Any cached associated interface proxies are reset in
ProcessDied() rather than Init(), per nasko@'s suggestion.
This clarifies that they are per-process state.

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/53be7caf3a84b07c4246c5c250ccac629113db9c
Cr-Commit-Position: refs/heads/master@{#422900}
==========

[commit-bot: I haz the power, 2016-10-04 20:20:45]

Patchset 5 (id:??) landed as
https://crrev.com/53be7caf3a84b07c4246c5c250ccac629113db9c
Cr-Commit-Position: refs/heads/master@{#422900}

[jam, 2016-10-05 16:12:57]

drive-by: just saw the new way of getting access to this method to accommodate
MockRPH. The API is quite cumbersome and unlike any other method on RPH

question: is this code path actually reached with MockRPH? What we do in these
cases is just put it on RPHImpl then. The reason that MockRPH implements the
public interface was that when we introduced the RPH interface we didn't want to
introduce yet another class that the impl and mock derive from. In situations
where we do need a method to be implemented on MockRPH, the pattern we've used
is to just add it to the public interface as a compromise.

[Ken Rockot(use gerrit already), 2016-10-05 16:18:32]

On Wed, Oct 5, 2016 at 9:12 AM, <jam@chromium.org> wrote:

> drive-by: just saw the new way of getting access to this method to
> accommodate
> MockRPH. The API is quite cumbersome and unlike any other method on RPH
>

Agreed, pretty cumbersome. But it's not unlike other RPH methods. There are
a few other examples of the pattern.


>
> question: is this code path actually reached with MockRPH? What we do in
> these
>

Yes, it's reached on MockRPH


> cases is just put it on RPHImpl then. The reason that MockRPH implements
> the
> public interface was that when we introduced the RPH interface we didn't
> want to
> introduce yet another class that the impl and mock derive from. In
> situations
> where we do need a method to be implemented on MockRPH, the pattern we've
> used
> is to just add it to the public interface as a compromise.
>

In this case the compromise would also include exposing the mojom itself in
content/public. This would be a significant departure from the status quo
since all of content's IPC messages are internal to content today.


>
> https://codereview.chromium.org/2383853003/
>

As a compromise, we could expose the accessors in the public API but only
forward-declare the mojom interface, so content embedders could never
practically make use of the API. I'd be happy to do that instead if you're
not opposed.

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

[jam, 2016-10-05 17:43:47]

On 2016/10/05 16:18:32, Ken Rockot wrote:
> On Wed, Oct 5, 2016 at 9:12 AM, <mailto:jam@chromium.org> wrote:
> 
> > drive-by: just saw the new way of getting access to this method to
> > accommodate
> > MockRPH. The API is quite cumbersome and unlike any other method on RPH
> >
> 
> Agreed, pretty cumbersome. But it's not unlike other RPH methods. There are
> a few other examples of the pattern.

Ah I didn't realize there were so many already. Ok I'll comment in the bug Nick
filed.

> 
> 
> >
> > question: is this code path actually reached with MockRPH? What we do in
> > these
> >
> 
> Yes, it's reached on MockRPH
> 
> 
> > cases is just put it on RPHImpl then. The reason that MockRPH implements
> > the
> > public interface was that when we introduced the RPH interface we didn't
> > want to
> > introduce yet another class that the impl and mock derive from. In
> > situations
> > where we do need a method to be implemented on MockRPH, the pattern we've
> > used
> > is to just add it to the public interface as a compromise.
> >
> 
> In this case the compromise would also include exposing the mojom itself in
> content/public. This would be a significant departure from the status quo
> since all of content's IPC messages are internal to content today.
> 
> 
> >
> > https://codereview.chromium.org/2383853003/
> >
> 
> As a compromise, we could expose the accessors in the public API but only
> forward-declare the mojom interface, so content embedders could never
> practically make use of the API. I'd be happy to do that instead if you're
> not opposed.
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.