Refactor some ResourceMessageReply usages.

Refactor some ResourceMessageReply usages.
Add an is_valid method to ppapi::host::ReplyMessageContext.
change scoped_ptr<ReplyMessageContext> to ReplyMessageContext in multiple sites and use is_valid to detect "null" ones.
Hand rewrite some of the previous factoring in PepperExtensionsCommonMessageFilter.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263730

[dmichael (off chromium), 2014-04-11 18:11:22]

Looks like a nice simplification for a couple of these, thanks!

I'm not sure if the bool+ReplyMessageContext is better than
scoped_ptr<ReplyMessageContext>. Maybe ReplyMessageContext can have a valid or
invalid state so you don't need the bool?

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc
(right):

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc:149:
base::Bind(&PepperExtensionsCommonMessageFilter::OnPostCompleted, this));
Should this just use Dispatch? It doesn't seem worth the extra task+context
switch in order to do a couple of DCHECKs.

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.h (right):

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.h:66:
ppapi::host::ReplyMessageContext reply_context,
const &?

https://codereview.chromium.org/231883002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_audio_input_host.cc (right):

https://codereview.chromium.org/231883002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_audio_input_host.cc:178:
open_context_.params.AppendHandle(serialized_shared_memory_handle);
What about adding an is_valid to the ReplyMessageContext? I notice that the
constructor doesn't initialize anything. If it did, a simple little is_valid()
or is_initialized() function could check the params to see if they have the
default values.

As is, it seems like using a scoped_ptr is actually simpler in this particular
Host.

[bbudge, 2014-04-11 20:41:18]

This includes the RMC is_valid() patch for now.

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc
(right):

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc:149:
base::Bind(&PepperExtensionsCommonMessageFilter::OnPostCompleted, this));
On 2014/04/11 18:11:23, dmichael wrote:
> Should this just use Dispatch? It doesn't seem worth the extra task+context
> switch in order to do a couple of DCHECKs.

It turns out that Dispatch needs a RenderViewHost instead of a RenderFrameHost
(there's a note to switch to RenderFrameHost as part of a refactoring project.)
So it would require some contortions to use Dispatch here unfortunately.

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.h (right):

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.h:66:
ppapi::host::ReplyMessageContext reply_context,
On 2014/04/11 18:11:23, dmichael wrote:
> const &?

Done. However it requires making a copy so I can call set_result on the params.
I see precedents in the code but it seems a bit fishy.

https://codereview.chromium.org/231883002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_audio_input_host.cc (right):

https://codereview.chromium.org/231883002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_audio_input_host.cc:178:
open_context_.params.AppendHandle(serialized_shared_memory_handle);
On 2014/04/11 18:11:23, dmichael wrote:
> What about adding an is_valid to the ReplyMessageContext? I notice that the
> constructor doesn't initialize anything. If it did, a simple little is_valid()
> or is_initialized() function could check the params to see if they have the
> default values.
> 
> As is, it seems like using a scoped_ptr is actually simpler in this particular
> Host.

I thought about this. I think it's a great idea so I created another CL for it.

[dmichael (off chromium), 2014-04-11 20:56:13]

lgtm

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc
(right):

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc:149:
base::Bind(&PepperExtensionsCommonMessageFilter::OnPostCompleted, this));
On 2014/04/11 20:41:19, bbudge wrote:
> On 2014/04/11 18:11:23, dmichael wrote:
> > Should this just use Dispatch? It doesn't seem worth the extra task+context
> > switch in order to do a couple of DCHECKs.
> 
> It turns out that Dispatch needs a RenderViewHost instead of a RenderFrameHost
> (there's a note to switch to RenderFrameHost as part of a refactoring
project.)
> So it would require some contortions to use Dispatch here unfortunately.
dispatcher_owner_->render_frame_host()->GetRenderViewHost()
?

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.h (right):

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.h:66:
ppapi::host::ReplyMessageContext reply_context,
On 2014/04/11 20:41:19, bbudge wrote:
> On 2014/04/11 18:11:23, dmichael wrote:
> > const &?
> 
> Done. However it requires making a copy so I can call set_result on the
params.
> I see precedents in the code but it seems a bit fishy.
Oh, right. I think either way is fine, then. Makes no practical difference.

https://codereview.chromium.org/231883002/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/pepper_device_enumeration_host_helper.cc (right):

https://codereview.chromium.org/231883002/diff/60001/content/renderer/pepper/...
content/renderer/pepper/pepper_device_enumeration_host_helper.cc:180:
enumerate_devices_context_ = ppapi::host::ReplyMessageContext();
optional: you could alternatively have a "clear()" method.

[bbudge, 2014-04-11 22:46:05]

I'll just check this combined patch instead of breaking it in two (for the
is_valid() part).

Thanks for looking.

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc
(right):

https://codereview.chromium.org/231883002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_extensions_common_message_filter.cc:149:
base::Bind(&PepperExtensionsCommonMessageFilter::OnPostCompleted, this));
On 2014/04/11 20:56:14, dmichael wrote:
> On 2014/04/11 20:41:19, bbudge wrote:
> > On 2014/04/11 18:11:23, dmichael wrote:
> > > Should this just use Dispatch? It doesn't seem worth the extra
task+context
> > > switch in order to do a couple of DCHECKs.
> > 
> > It turns out that Dispatch needs a RenderViewHost instead of a
RenderFrameHost
> > (there's a note to switch to RenderFrameHost as part of a refactoring
> project.)
> > So it would require some contortions to use Dispatch here unfortunately.
> dispatcher_owner_->render_frame_host()->GetRenderViewHost()
> ?

I was too lazy to look it up!
Done.

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_extensions_common_host.h (right):

https://codereview.chromium.org/231883002/diff/40001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_extensions_common_host.h:66:
ppapi::host::ReplyMessageContext reply_context,
On 2014/04/11 20:56:14, dmichael wrote:
> On 2014/04/11 20:41:19, bbudge wrote:
> > On 2014/04/11 18:11:23, dmichael wrote:
> > > const &?
> > 
> > Done. However it requires making a copy so I can call set_result on the
> params.
> > I see precedents in the code but it seems a bit fishy.
> Oh, right. I think either way is fine, then. Makes no practical difference.

Went back to non-const as it's less surprising and less code.

https://codereview.chromium.org/231883002/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/pepper_device_enumeration_host_helper.cc (right):

https://codereview.chromium.org/231883002/diff/60001/content/renderer/pepper/...
content/renderer/pepper/pepper_device_enumeration_host_helper.cc:180:
enumerate_devices_context_ = ppapi::host::ReplyMessageContext();
On 2014/04/11 20:56:14, dmichael wrote:
> optional: you could alternatively have a "clear()" method. 

Going through RMC usage and copying sounds like a good Noogler project in
Pepper.

[bbudge, 2014-04-14 19:15:09]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-04-14 19:15:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/231883002/100001

[commit-bot: I haz the power, 2014-04-14 21:51:59]

Change committed as 263730