[Print Preview] Modified PP_PrintSettings_Dev interface to support auto fit to page functionality.

[Print Preview] Modified PP_PrintSettings_Dev interface to support auto fit to page functionality.

BUG=85132
TEST=none


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

[kmadhusu, 2012-04-19 03:13:02]

dmichael:
As per our discussion, I added more params to the PP_PrintSettings_Dev to
support auto fit to page functionality. I found out that plugin is updated using
the chrome update mechanism. Therefore, We don't have to worry about backward
compatibility. I have submitted the webkit side patch for review
(https://bugs.webkit.org/show_bug.cgi?id=84312). Please review the ppapi related
changes.

vandebo:
Please review the chrome/renderer changes.

Thanks.

[vandebo (ex-Chrome), 2012-04-19 18:45:31]

http://codereview.chromium.org/10083059/diff/4003/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/10083059/diff/4003/chrome/common/print_message...
chrome/common/print_messages.h:44: bool fit_pdf_page_to_paper_size;
For some reason this name sticks out as odd to me.  Does it matter what the
value is when we're not printing a pdf?

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:323: // Don't fit to page when we save
to pdf or when we print for preview.
This is a bit weird - as we talked about, using an enum in the printBegin call
will clean it up.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:623: bool print_for_preview)
The frame and view doesn't care if we're printing for preview or not, it does
care if we're fitting to page or not, so that should probably be the argument.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:700: node_to_print_, dpi_,
nit: you might as well put dpi on its own line.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1333: print_pages_params_->params,
frame, node, print_for_preview_));
style: we only wrap if an argument won't fit.

http://codereview.chromium.org/10083059/diff/4003/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10083059/diff/4003/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:236: int PrintBegin(const
gfx::Rect& content_area,
You'll probably need to make a chrome commit first that adds the new version of
this interface.  Then make the webkit commit that uses it, then your third
commit can remove the old one and have the rest of your changes from this CL. 
The forth commit will remove the extra stuff from WebKit.

[dmichael (off chromium), 2012-04-19 20:11:46]

+viettrungluu to confirm that PPP_Printing is not used by flash and thus does
not need backwards compatibility.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:46: bool print_for_preview);
optional suggestion: I like using enums instead of bools for parameters to make
the call-sites self-documenting.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:69: bool fit_pdf_page_to_paper_size);
ditto

http://codereview.chromium.org/10083059/diff/4003/ppapi/api/dev/ppp_printing_...
File ppapi/api/dev/ppp_printing_dev.idl (right):

http://codereview.chromium.org/10083059/diff/4003/ppapi/api/dev/ppp_printing_...
ppapi/api/dev/ppp_printing_dev.idl:11: M14 = 0.5
You should rev the version here by adding something like
M20 = 0.5

[viettrungluu, 2012-04-19 20:19:05]

Flash doesn't use it quite yet (well, it's behind a flag) ... and this change is
actually helpful.

But you really should (must?) increment the version number.

http://codereview.chromium.org/10083059/diff/4003/ppapi/api/dev/ppp_printing_...
File ppapi/api/dev/ppp_printing_dev.idl (right):

http://codereview.chromium.org/10083059/diff/4003/ppapi/api/dev/ppp_printing_...
ppapi/api/dev/ppp_printing_dev.idl:11: M14 = 0.5
On 2012/04/19 20:11:46, dmichael wrote:
> You should rev the version here by adding something like
> M20 = 0.5

Since this is decidedly binary-incompatible, you should really increment the
version.

[kmadhusu, 2012-04-20 22:29:57]

Thanks for all your comments. CC'ed brettw@.

I have addressed all your review comments. This CL has ppapi changes to support
the functionality. I moved the chrome/renderer changes to a separate CL
(http://codereview.chromium.org/10168013/) to make the commit process easier. I
have uploaded the webkit patch for review
(https://bugs.webkit.org/show_bug.cgi?id=84312).

To avoid the compile errors, while loading these patches seperately, I have
created the required overloaded printBegin functions and removed OVERRIDE
keywords correspondingly. Once I land the Webkit changes, I will remove the old
printBegin function and add the OVERRIDE keyword to the actual function.

Once I get your initial approval, I will set the review flag in the webkit
patch.

Thanks.

http://codereview.chromium.org/10083059/diff/4003/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/10083059/diff/4003/chrome/common/print_message...
chrome/common/print_messages.h:44: bool fit_pdf_page_to_paper_size;
On 2012/04/19 18:45:31, vandebo wrote:
> For some reason this name sticks out as odd to me.  Does it matter what the
> value is when we're not printing a pdf?

Renamed the variable and as we discussed, I committed a separate CL for that.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:323: // Don't fit to page when we save
to pdf or when we print for preview.
On 2012/04/19 18:45:31, vandebo wrote:
> This is a bit weird - as we talked about, using an enum in the printBegin call
> will clean it up.

Done.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:623: bool print_for_preview)
On 2012/04/19 18:45:31, vandebo wrote:
> The frame and view doesn't care if we're printing for preview or not, it does
> care if we're fitting to page or not, so that should probably be the argument.

Fixed. Constructor now takes a print scaling option enum value.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:700: node_to_print_, dpi_,
On 2012/04/19 18:45:31, vandebo wrote:
> nit: you might as well put dpi on its own line.

Done.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1333: print_pages_params_->params,
frame, node, print_for_preview_));
On 2012/04/19 18:45:31, vandebo wrote:
> style: we only wrap if an argument won't fit.

Done.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:46: bool print_for_preview);
On 2012/04/19 20:11:46, dmichael wrote:
> optional suggestion: I like using enums instead of bools for parameters to
make
> the call-sites self-documenting.

Done. Added an enum param to specify the print scaling option.

http://codereview.chromium.org/10083059/diff/4003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:69: bool fit_pdf_page_to_paper_size);
On 2012/04/19 20:11:46, dmichael wrote:
> ditto

This bool is no longer required. So I removed from the param list.

http://codereview.chromium.org/10083059/diff/4003/ppapi/api/dev/ppp_printing_...
File ppapi/api/dev/ppp_printing_dev.idl (right):

http://codereview.chromium.org/10083059/diff/4003/ppapi/api/dev/ppp_printing_...
ppapi/api/dev/ppp_printing_dev.idl:11: M14 = 0.5
On 2012/04/19 20:19:06, viettrungluu wrote:
> On 2012/04/19 20:11:46, dmichael wrote:
> > You should rev the version here by adding something like
> > M20 = 0.5
> 
> Since this is decidedly binary-incompatible, you should really increment the
> version.

Done. Added M20 = 0.6 and also updated interface names in ppp_printing_dev.h

http://codereview.chromium.org/10083059/diff/4003/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10083059/diff/4003/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:236: int PrintBegin(const
gfx::Rect& content_area,
On 2012/04/19 18:45:31, vandebo wrote:
> You'll probably need to make a chrome commit first that adds the new version
of
> this interface.  Then make the webkit commit that uses it, then your third
> commit can remove the old one and have the rest of your changes from this CL. 
> The forth commit will remove the extra stuff from WebKit.

I agree.. So this will be my first commit (Just the ppapi changes).

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:261: 2, printer_dpi);
dmichael: Can I include ppapi/c/dev/ppp_printing_dev.h in this file and use
PP_PRINTSCALEOPTION_ACTUAL_SIZE here?

[dmichael (off chromium), 2012-04-20 23:01:45]

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1162: print_scaling_option);
Please explicitly convert the enum values or add static checks to make sure they
match up from WebKit to pepper. (see, e.g., the COMPILE_ASSERT_MATCHING_ENUM
stuff earlier in this file).

This will require the WebKit change to land, so if it turns out to not be
possible in this CL, that's OK.

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:238: //
WebKit::WebPrintScalingOption after fixing crbug.com/85132.
Are you sure that's going to work okay? int and WebPrintScalingOption are
distinct types. Even though an enum can implicitly convert to an int, I doubt it
will work right for overriding a virtual function.

You might need to land a WebKit side change first to add the new virtual
function with a default implementation... but somebody with more WebKit
experience (vandebo?) will be able to advise you about the right way to go.

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:261: 2, printer_dpi);
On 2012/04/20 22:29:57, kmadhusu wrote:
> dmichael: Can I include ppapi/c/dev/ppp_printing_dev.h in this file and use
> PP_PRINTSCALEOPTION_ACTUAL_SIZE here?
You could, but shouldn't it be the WebKit value at this level?

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:267: int print_scaling_option,
Is this the typical way of making these 2-sided changes? I might have expected
you to land the WebKit side first with a default implementation initially.

[vandebo (ex-Chrome), 2012-04-21 01:06:48]

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:238: //
WebKit::WebPrintScalingOption after fixing crbug.com/85132.
On 2012/04/20 23:01:45, dmichael wrote:
> Are you sure that's going to work okay? int and WebPrintScalingOption are
> distinct types. Even though an enum can implicitly convert to an int, I doubt
it
> will work right for overriding a virtual function.

Kausalya and I talked about it, and yes, the enum needs to go into WebKit first.

> You might need to land a WebKit side change first to add the new virtual
> function with a default implementation... but somebody with more WebKit
> experience (vandebo?) will be able to advise you about the right way to go.

It took us a bit to understand what you meant here (darn overloaded english
words).  What you suggest would maintain compile-ability of the codebase, but
would break printing when in the middle of the multi-repo commit.  This way
printing continues to work throughout the process.

[dmichael (off chromium), 2012-04-23 15:21:09]

lgtm

[kmadhusu, 2012-04-23 16:39:29]

dmichael: Thanks for the lgtm stamp. I assume you reviewed my latest patch
(PatchSet 11).

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1162: print_scaling_option);
On 2012/04/20 23:01:45, dmichael wrote:
> Please explicitly convert the enum values or add static checks to make sure
they
> match up from WebKit to pepper. (see, e.g., the COMPILE_ASSERT_MATCHING_ENUM
> stuff earlier in this file).
> 
> This will require the WebKit change to land, so if it turns out to not be
> possible in this CL, that's OK.

Assuming, I will land this patch after committing the webkit header file, I have
 added these compile assert checks.

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:238: //
WebKit::WebPrintScalingOption after fixing crbug.com/85132.
On 2012/04/20 23:01:45, dmichael wrote:
> Are you sure that's going to work okay? int and WebPrintScalingOption are
> distinct types. Even though an enum can implicitly convert to an int, I doubt
it
> will work right for overriding a virtual function.
>

As you said int and WebPrintScalingOption are distinct types. If you see my
webkit patch, you will notice that I had "int" data type for
print_scaling_option param.
 
> You might need to land a WebKit side change first to add the new virtual
> function with a default implementation... but somebody with more WebKit
> experience (vandebo?) will be able to advise you about the right way to go.

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:261: 2, printer_dpi);
On 2012/04/20 23:01:45, dmichael wrote:
> On 2012/04/20 22:29:57, kmadhusu wrote:
> > dmichael: Can I include ppapi/c/dev/ppp_printing_dev.h in this file and use
> > PP_PRINTSCALEOPTION_ACTUAL_SIZE here?
> You could, but shouldn't it be the WebKit value at this level?

Yeah it is a WebKit value. Since I had 'int' data type in my previous version of
printBegin() function, I thought I will pass the PP_PrintScaleOption_Dev  value
directly. Now that, we are planning to commit the webkit header file first, I 
will pass the webkit enum value directly.

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:267: int print_scaling_option,
On 2012/04/20 23:01:45, dmichael wrote:
> Is this the typical way of making these 2-sided changes? I might have expected
> you to land the WebKit side first with a default implementation initially.

If we are going to create a new function and the  call site is in Chromium code,
then we will land the Webkit patch first. Once the webkit changes are rolled
into Chromium, we will update the Chromium to use the new webkit function. 

In our case, we have code calling (both ways) from Chromium --> Webkit  -->
Chromium plugin.  We are just trying to support printing and to avoid compile
errors while committing these individual patches.

[dmichael (off chromium), 2012-04-23 17:02:26]

On 2012/04/23 16:39:29, kmadhusu wrote:
> dmichael: Thanks for the lgtm stamp. I assume you reviewed my latest patch
> (PatchSet 11).
> 
Yes, thanks for addressing my concerns.

[dmichael (off chromium), 2012-04-23 17:02:40]

still lgtm

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:238: //
WebKit::WebPrintScalingOption after fixing crbug.com/85132.
On 2012/04/23 16:39:30, kmadhusu wrote:
> On 2012/04/20 23:01:45, dmichael wrote:
> > Are you sure that's going to work okay? int and WebPrintScalingOption are
> > distinct types. Even though an enum can implicitly convert to an int, I
doubt
> it
> > will work right for overriding a virtual function.
> >
> 
> As you said int and WebPrintScalingOption are distinct types. If you see my
> webkit patch, you will notice that I had "int" data type for
> print_scaling_option param.
Okay, that would have worked for the initial commit, but would have broken had
you changed to the enum in the interface later. In any case, I like using the
enum in the API and landing a WebKit change with the enum first better, as you
seem to be proposing to do now.

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:267: int print_scaling_option,
On 2012/04/23 16:39:30, kmadhusu wrote:
> On 2012/04/20 23:01:45, dmichael wrote:
> > Is this the typical way of making these 2-sided changes? I might have
expected
> > you to land the WebKit side first with a default implementation initially.
> 
> If we are going to create a new function and the  call site is in Chromium
code,
> then we will land the Webkit patch first. Once the webkit changes are rolled
> into Chromium, we will update the Chromium to use the new webkit function. 
> 
> In our case, we have code calling (both ways) from Chromium --> Webkit  -->
> Chromium plugin.  We are just trying to support printing and to avoid compile
> errors while committing these individual patches. 
Right...  I think you could land the WebKit side first but not use new function
yet there (so long as it wasn't a pure virtual function). But now I see that
doesn't really make sense; I was stuck on the problem of the enum. I like the
approach of putting in the enum first, then landing this change (as in patch set
11), then landing the WebKit side with the updated printBegin function.

[brettw, 2012-04-23 17:57:48]

http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_d...
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_d...
ppapi/c/dev/ppp_printing_dev.h:94: struct PPP_Printing_Dev_0_6 {
This versioning isn't right. The version of the interface changed but the struct
that's being passed did not. This means you'll break backwards compat. Try to
see if versioning works with the structs (not sure). If not, you'll have to
hardcode the version in the struct name.

[kmadhusu, 2012-04-23 18:20:12]

http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_d...
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_d...
ppapi/c/dev/ppp_printing_dev.h:94: struct PPP_Printing_Dev_0_6 {
On 2012/04/23 17:57:48, brettw wrote:
> This versioning isn't right. The version of the interface changed but the
struct
> that's being passed did not. This means you'll break backwards compat. Try to
> see if versioning works with the structs (not sure). If not, you'll have to
> hardcode the version in the struct name.

I agree that I did not change PPP_Printing_Dev_0_5. But I changed the chrome
label version in ppp_printing_dev.idl. I thought these numbers need to match
that version. Is it right?

How can I test if my changes breaks backward compatibility? (unit_tests,
browser_tests)?? (Sorry for the silly question).

From the description of http://codereview.chromium.org/8989006/, It looks like I
need to create a versioned struct and typedef a unversioned struct.

Eg:
struct PP_PrintSettings_Dev_0_5 {
...
};

typedef struct PP_PrintSettings_Dev_0_5 PP_PrintSettings_Dev;

Is it correct?

[dmichael (off chromium), 2012-04-23 19:00:39]

http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_d...
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_d...
ppapi/c/dev/ppp_printing_dev.h:94: struct PPP_Printing_Dev_0_6 {
On 2012/04/23 17:57:48, brettw wrote:
> This versioning isn't right. The version of the interface changed but the
struct
> that's being passed did not. This means you'll break backwards compat. Try to
> see if versioning works with the structs (not sure). If not, you'll have to
> hardcode the version in the struct name.
I figured this was okay in this case since backwards-compat isn't really
necessary here and isn't implemented for this interface. If we wanted to allow
either Chrome or the plugin to handle both 0.5 and 0.6 dynamically, we would
need two versions of the parameter struct (_and_ two versions of the interface).

[brettw, 2012-04-23 22:16:44]

Okay, we can do what you did. Normally we have to provide backwards compat for
Flash, but printing isn't hooked up for that, and PDF is always build and
distributed with Chrome. LGTM

[kmadhusu, 2012-05-11 21:38:53]

dmichael@ (repeating the message for future reference): Darin reviewed my webkit
patch and asked me to group the required fit to page params as
WebPrintParams and to pass that struct around from Chromium -> WebKit ->
WebPlugin -> PPAPI_Plugin_Instance. So, I modified the webkit patch
(https://bugs.webkit.org/show_bug.cgi?id=84312) and the corresponding
PP_Printing Interface CL (https://chromiumcodereview.appspot.com/10083059) and
Chrome printBegin() function call sites
(https://chromiumcodereview.appspot.com/10168013/) accordingly. 

Looks like, I will be landing the CL's in the following order:
(1) Webkit Changes.
(2) PP_Printing interface changes.
(3) Chrome changes to call the overloaded printBegin() function.

After all the required CL's are committed, I will submit a webkit CL and a
PP_PrintingInterface CL to remove the old printBegin() function signatures. 

PTAL.

Thanks.

[dmichael (off chromium), 2012-05-14 17:16:38]

lgtm

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:126: using WebKit::WebPrintParams;
nit: these should be in alphabetical order

[dmichael (off chromium), 2012-05-14 17:18:24]

oops, one more nit. still lgtm.

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:36: using WebKit::WebPrintParams;
nit: these should be alphabetical

[kmadhusu, 2012-05-14 18:00:32]

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:126: using WebKit::WebPrintParams;
On 2012/05/14 17:16:39, dmichael wrote:
> nit: these should be in alphabetical order

Done.

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:36: using WebKit::WebPrintParams;
On 2012/05/14 17:18:24, dmichael wrote:
> nit: these should be alphabetical

Done.

[viettrungluu, 2012-05-15 20:23:57]

Note: If this doesn't land soon, Flapper will start using the old (current)
PPP_Printing interface, and you'll have to make everything backwards compatible.

https://chromiumcodereview.appspot.com/10083059/diff/55006/ppapi/api/dev/ppp_...
File ppapi/api/dev/ppp_printing_dev.idl (right):

https://chromiumcodereview.appspot.com/10083059/diff/55006/ppapi/api/dev/ppp_...
ppapi/api/dev/ppp_printing_dev.idl:11: M20 = 0.6
Now M21.

[kmadhusu, 2012-05-15 20:26:44]

On 2012/05/15 20:23:57, viettrungluu wrote:
> Note: If this doesn't land soon, Flapper will start using the old (current)
> PPP_Printing interface, and you'll have to make everything backwards
compatible.

Thanks for the info. Do you have any specific date? I am just waiting for the
webkit patch to land first. 

> 
>
https://chromiumcodereview.appspot.com/10083059/diff/55006/ppapi/api/dev/ppp_...
> File ppapi/api/dev/ppp_printing_dev.idl (right):
> 
>
https://chromiumcodereview.appspot.com/10083059/diff/55006/ppapi/api/dev/ppp_...
> ppapi/api/dev/ppp_printing_dev.idl:11: M20 = 0.6
> Now M21.

[kmadhusu, 2012-05-15 22:55:39]

Addressed review comments.

FYI: I got r+ for the webkit patch. Once it is landed, I will land these
changes.

Thanks.

https://chromiumcodereview.appspot.com/10083059/diff/55006/ppapi/api/dev/ppp_...
File ppapi/api/dev/ppp_printing_dev.idl (right):

https://chromiumcodereview.appspot.com/10083059/diff/55006/ppapi/api/dev/ppp_...
ppapi/api/dev/ppp_printing_dev.idl:11: M20 = 0.6
On 2012/05/15 20:23:58, viettrungluu wrote:
> Now M21.

Done.

[commit-bot: I haz the power, 2012-05-16 04:30:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10083059/62004

[commit-bot: I haz the power, 2012-05-16 16:29:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10083059/62004

[commit-bot: I haz the power, 2012-05-16 17:59:21]

Change committed as 137453