Moves FrameHostMsg_SetCookie to a Mojo message

Moves FrameHostMsg_SetCookie to a Mojo message

Adds a new BrowserAssociatedInterface to RenderFrameMessageFilter
and converts a single message (SetCookie) to Mojo.

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

Committed: https://crrev.com/1587e33c1578c277af84c700bd213f5743f09f18
Cr-Commit-Position: refs/heads/master@{#408173}

[Ken Rockot(use gerrit already), 2016-07-19 22:42:58]

Description was changed from

==========
Moves FrameHostMsg_SetCookie to a Mojo message

Adds a new BrowserAssociatedInterface to RenderFrameMessageFilter
and converts a single message (SetCookie) to Mojo.

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

to

==========
Moves FrameHostMsg_SetCookie to a Mojo message

Adds a new BrowserAssociatedInterface to RenderFrameMessageFilter
and converts a single message (SetCookie) to Mojo.

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

[Ken Rockot(use gerrit already), 2016-07-19 22:43:04]

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

[commit-bot: I haz the power, 2016-07-19 22:43: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-07-19 22:49:33]

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

[commit-bot: I haz the power, 2016-07-19 22:50:06]

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-07-19 22:50:33]

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

[commit-bot: I haz the power, 2016-07-19 22:51:08]

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-07-19 22:58:49]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-07-19 23:03:03]

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

[commit-bot: I haz the power, 2016-07-19 23:03:04]

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...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Rockot(use gerrit already), 2016-07-20 23:57:03]

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

[commit-bot: I haz the power, 2016-07-20 23:57: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, 2016-07-21 00:09:13]

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

[commit-bot: I haz the power, 2016-07-21 00:09:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-07-22 20:04:57]

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

[commit-bot: I haz the power, 2016-07-22 20:05: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, 2016-07-22 20:12:53]

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

[commit-bot: I haz the power, 2016-07-22 20:12:55]

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

[Ken Rockot(use gerrit already), 2016-07-25 21:25:45]

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

[commit-bot: I haz the power, 2016-07-25 21:26:19]

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-07-25 21:28:00]

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

[Ken Rockot(use gerrit already), 2016-07-25 21:28:00]

As discussed, this exposes the filter through RPHI. Note that
RenderFrameMessageFilter needs to be destroyed on the IO thread, so RPHI is
forced to clear its reference before resetting the ChannelProxy. A little weird,
but I didn't like the alternatives either. WDYT?

[commit-bot: I haz the power, 2016-07-25 21:52:22]

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

[commit-bot: I haz the power, 2016-07-25 21:52:23]

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

[jam, 2016-07-26 17:13:38]

On 2016/07/25 21:28:00, Ken Rockot wrote:
> As discussed, this exposes the filter through RPHI. Note that
> RenderFrameMessageFilter needs to be destroyed on the IO thread, so RPHI is
> forced to clear its reference before resetting the ChannelProxy. A little
weird,
> but I didn't like the alternatives either. WDYT?

There are helper methods to enforce this, a BMF can override OnDestruct and call
BrowserThread::DeleteOnIOThread::Destruct(this);
i.e. see
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...

[Ken Rockot(use gerrit already), 2016-07-26 18:01:33]

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

[Ken Rockot(use gerrit already), 2016-07-26 18:01:45]

On 2016/07/26 at 17:13:38, jam wrote:
> On 2016/07/25 21:28:00, Ken Rockot wrote:
> > As discussed, this exposes the filter through RPHI. Note that
> > RenderFrameMessageFilter needs to be destroyed on the IO thread, so RPHI is
> > forced to clear its reference before resetting the ChannelProxy. A little
weird,
> > but I didn't like the alternatives either. WDYT?
> 
> There are helper methods to enforce this, a BMF can override OnDestruct and
call
> BrowserThread::DeleteOnIOThread::Destruct(this);
> i.e. see
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...

Aha. Done.

[commit-bot: I haz the power, 2016-07-26 18:02:15]

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-07-26 19:03:16]

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

[commit-bot: I haz the power, 2016-07-26 19:03:17]

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

[jam, 2016-07-26 19:13:33]

https://codereview.chromium.org/2167513003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2167513003/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:250:
RenderFrameMessageFilter* render_frame_message_filter() const {
nit: add a _for_testing suffix since it's only called from a test.

https://codereview.chromium.org/2167513003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2167513003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1457:
RenderThread::Get()->GetChannel()->GetRemoteAssociatedInterface(
it's a bit odd that this mojo interface is registered on the process host object
on the browser side, but on the renderer it's fetched via the frame object. it
seems that there should be symmetry.

any reason why we can't make a getter for this on the render thread class? that
way we'd only create one MP for this, which I think is all we need.

[Ken Rockot(use gerrit already), 2016-07-26 20:52:01]

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

[commit-bot: I haz the power, 2016-07-26 20:52:35]

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-07-26 20:58:15]

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

[Ken Rockot(use gerrit already), 2016-07-26 20:58:21]

Patchset #7 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-07-26 20:58:50]

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-07-26 21:00:34]

https://codereview.chromium.org/2167513003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2167513003/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:250:
RenderFrameMessageFilter* render_frame_message_filter() const {
On 2016/07/26 at 19:13:32, jam wrote:
> nit: add a _for_testing suffix since it's only called from a test.

Done

https://codereview.chromium.org/2167513003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2167513003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1457:
RenderThread::Get()->GetChannel()->GetRemoteAssociatedInterface(
On 2016/07/26 at 19:13:32, jam wrote:
> it's a bit odd that this mojo interface is registered on the process host
object on the browser side, but on the renderer it's fetched via the frame
object. it seems that there should be symmetry.
> 
> any reason why we can't make a getter for this on the render thread class?
that way we'd only create one MP for this, which I think is all we need.

Done

[commit-bot: I haz the power, 2016-07-26 22:08:58]

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

[commit-bot: I haz the power, 2016-07-26 22:08:59]

Dry run: This issue passed the CQ dry run.

[jam, 2016-07-26 23:38:08]

lgtm

[Ken Rockot(use gerrit already), 2016-07-26 23:44:58]

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

[Ken Rockot(use gerrit already), 2016-07-26 23:44:59]

+tsepez@ for mojom and messages

[Tom Sepez, 2016-07-27 17:35:30]

https://codereview.chromium.org/2167513003/diff/160001/content/common/render_...
File content/common/render_frame_message_filter.mojom (right):

https://codereview.chromium.org/2167513003/diff/160001/content/common/render_...
content/common/render_frame_message_filter.mojom:11: // any subsequent
FrameHostMsg_GetCookies request.
Wouldn't we also have to move GetCookies to mojom as well to ensure we don't
race against the IPC impl?  Curious about how we ensure this.  Thanks.

[Ken Rockot(use gerrit already), 2016-07-27 17:37:03]

On 2016/07/27 at 17:35:30, tsepez wrote:
>
https://codereview.chromium.org/2167513003/diff/160001/content/common/render_...
> File content/common/render_frame_message_filter.mojom (right):
> 
>
https://codereview.chromium.org/2167513003/diff/160001/content/common/render_...
> content/common/render_frame_message_filter.mojom:11: // any subsequent
FrameHostMsg_GetCookies request.
> Wouldn't we also have to move GetCookies to mojom as well to ensure we don't
race against the IPC impl?  Curious about how we ensure this.  Thanks.

That is in fact the purpose of BrowserAssociatedInterface - the mojo interface
in this case is actually sharing the IPC Channel's pipe, so ordering is
guaranteed between any such interface and legacy IPC messages.

[Tom Sepez, 2016-07-27 17:38:35]

On 2016/07/27 17:37:03, Ken Rockot wrote:
> On 2016/07/27 at 17:35:30, tsepez wrote:
> >
>
https://codereview.chromium.org/2167513003/diff/160001/content/common/render_...
> > File content/common/render_frame_message_filter.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2167513003/diff/160001/content/common/render_...
> > content/common/render_frame_message_filter.mojom:11: // any subsequent
> FrameHostMsg_GetCookies request.
> > Wouldn't we also have to move GetCookies to mojom as well to ensure we don't
> race against the IPC impl?  Curious about how we ensure this.  Thanks.
> 
> That is in fact the purpose of BrowserAssociatedInterface - the mojo interface
> in this case is actually sharing the IPC Channel's pipe, so ordering is
> guaranteed between any such interface and legacy IPC messages.

Ah, gotcha, LGTM.

[Ken Rockot(use gerrit already), 2016-07-27 17:39:20]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-07-27 17:39:39]

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-07-27 17:44:28]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-07-27 17:45:47]

Description was changed from

==========
Moves FrameHostMsg_SetCookie to a Mojo message

Adds a new BrowserAssociatedInterface to RenderFrameMessageFilter
and converts a single message (SetCookie) to Mojo.

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

to

==========
Moves FrameHostMsg_SetCookie to a Mojo message

Adds a new BrowserAssociatedInterface to RenderFrameMessageFilter
and converts a single message (SetCookie) to Mojo.

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

Committed: https://crrev.com/1587e33c1578c277af84c700bd213f5743f09f18
Cr-Commit-Position: refs/heads/master@{#408173}
==========

[commit-bot: I haz the power, 2016-07-27 17:45:48]

Patchset 7 (id:??) landed as
https://crrev.com/1587e33c1578c277af84c700bd213f5743f09f18
Cr-Commit-Position: refs/heads/master@{#408173}