Implement asynchronous interface/plumbing for GetDefaultPrintSettings.

Implement asynchronous interface/plumbing for GetDefaultPrintSettings. Previously we had a synchronous interface but that implementation had problems, so now we need an asynchronous interface. This is needed for Flash. 

The implementation of getting the print settings and a test will come in the next CL.

BUG=138333
TEST=none

[raymes, 2012-07-20 22:46:56]

https://chromiumcodereview.appspot.com/10795051/diff/21/ppapi/api/dev/ppp_pri...
File ppapi/api/dev/ppp_printing_dev.idl (right):

https://chromiumcodereview.appspot.com/10795051/diff/21/ppapi/api/dev/ppp_pri...
ppapi/api/dev/ppp_printing_dev.idl:13: M23 = 0.7
Trung mentioned that this interface version should always match the ppb
interface version. I'm not certain how to guarantee that though (just having the
label here doesn't guarantee the interface is generated)

[dmichael (off chromium), 2012-07-23 18:20:09]

http://codereview.chromium.org/10795051/diff/21/content/browser/renderer_host...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

http://codereview.chromium.org/10795051/diff/21/content/browser/renderer_host...
content/browser/renderer_host/pepper/pepper_message_filter.cc:725:
PP_PrintSettings_Dev settings;
Previously, you filled in some reasonable defaults. It seems like this might
break some stuff if you don't fill in the same defaults here for now (until you
hook it up for real).

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppb_printing_de...
File ppapi/api/dev/ppb_printing_dev.idl (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppb_printing_de...
ppapi/api/dev/ppb_printing_dev.idl:24: * the settings have been copied into
<code>print_settings</code>.
What happens if you call this a second time before getting a response?
PP_ERROR_INPROGRESS?

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppp_printing_de...
File ppapi/api/dev/ppp_printing_dev.idl (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppp_printing_de...
ppapi/api/dev/ppp_printing_dev.idl:13: M23 = 0.7
On 2012/07/20 22:46:56, raymes wrote:
> Trung mentioned that this interface version should always match the ppb
> interface version.
I don't think that's strictly true...  your change would work fine either way.
But I think it's a practice that we wanted to start doing, even though it's
unnecessary a lot of the time.

> I'm not certain how to guarantee that though (just having the
> label here doesn't guarantee the interface is generated)
There's a presubmit check to make sure that IDL changes are reflected in the
code (though it doesn't check that PPP and PPB interfaces match).

http://codereview.chromium.org/10795051/diff/21/ppapi/c/dev/ppp_printing_dev.h
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/c/dev/ppp_printing_dev....
ppapi/c/dev/ppp_printing_dev.h:22: #define PPP_PRINTING_DEV_INTERFACE
PPP_PRINTING_DEV_INTERFACE_0_6
Hmm, it looks like you didn't re-generate? I thought the presubmit would catch
that.

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.cc
File ppapi/cpp/dev/printing_dev.cc (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.cc...
ppapi/cpp/dev/printing_dev.cc:116: callback.pp_completion_callback());
You might want to check with Trung about this. If Flash is using PPB_Printing,
we might need to make this method work with 0.6 *and* 0.7 (if possible), so that
a new version of Flapper can get pushed to an older version of Chrome (where
only 0.6 is supported).

It'll be ugly if you have to support that, but *may* be possible with
CallOnMainThread or something.

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.h
File ppapi/cpp/dev/printing_dev.h (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.h#...
ppapi/cpp/dev/printing_dev.h:40: bool
GetDefaultPrintSettings(PP_PrintSettings_Dev* print_settings) const;
Can you just get rid of this? Since this is an internal-only interface, I'd
rather not have to worry about source compatibility.

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/plugin_dispatcher.cc
File ppapi/proxy/plugin_dispatcher.cc (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/plugin_dispatcher...
ppapi/proxy/plugin_dispatcher.cc:103: "the |interface_name()| template function
to the interfaces C++ wrapper?";
nit: interfaces->interface's?

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/ppb_instance_prox...
File ppapi/proxy/ppb_instance_proxy.cc (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/ppb_instance_prox...
ppapi/proxy/ppb_instance_proxy.cc:798: return;  // Instance was probably
deleted.
The callback was probably already run in this case, but maybe it would be good
to ClearAndAbort it anyway (at the very least, this will release our reference
on it). If it's already been run, it won't be run again.

[raymes, 2012-07-24 00:37:07]

Thanks for the review! You might be getting a few more while yzshen is away.
Hope that's ok!

http://codereview.chromium.org/10795051/diff/21/content/browser/renderer_host...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

http://codereview.chromium.org/10795051/diff/21/content/browser/renderer_host...
content/browser/renderer_host/pepper/pepper_message_filter.cc:725:
PP_PrintSettings_Dev settings;
This is only used by the new version of the API so it wont be used until I
implement it properly.

On 2012/07/23 18:20:09, dmichael wrote:
> Previously, you filled in some reasonable defaults. It seems like this might
> break some stuff if you don't fill in the same defaults here for now (until
you
> hook it up for real).

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppb_printing_de...
File ppapi/api/dev/ppb_printing_dev.idl (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppb_printing_de...
ppapi/api/dev/ppb_printing_dev.idl:24: * the settings have been copied into
<code>print_settings</code>.
On 2012/07/23 18:20:09, dmichael wrote:
> What happens if you call this a second time before getting a response?
> PP_ERROR_INPROGRESS?

Done.

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppp_printing_de...
File ppapi/api/dev/ppp_printing_dev.idl (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/api/dev/ppp_printing_de...
ppapi/api/dev/ppp_printing_dev.idl:13: M23 = 0.7
Done. I had to remove the M21 label for it to pick up the change. Backward
compatibility isn't needed on the PPP side by the looks of it.

On 2012/07/23 18:20:09, dmichael wrote:
> On 2012/07/20 22:46:56, raymes wrote:
> > Trung mentioned that this interface version should always match the ppb
> > interface version.
> I don't think that's strictly true...  your change would work fine either way.
> But I think it's a practice that we wanted to start doing, even though it's
> unnecessary a lot of the time.
> 
> > I'm not certain how to guarantee that though (just having the
> > label here doesn't guarantee the interface is generated)
> There's a presubmit check to make sure that IDL changes are reflected in the
> code (though it doesn't check that PPP and PPB interfaces match).

http://codereview.chromium.org/10795051/diff/21/ppapi/c/dev/ppp_printing_dev.h
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/c/dev/ppp_printing_dev....
ppapi/c/dev/ppp_printing_dev.h:22: #define PPP_PRINTING_DEV_INTERFACE
PPP_PRINTING_DEV_INTERFACE_0_6
On 2012/07/23 18:20:09, dmichael wrote:
> Hmm, it looks like you didn't re-generate? I thought the presubmit would catch
> that.

Done.

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.cc
File ppapi/cpp/dev/printing_dev.cc (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.cc...
ppapi/cpp/dev/printing_dev.cc:116: callback.pp_completion_callback());
Good call. I updated the CL with your suggestion and I'll get Trungs thoughts
about it first.

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.h
File ppapi/cpp/dev/printing_dev.h (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/cpp/dev/printing_dev.h#...
ppapi/cpp/dev/printing_dev.h:40: bool
GetDefaultPrintSettings(PP_PrintSettings_Dev* print_settings) const;
If I remove this, flash wont compile I have to do this in 2 commits. I added a
TODO though.

On 2012/07/23 18:20:09, dmichael wrote:
> Can you just get rid of this? Since this is an internal-only interface, I'd
> rather not have to worry about source compatibility.

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/plugin_dispatcher.cc
File ppapi/proxy/plugin_dispatcher.cc (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/plugin_dispatcher...
ppapi/proxy/plugin_dispatcher.cc:103: "the |interface_name()| template function
to the interfaces C++ wrapper?";
On 2012/07/23 18:20:09, dmichael wrote:
> nit: interfaces->interface's?

Done.

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/ppb_instance_prox...
File ppapi/proxy/ppb_instance_proxy.cc (right):

http://codereview.chromium.org/10795051/diff/21/ppapi/proxy/ppb_instance_prox...
ppapi/proxy/ppb_instance_proxy.cc:798: return;  // Instance was probably
deleted.
Our callback is stored in the instance data so if we can't get the instance
data, we can't access the callback. However, if the instance data is gone,
presumably the reference to the callback is released anyway?

On 2012/07/23 18:20:09, dmichael wrote:
> The callback was probably already run in this case, but maybe it would be good
> to ClearAndAbort it anyway (at the very least, this will release our reference
> on it). If it's already been run, it won't be run again.

[viettrungluu, 2012-07-24 00:44:35]

Calling the callback compatibly is fine, but the design of the new async method
isn't.

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev.cc
File ppapi/cpp/dev/printing_dev.cc (right):

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:109: int32_t
Printing_Dev::GetDefaultPrintSettings(
This isn't a very good design, since Printing_Dev isn't a resource, and thus
there's no way to cancel the callback (and how exactly would one manage the
lifetime of |print_settings|?).

Moreover, C++ wrappers should use the callback-with-output stuff.

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:112: if (has_interface<PPB_Printing_Dev_0_6>()) {
These interface checks are in the wrong order.

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:114:
get_interface<PPB_Printing_Dev_0_6>()->GetDefaultPrintSettings(
You should convert from a PP_Bool to a bool properly.

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:117: success ? PP_OK : PP_ERROR);
PP_ERROR_FAILED

[viettrungluu, 2012-07-24 01:08:37]

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev.cc
File ppapi/cpp/dev/printing_dev.cc (right):

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:109: int32_t
Printing_Dev::GetDefaultPrintSettings(
On 2012/07/24 00:44:35, viettrungluu wrote:
> This isn't a very good design, since Printing_Dev isn't a resource, and thus
> there's no way to cancel the callback (and how exactly would one manage the
> lifetime of |print_settings|?).
> 
> Moreover, C++ wrappers should use the callback-with-output stuff.

(Probably using the callback-with-output solves the problem enough for our
purposes, since the caller can use a completion callback factory.)

But you should make this respect the flags in |callback| -- i.e., only return
something other than |PP_OK_COMPLETIONPENDING| when
|PP_COMPLETIONCALLBACK_FLAG_OPTIONAL| is set. Actually, it'd probably be easier
to make this return void and post the callback if necessary.

[viettrungluu, 2012-07-24 03:22:18]

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev.cc
File ppapi/cpp/dev/printing_dev.cc (right):

http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:85:
Module::Get()->AddPluginInterface(kPPPPrintingInterface, &ppp_printing);
You'll also need to modify this to support both the 0.6 and 0.7 PPP interfaces.

[raymes, 2012-07-24 17:54:33]

Discussed the CL with Brett and Trung and the conclusion is that we
want to approach this in a different way, so discarding this CL.

On Mon, Jul 23, 2012 at 8:22 PM,  <viettrungluu@chromium.org> wrote:
>
>
http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev.cc
> File ppapi/cpp/dev/printing_dev.cc (right):
>
>
http://codereview.chromium.org/10795051/diff/13003/ppapi/cpp/dev/printing_dev...
> ppapi/cpp/dev/printing_dev.cc:85:
> Module::Get()->AddPluginInterface(kPPPPrintingInterface, &ppp_printing);
> You'll also need to modify this to support both the 0.6 and 0.7 PPP
> interfaces.
>
> http://codereview.chromium.org/10795051/