PPAPI: Refactor renderer side of browser host creation

PPAPI: Refactor renderer side of browser host creation

This is split off from a larger CL: https://codereview.chromium.org/605593002/

This part does a few things:
- Introduce CompletedBrowserResourceHosts serialized struct. This associates the sequence number with the vector of host IDs, and makes it a little more self-documenting.
- Make RendererPpapiHost::CreateBrowserResourceHosts return the sequence number for the Create request, to aid in correlating the response with the request.
- Change ResourceConverterImpl so that rather than Bind the data for an asynchronous conversion in to a callback, it will explicitly hold a queue of pending conversions.

See this doc for more details:
https://docs.google.com/a/chromium.org/document/d/1-BSlFJi_PQIet-av5FeMlEC5_BCZyapsonUPnx--6iY/edit#
BUG=417316

[dmichael (off chromium), 2014-11-14 18:03:47]

dmichael@chromium.org changed reviewers:
+ raymes@chromium.org

[dmichael (off chromium), 2014-11-14 20:22:15]

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

[dmichael (off chromium), 2014-11-14 20:22:16]

+tsepez for ppapi/proxy/ppapi_messages.h

[Tom Sepez, 2014-11-14 20:33:58]

Messages LGTM.

https://codereview.chromium.org/730603002/diff/20001/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/730603002/diff/20001/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:433: IPC_STRUCT_TRAITS_MEMBER(sequence_num)
nit: some truly compulsive folks might want you to not abbreviate and write out
"sequence_number" instead of "sequence_num."  I'm not one of them.

[dmichael (off chromium), 2014-11-14 21:25:40]

dmichael@chromium.org changed reviewers:
+ piman@chromium.org

[dmichael (off chromium), 2014-11-14 21:25:41]

+piman for content/public/renderer/renderer_ppapi_host.h
content depending on ppapi/proxy seems OK to me, but this is the first example I
see.

[piman, 2014-11-14 21:39:13]

LGTM for content/public.

[raymes, 2014-11-17 02:48:11]

I couldn't tell, but are the sequence numbers actually used for anything
important yet?

https://codereview.chromium.org/730603002/diff/20001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/730603002/diff/20001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:55: PendingResourceCallback;
nit: ResourceHostCreationCallback might be a bit more descriptive

https://codereview.chromium.org/730603002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/resource_converter.cc (right):

https://codereview.chromium.org/730603002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/resource_converter.cc:356: hosts.sequence_num <
pending_conversions_.front().sequence_num) {
should the sequence numbers be equal?

https://codereview.chromium.org/730603002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/resource_converter.cc:360: const PendingConversion&
conversion = pending_conversions_.front();
Will FlushComplete() always come back in-order?

[raymes, 2014-11-17 02:48:30]

BTW, thanks for looking at this hairy part of the code! :)

[dmichael (off chromium), 2014-11-17 22:26:20]

Thanks for looking! Comments so far addressed.

https://codereview.chromium.org/730603002/diff/20001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/730603002/diff/20001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:55: PendingResourceCallback;
On 2014/11/17 02:48:11, raymes wrote:
> nit: ResourceHostCreationCallback might be a bit more descriptive

Good suggestion, done.

(Sorry for the churn, I ran git cl format this time after doing a couple of name
changes)

https://codereview.chromium.org/730603002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/resource_converter.cc (right):

https://codereview.chromium.org/730603002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/resource_converter.cc:356: hosts.sequence_num <
pending_conversions_.front().sequence_num) {
On 2014/11/17 02:48:11, raymes wrote:
> should the sequence numbers be equal?
For now :) I improved my check in the latest patch, thanks.

Later, when responses can come back either via the usual async reply or by the
response to GetAllPendingBrowserHosts, FlushComplete might happen twice for a
given sequence number. I.e., the browser had already sent the async reply, but
the renderer hasn't gotten it yet when it does GetAllPendingBrowserHosts. In
that case, we'll process the FlushComplete in response to
GetAllPendingBrowserHosts. Later, when we get the async reply message, we'll see
that the sequence number must have happened already, so we can ignore it.

See FlushComplete in:
https://codereview.chromium.org/605593002/diff/320001/content/renderer/pepper...

(And to answer your other question, this check is the only current use of the
sequence number. But hopefully the above comment helps explain what it will be
used for.)

https://codereview.chromium.org/730603002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/resource_converter.cc:360: const PendingConversion&
conversion = pending_conversions_.front();
On 2014/11/17 02:48:11, raymes wrote:
> Will FlushComplete() always come back in-order?
Yes. The order of async messages is guaranteed. When we make the Sync call, the
browser-side handler will also return them in order.

If you can think of a counter-example, let me know.

https://codereview.chromium.org/730603002/diff/20001/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/730603002/diff/20001/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:433: IPC_STRUCT_TRAITS_MEMBER(sequence_num)
On 2014/11/14 20:33:58, Tom Sepez wrote:
> nit: some truly compulsive folks might want you to not abbreviate and write
out
> "sequence_number" instead of "sequence_num."  I'm not one of them.
I can sympathize. Changed.

[raymes, 2014-11-18 00:17:41]

lgtm if we decide to go down this path. If we do I think it would be good to
consider how we can put some documentation in the code to explain some of the
complexity. In particular the strangeness in message ordering due to sync+async
messages - I still find it a bit hard to understand - maybe describing some
concrete scenarios would help.

Thanks again for looking at this!