Make SyncLoad result handling pluggable

Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

BUG=603396
Committed: https://crrev.com/1c895912ab579d514c7e1c03c0121dff543ce507
Cr-Commit-Position: refs/heads/master@{#423840}

[tzik, 2016-10-04 16:36:08]

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

[commit-bot: I haz the power, 2016-10-04 16:36:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-10-04 16:50:59]

Description was changed from

==========
Make SyncLoad result handling pluggable

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

to

==========
Make SyncLoad result handling pluggable

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

[tzik, 2016-10-04 16:50:59]

tzik@chromium.org changed reviewers:
+ yhirano@chromium.org

[tzik, 2016-10-04 16:51:20]

Description was changed from

==========
Make SyncLoad result handling pluggable

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

to

==========
Make SyncLoad result handling pluggable

WIP

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

[commit-bot: I haz the power, 2016-10-04 17:23:51]

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

[commit-bot: I haz the power, 2016-10-04 17:23:52]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-10-05 03:58:34]

Description was changed from

==========
Make SyncLoad result handling pluggable

WIP

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

to

==========
Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

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

[tzik, 2016-10-05 03:58:34]

tzik@chromium.org changed reviewers:
+ mmenke@chromium.org

[tzik, 2016-10-05 04:03:18]

Description was changed from

==========
Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

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

to

==========
Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

BUG=603396
==========

[tzik, 2016-10-05 04:03:25]

PTAL

[yhirano, 2016-10-05 08:49:53]

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.h:21: 
+base/callback.h

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
File content/browser/loader/sync_resource_handler.cc (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
content/browser/loader/sync_resource_handler.cc:35:
result_handler_.Run(nullptr);
This nullptr is corresponding to L427 in resource_dispatcher_host_impl.cc but
its relationship is not obvious. Some comments in ResourceDispatcherHostImpl
would be helpful, I think.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
content/browser/loader/sync_resource_handler.cc:111: ResourceMessageFilter*
filter = GetFilter();
This is not used.

[mmenke, 2016-10-05 15:21:34]

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:418: void
HandleSyncLoadResult(base::WeakPtr<ResourceMessageFilter> filter,
Need to document this.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:427:
sync_result->set_reply_error();
nit:  Use braces with if else

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1091:
SyncLoadResultCallback cb = base::Bind(
I'd suggest just writing out callback.  (I consider cb a not too common
abbreviation, at least in loader/)

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1093:
base::Passed(WrapUnique(sync_result)));
This doesn't seem legal.  We don't own sync_result (Or at least the old code
doesn't delete it, even when it doesn't send the message), so I don't think we
can legally wrap it in a sync pointer, unless the old code was actually leaking
it.

The API docs aren't clear, but I think we just need to keep around the raw
pointer?  Don't ask me what we should do if the filter is deleted.  This seems
underspecified to me.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1420: bool is_sync_load
= !sync_result_handler.is_null();
Higher up, you just rely on implicit conversion of SyncLoadResultCallback to a
bool.  Should be consistent.  Either remove the is_null() here, or add it
further up.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.h:88:
base::Callback<void(const SyncLoadResult* result)>;
Need to document this.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
File content/browser/loader/sync_resource_handler.cc (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
content/browser/loader/sync_resource_handler.cc:122: result_handler_.Reset();
base::ResetAndReturn(result_handler_).Run?

[kinuko, 2016-10-05 15:52:57]

(drive-by)

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1093:
base::Passed(WrapUnique(sync_result)));
On 2016/10/05 15:21:33, mmenke wrote:
> This doesn't seem legal.  We don't own sync_result (Or at least the old code
> doesn't delete it, even when it doesn't send the message), so I don't think we
> can legally wrap it in a sync pointer, unless the old code was actually
leaking
> it.
> 
> The API docs aren't clear, but I think we just need to keep around the raw
> pointer?  Don't ask me what we should do if the filter is deleted.  This seems
> underspecified to me.

It looks we could also just copy the message by value (*sync_result) and let the
callback hold it too, which might make the code feel safer (adding callback
indirection makes its lifetime obscure).

[mmenke, 2016-10-05 16:31:27]

On 2016/10/05 15:52:57, kinuko (slow) wrote:
> (drive-by)
> 
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> content/browser/loader/resource_dispatcher_host_impl.cc:1093:
> base::Passed(WrapUnique(sync_result)));
> On 2016/10/05 15:21:33, mmenke wrote:
> > This doesn't seem legal.  We don't own sync_result (Or at least the old code
> > doesn't delete it, even when it doesn't send the message), so I don't think
we
> > can legally wrap it in a sync pointer, unless the old code was actually
> leaking
> > it.
> > 
> > The API docs aren't clear, but I think we just need to keep around the raw
> > pointer?  Don't ask me what we should do if the filter is deleted.  This
seems
> > underspecified to me.
> 
> It looks we could also just copy the message by value (*sync_result) and let
the
> callback hold it too, which might make the code feel safer (adding callback
> indirection makes its lifetime obscure).

If we copy it, we leak the original...don't we?  Someone may have to delve into
the IPC code to figure out just what's going on.

[kinuko, 2016-10-05 18:43:01]

On 2016/10/05 16:31:27, mmenke wrote:
> On 2016/10/05 15:52:57, kinuko (slow) wrote:
> > (drive-by)
> > 
> >
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> > content/browser/loader/resource_dispatcher_host_impl.cc:1093:
> > base::Passed(WrapUnique(sync_result)));
> > On 2016/10/05 15:21:33, mmenke wrote:
> > > This doesn't seem legal.  We don't own sync_result (Or at least the old
code
> > > doesn't delete it, even when it doesn't send the message), so I don't
think
> we
> > > can legally wrap it in a sync pointer, unless the old code was actually
> > leaking
> > > it.
> > > 
> > > The API docs aren't clear, but I think we just need to keep around the raw
> > > pointer?  Don't ask me what we should do if the filter is deleted.  This
> seems
> > > underspecified to me.
> > 
> > It looks we could also just copy the message by value (*sync_result) and let
> the
> > callback hold it too, which might make the code feel safer (adding callback
> > indirection makes its lifetime obscure).
> 
> If we copy it, we leak the original...don't we?  Someone may have to delve
into
> the IPC code to figure out just what's going on.

This method is called from OnMessageReceived, am I right?  The original message
is passed as const ref to OnMessageReceived (so the original body is held by the
caller), and handler code given to the IPC macro is called with '&' added to the
const ref parameter.  So I don't think it'll leak.

The IPC macros are expanded like following:

  ...
  const IPC::Message& ipc_message__ = msg;
  ...
  if (!msg_class::DispatchDelayReply(&ipc_message__, obj, param__,
&member_func))
    ...

[mmenke, 2016-10-05 19:46:22]

On 2016/10/05 18:43:01, kinuko (slow) wrote:
> On 2016/10/05 16:31:27, mmenke wrote:
> > On 2016/10/05 15:52:57, kinuko (slow) wrote:
> > > (drive-by)
> > > 
> > >
> >
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> > > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> > > content/browser/loader/resource_dispatcher_host_impl.cc:1093:
> > > base::Passed(WrapUnique(sync_result)));
> > > On 2016/10/05 15:21:33, mmenke wrote:
> > > > This doesn't seem legal.  We don't own sync_result (Or at least the old
> code
> > > > doesn't delete it, even when it doesn't send the message), so I don't
> think
> > we
> > > > can legally wrap it in a sync pointer, unless the old code was actually
> > > leaking
> > > > it.
> > > > 
> > > > The API docs aren't clear, but I think we just need to keep around the
raw
> > > > pointer?  Don't ask me what we should do if the filter is deleted.  This
> > seems
> > > > underspecified to me.
> > > 
> > > It looks we could also just copy the message by value (*sync_result) and
let
> > the
> > > callback hold it too, which might make the code feel safer (adding
callback
> > > indirection makes its lifetime obscure).
> > 
> > If we copy it, we leak the original...don't we?  Someone may have to delve
> into
> > the IPC code to figure out just what's going on.
> 
> This method is called from OnMessageReceived, am I right?  The original
message
> is passed as const ref to OnMessageReceived (so the original body is held by
the
> caller), and handler code given to the IPC macro is called with '&' added to
the
> const ref parameter.  So I don't think it'll leak.
> 
> The IPC macros are expanded like following:
> 
>   ...
>   const IPC::Message& ipc_message__ = msg;
>   ...
>   if (!msg_class::DispatchDelayReply(&ipc_message__, obj, param__,
> &member_func))
>     ...

If that's the case, the old code looks like it only copied the pointer, never
the contents of it, and then passed it back to filter asynchronously...Why was
the old code correct?

[kinuko, 2016-10-05 20:57:02]

On 2016/10/05 19:46:22, mmenke wrote:
> On 2016/10/05 18:43:01, kinuko (slow) wrote:
> > On 2016/10/05 16:31:27, mmenke wrote:
> > > On 2016/10/05 15:52:57, kinuko (slow) wrote:
> > > > (drive-by)
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> > > > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
> > > > content/browser/loader/resource_dispatcher_host_impl.cc:1093:
> > > > base::Passed(WrapUnique(sync_result)));
> > > > On 2016/10/05 15:21:33, mmenke wrote:
> > > > > This doesn't seem legal.  We don't own sync_result (Or at least the
old
> > code
> > > > > doesn't delete it, even when it doesn't send the message), so I don't
> > think
> > > we
> > > > > can legally wrap it in a sync pointer, unless the old code was
actually
> > > > leaking
> > > > > it.
> > > > > 
> > > > > The API docs aren't clear, but I think we just need to keep around the
> raw
> > > > > pointer?  Don't ask me what we should do if the filter is deleted. 
This
> > > seems
> > > > > underspecified to me.
> > > > 
> > > > It looks we could also just copy the message by value (*sync_result) and
> let
> > > the
> > > > callback hold it too, which might make the code feel safer (adding
> callback
> > > > indirection makes its lifetime obscure).
> > > 
> > > If we copy it, we leak the original...don't we?  Someone may have to delve
> > into
> > > the IPC code to figure out just what's going on.
> > 
> > This method is called from OnMessageReceived, am I right?  The original
> message
> > is passed as const ref to OnMessageReceived (so the original body is held by
> the
> > caller), and handler code given to the IPC macro is called with '&' added to
> the
> > const ref parameter.  So I don't think it'll leak.
> > 
> > The IPC macros are expanded like following:
> > 
> >   ...
> >   const IPC::Message& ipc_message__ = msg;
> >   ...
> >   if (!msg_class::DispatchDelayReply(&ipc_message__, obj, param__,
> > &member_func))
> >     ...
> 
> If that's the case, the old code looks like it only copied the pointer, never
> the contents of it, and then passed it back to filter asynchronously...Why was
> the old code correct?

Oops right... yep it gets deleted in Send, and the pointer that the method is
given is not the one OnMessageReceived is given.  Hmm... then it feels wrapping
into unique ptr could be probably ok?

[kinuko, 2016-10-05 20:57:48]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-10-05 20:57:49]

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1093:
base::Passed(WrapUnique(sync_result)));
On 2016/10/05 15:52:57, kinuko (slow) wrote:
> On 2016/10/05 15:21:33, mmenke wrote:
> > This doesn't seem legal.  We don't own sync_result (Or at least the old code
> > doesn't delete it, even when it doesn't send the message), so I don't think
we
> > can legally wrap it in a sync pointer, unless the old code was actually
> leaking
> > it.
> > 
> > The API docs aren't clear, but I think we just need to keep around the raw
> > pointer?  Don't ask me what we should do if the filter is deleted.  This
seems
> > underspecified to me.
> 
> It looks we could also just copy the message by value (*sync_result) and let
the
> callback hold it too, which might make the code feel safer (adding callback
> indirection makes its lifetime obscure).

^^ sorry please ignore this comment, that was bogus

[tzik, 2016-10-06 12:52:03]

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:418: void
HandleSyncLoadResult(base::WeakPtr<ResourceMessageFilter> filter,
On 2016/10/05 15:21:33, mmenke wrote:
> Need to document this.

Done.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:427:
sync_result->set_reply_error();
On 2016/10/05 15:21:33, mmenke wrote:
> nit:  Use braces with if else

Done.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1091:
SyncLoadResultCallback cb = base::Bind(
On 2016/10/05 15:21:33, mmenke wrote:
> I'd suggest just writing out callback.  (I consider cb a not too common
> abbreviation, at least in loader/)

Done.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1093:
base::Passed(WrapUnique(sync_result)));
On 2016/10/05 20:57:49, kinuko (slow) wrote:
> On 2016/10/05 15:52:57, kinuko (slow) wrote:
> > On 2016/10/05 15:21:33, mmenke wrote:
> > > This doesn't seem legal.  We don't own sync_result (Or at least the old
code
> > > doesn't delete it, even when it doesn't send the message), so I don't
think
> we
> > > can legally wrap it in a sync pointer, unless the old code was actually
> > leaking
> > > it.
> > > 
> > > The API docs aren't clear, but I think we just need to keep around the raw
> > > pointer?  Don't ask me what we should do if the filter is deleted.  This
> seems
> > > underspecified to me.
> > 
> > It looks we could also just copy the message by value (*sync_result) and let
> the
> > callback hold it too, which might make the code feel safer (adding callback
> > indirection makes its lifetime obscure).
> 
> ^^ sorry please ignore this comment, that was bogus

This is called by MessageT<>::DispatchDelayReply via OnMessageReceived.
https://cs.chromium.org/chromium/src/ipc/ipc_message_templates.h?sq=package:c...

|sync_result| is created as |reply| by SyncMessage::GenerateReply in MT::DDR
above, and eventually deleted in Send().
So, I think the old code is leaky and it's safe to store it in std::unique_ptr.
The IPC system implicitly passes the ownership of it.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1420: bool is_sync_load
= !sync_result_handler.is_null();
On 2016/10/05 15:21:33, mmenke wrote:
> Higher up, you just rely on implicit conversion of SyncLoadResultCallback to a
> bool.  Should be consistent.  Either remove the is_null() here, or add it
> further up.

That is an implicit use of an explicit conversion operator :p).
To use it here, we need explicit static_cast<bool>(result_handler) or
!!result_handler.

Updated it to `!!result_handler` as it looks cleaner to me.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.h:21: 
On 2016/10/05 08:49:53, yhirano wrote:
> +base/callback.h

Done. Added base/callback_forward.h.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.h:88:
base::Callback<void(const SyncLoadResult* result)>;
On 2016/10/05 15:21:33, mmenke wrote:
> Need to document this.

Done.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
File content/browser/loader/sync_resource_handler.cc (right):

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
content/browser/loader/sync_resource_handler.cc:35:
result_handler_.Run(nullptr);
On 2016/10/05 08:49:53, yhirano wrote:
> This nullptr is corresponding to L427 in resource_dispatcher_host_impl.cc but
> its relationship is not obvious. Some comments in ResourceDispatcherHostImpl
> would be helpful, I think.

Done.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
content/browser/loader/sync_resource_handler.cc:111: ResourceMessageFilter*
filter = GetFilter();
On 2016/10/05 08:49:53, yhirano wrote:
> This is not used.

Done.

https://codereview.chromium.org/2390313002/diff/1/content/browser/loader/sync...
content/browser/loader/sync_resource_handler.cc:122: result_handler_.Reset();
On 2016/10/05 15:21:33, mmenke wrote:
> base::ResetAndReturn(result_handler_).Run?

Done.

[tzik, 2016-10-06 12:52:31]

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

[commit-bot: I haz the power, 2016-10-06 12:52:41]

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-06 13:00:57]

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

[commit-bot: I haz the power, 2016-10-06 13:00:58]

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

[tzik, 2016-10-06 14:41:30]

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

[tzik, 2016-10-06 14:41:38]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-10-06 14:41:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-10-06 15:16:48]

LGTM

https://codereview.chromium.org/2390313002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2390313002/diff/60001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:153: const
scoped_refptr<ResourceResponse>& response);
include base/memory/ref_counted.h

[commit-bot: I haz the power, 2016-10-06 15:27:51]

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

[commit-bot: I haz the power, 2016-10-06 15:27:52]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-10-06 15:58:05]

kinuko@chromium.org changed reviewers:
- kinuko@chromium.org

[tzik, 2016-10-06 16:08:27]

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

[commit-bot: I haz the power, 2016-10-06 16:08:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-10-06 16:08:54]

https://codereview.chromium.org/2390313002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2390313002/diff/60001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:153: const
scoped_refptr<ResourceResponse>& response);
On 2016/10/06 15:16:48, mmenke wrote:
> include base/memory/ref_counted.h

Done.

[tzik, 2016-10-06 16:09:14]

tzik@chromium.org changed reviewers:
+ kinuko@chromium.org

[commit-bot: I haz the power, 2016-10-06 16:57:35]

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

[commit-bot: I haz the power, 2016-10-06 16:57:36]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-10-06 17:46:51]

kinuko@chromium.org changed reviewers:
- kinuko@chromium.org

[yhirano, 2016-10-07 10:56:41]

lgtm

[tzik, 2016-10-07 12:31:01]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-10-07 12:31:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2390313002/#ps80001
(title: "+#include "b/m/ref_counted.h"")

[commit-bot: I haz the power, 2016-10-07 12:31:08]

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-07 12:37:06]

Description was changed from

==========
Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

BUG=603396
==========

to

==========
Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

BUG=603396
==========

[commit-bot: I haz the power, 2016-10-07 12:37:07]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-07 12:39:34]

Description was changed from

==========
Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

BUG=603396
==========

to

==========
Make SyncLoad result handling pluggable

Use a base::Callback to dispatch a SyncLoad result instead of
IPC::Message, so that we can share most of synchronous load code between
Chrome IPC version and Mojo IPC version.

BUG=603396

Committed: https://crrev.com/1c895912ab579d514c7e1c03c0121dff543ce507
Cr-Commit-Position: refs/heads/master@{#423840}
==========

[commit-bot: I haz the power, 2016-10-07 12:39:35]

Patchset 4 (id:??) landed as
https://crrev.com/1c895912ab579d514c7e1c03c0121dff543ce507
Cr-Commit-Position: refs/heads/master@{#423840}