Added PPB function to return default print settings

This returns the default print settings for the default printer. Currently this just returns some generic values until it is hooked up to the chrome printing code.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142232

[raymes, 2012-06-12 21:38:28]

If you are too busy, please let me know and I can nag someone else.

[yzshen1, 2012-06-12 23:14:34]

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
File ppapi/api/dev/pp_print_settings_dev.idl (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
ppapi/api/dev/pp_print_settings_dev.idl:35: /** This is the size of the
printable area in points (1/72 of an inch) */
Please add period at the end of a sentence.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
ppapi/api/dev/pp_print_settings_dev.idl:41: PP_PrintScalingOption_Dev
print_scaling_option;
nit: I would suggest to remove 'print_' to be consistent with other members.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
File ppapi/api/dev/ppb_printing_dev.idl (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_printing_dev.idl:7: * Implementation of the Printing
interface.
This is not an 'implementation', right?

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_printing_dev.idl:17: * Output the default print settings for
the default printer into
Output -> Outputs.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_printing_dev.idl:20: PP_Bool GetDefaultPrintSettings([in]
PP_Instance instance,
[Just to make sure you have considered it.]
Is it necessary to be a sync call?

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
File ppapi/cpp/dev/printing_dev.cc (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
ppapi/cpp/dev/printing_dev.cc:79: bool Printing_Dev::GetDefaultPrintSettings(
Please keep method definitions in the same order as the declarations.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
File ppapi/cpp/dev/printing_dev.h (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
ppapi/cpp/dev/printing_dev.h:36: static bool GetDefaultPrintSettings(const
InstanceHandle& instance,
Can it be a non-static method of this class?

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppapi_...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppapi_...
ppapi/proxy/ppapi_messages.h:469:
IPC_SYNC_MESSAGE_ROUTED1_2(PpapiMsg_PPBPrinting_GetDefaultPrintSettings,
It should be called Ppapi*Host*Msg and put in the PpapiHostMsg section below.

And you could see that messages are ordered by the corresponding interface name,
alphabetically. In this case, you may want to put it in the PPB_Instance group.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppapi_...
ppapi/proxy/ppapi_messages.h:471: std::string /* settings_string */,
nit: usually you don't need to have the type in the parameter name.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppb_in...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppb_in...
ppapi/proxy/ppb_instance_proxy.cc:471: if (result && print_settings &&
You could test print_settings at the top of the method and fail early.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppb_in...
ppapi/proxy/ppb_instance_proxy.cc:473: memcpy(print_settings,
&settings_string[0], sizeof(*print_settings));
I don't think it is a good idea to pass a raw buffer.
Please pass the structure itself. :)

[raymes, 2012-06-13 18:23:46]

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
File ppapi/api/dev/pp_print_settings_dev.idl (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
ppapi/api/dev/pp_print_settings_dev.idl:35: /** This is the size of the
printable area in points (1/72 of an inch) */
On 2012/06/12 23:14:34, yzshen1 wrote:
> Please add period at the end of a sentence.

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
ppapi/api/dev/pp_print_settings_dev.idl:41: PP_PrintScalingOption_Dev
print_scaling_option;
On 2012/06/12 23:14:34, yzshen1 wrote:
> nit: I would suggest to remove 'print_' to be consistent with other members.

I just moved this struct from the other idl file. If I change the name, I'll
have to change code in flapper as well. I'd prefer not to do that for now.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
File ppapi/api/dev/ppb_printing_dev.idl (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_printing_dev.idl:7: * Implementation of the Printing
interface.
On 2012/06/12 23:14:34, yzshen1 wrote:
> This is not an 'implementation', right?

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_printing_dev.idl:17: * Output the default print settings for
the default printer into
On 2012/06/12 23:14:34, yzshen1 wrote:
> Output -> Outputs.

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_printing_dev.idl:20: PP_Bool GetDefaultPrintSettings([in]
PP_Instance instance,
On 2012/06/12 23:14:34, yzshen1 wrote:
> [Just to make sure you have considered it.]
> Is it necessary to be a sync call?

Yes, Trung wants this to be synchronous for flash.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
File ppapi/cpp/dev/printing_dev.cc (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
ppapi/cpp/dev/printing_dev.cc:79: bool Printing_Dev::GetDefaultPrintSettings(
On 2012/06/12 23:14:34, yzshen1 wrote:
> Please keep method definitions in the same order as the declarations.

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
File ppapi/cpp/dev/printing_dev.h (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/cpp/dev/prin...
ppapi/cpp/dev/printing_dev.h:36: static bool GetDefaultPrintSettings(const
InstanceHandle& instance,
On 2012/06/12 23:14:34, yzshen1 wrote:
> Can it be a non-static method of this class?

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppapi_...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppapi_...
ppapi/proxy/ppapi_messages.h:469:
IPC_SYNC_MESSAGE_ROUTED1_2(PpapiMsg_PPBPrinting_GetDefaultPrintSettings,
On 2012/06/12 23:14:34, yzshen1 wrote:
> It should be called Ppapi*Host*Msg and put in the PpapiHostMsg section below.
> 
> And you could see that messages are ordered by the corresponding interface
name,
> alphabetically. In this case, you may want to put it in the PPB_Instance
group.

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppapi_...
ppapi/proxy/ppapi_messages.h:471: std::string /* settings_string */,
On 2012/06/12 23:14:34, yzshen1 wrote:
> nit: usually you don't need to have the type in the parameter name.

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppb_in...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppb_in...
ppapi/proxy/ppb_instance_proxy.cc:471: if (result && print_settings &&
On 2012/06/12 23:14:34, yzshen1 wrote:
> You could test print_settings at the top of the method and fail early.

Done.

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/proxy/ppb_in...
ppapi/proxy/ppb_instance_proxy.cc:473: memcpy(print_settings,
&settings_string[0], sizeof(*print_settings));
On 2012/06/12 23:14:34, yzshen1 wrote:
> I don't think it is a good idea to pass a raw buffer.
> Please pass the structure itself. :)

This is what was done in the existing code that transferred a
PP_PrintSettings_Dev so I just copied the approach. But I agree that passing the
struct is better.

[yzshen1, 2012-06-14 05:01:38]

LGTM with a few nits.

Thanks!

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
File ppapi/api/dev/pp_print_settings_dev.idl (right):

https://chromiumcodereview.appspot.com/10544085/diff/10001/ppapi/api/dev/pp_p...
ppapi/api/dev/pp_print_settings_dev.idl:41: PP_PrintScalingOption_Dev
print_scaling_option;
Okay.

On 2012/06/13 18:23:46, raymes wrote:
> On 2012/06/12 23:14:34, yzshen1 wrote:
> > nit: I would suggest to remove 'print_' to be consistent with other members.
> 
> I just moved this struct from the other idl file. If I change the name, I'll
> have to change code in flapper as well. I'd prefer not to do that for now.

https://chromiumcodereview.appspot.com/10544085/diff/7024/ppapi/cpp/dev/print...
File ppapi/cpp/dev/printing_dev.cc (right):

https://chromiumcodereview.appspot.com/10544085/diff/7024/ppapi/cpp/dev/print...
ppapi/cpp/dev/printing_dev.cc:90: const InstanceHandle& instance_handle,
Can we use associated_instance_?

[raymes, 2012-06-14 17:30:49]

https://chromiumcodereview.appspot.com/10544085/diff/7024/ppapi/cpp/dev/print...
File ppapi/cpp/dev/printing_dev.cc (right):

https://chromiumcodereview.appspot.com/10544085/diff/7024/ppapi/cpp/dev/print...
ppapi/cpp/dev/printing_dev.cc:90: const InstanceHandle& instance_handle,
On 2012/06/14 05:01:38, yzshen1 wrote:
> Can we use associated_instance_? 

Done.

[viettrungluu, 2012-06-14 19:19:55]

lgtm

[commit-bot: I haz the power, 2012-06-14 19:32:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10544085/2006

[commit-bot: I haz the power, 2012-06-14 21:15:52]

Change committed as 142232