[Print Preview] Modified PrepareFrameAndViewPrint class to call the new printBegin function to supp…

Use overloaded printBegin() chromium-webkit api in PrepareFrameAndViewPrint class and webkit-glue.cc

BUG=85132
TEST=none


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

[kmadhusu, 2012-04-20 22:23:58]

I moved the chrome/renderer related changes from
http://codereview.chromium.org/10083059/ to a separate CL to make the commit
process easier. 

FYI: You can find the webkit patch at
https://bugs.webkit.org/show_bug.cgi?id=84312 and ppapi patch at
http://codereview.chromium.org/10083059.

Please review these changes and let me know your comments.

Once I get your initial approval, I will submit the webkit patch for review.

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

http://codereview.chromium.org/10168013/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:21: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebPrintScalingOption.h"
Style??? Do I need to intent 4 space?

[vandebo (ex-Chrome), 2012-04-20 23:26:11]

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

http://codereview.chromium.org/10168013/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1298: print_scaling_option_ =
static_cast<WebKit::WebPrintScalingOption>(
This cast is pretty ugly.  A conditional or trinary op would be better.

http://codereview.chromium.org/10168013/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1430: settings.params.fit_to_paper_size
=
At this point, it might be clearer to use an if or two...

settings.params.fir_to_paper_size = true
print_scaling_option_ = WebKit::WebPrintFitToPrintableArea;
if (print_for_preview_ || settings.params.print_to_pdf) {
  print_scaling_option_ = false;
  settings.params.fit_to_paper_size = WebKit::WebPrintActualSize;
} else if(PrintingNodeOrPdfFrame(frame, node) &&
          frame->isPrintScalingDisabledForPlugin(node)) {
  settings.params.fit_to_paper_size = false
  print_scaling_option_ = WebKit::WebPrintScaleNone;
}

Or something like this.

[kmadhusu, 2012-04-23 17:39:10]

As we discussed, I created two bugs in webkit. 
(https://bugs.webkit.org/show_bug.cgi?id=84312
https://bugs.webkit.org/show_bug.cgi?id=84608).

Addressed review comments.

Thanks.

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

http://codereview.chromium.org/10168013/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1298: print_scaling_option_ =
static_cast<WebKit::WebPrintScalingOption>(
On 2012/04/20 23:26:11, vandebo wrote:
> This cast is pretty ugly.  A conditional or trinary op would be better.

Done... Removed print_scaling_option_ from PrintWebViewHelper. Modified
PrintMsg_Print_Params fit_to_page => print_scaling_option.

http://codereview.chromium.org/10168013/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1430: settings.params.fit_to_paper_size
=
On 2012/04/20 23:26:11, vandebo wrote:
> At this point, it might be clearer to use an if or two...
> 
> settings.params.fir_to_paper_size = true
> print_scaling_option_ = WebKit::WebPrintFitToPrintableArea;
> if (print_for_preview_ || settings.params.print_to_pdf) {
>   print_scaling_option_ = false;
>   settings.params.fit_to_paper_size = WebKit::WebPrintActualSize;
> } else if(PrintingNodeOrPdfFrame(frame, node) &&
>           frame->isPrintScalingDisabledForPlugin(node)) {
>   settings.params.fit_to_paper_size = false
>   print_scaling_option_ = WebKit::WebPrintScaleNone;
> }
> 
> Or something like this.

Done.

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

http://codereview.chromium.org/10168013/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:21: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebPrintScalingOption.h"
On 2012/04/20 22:23:58, kmadhusu wrote:
> Style??? Do I need to intent 4 space?

Leaving this as it is.

http://codereview.chromium.org/10168013/diff/13001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/13001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1434:
job_settings->GetBoolean(printing::kSettingFitToPageEnabled,
(We will send FitToPage option along with JobSettings from JS -> C++ code)

[vandebo (ex-Chrome), 2012-04-23 21:42:19]

LGTM

http://codereview.chromium.org/10168013/diff/13001/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/10168013/diff/13001/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:147: // True if we want to fit the html/pdf
source to paper size.
nit: Update the comment, true doesn't make sense any more.

http://codereview.chromium.org/10168013/diff/13001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/13001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1428: WebKit::WebPrintScalingOption
scaling_option =
nit: No need for a local variable, just set the one in params at the various
points.

http://codereview.chromium.org/10168013/diff/13001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1434:
job_settings->GetBoolean(printing::kSettingFitToPageEnabled,
nit: Do you want to create another method like IsPrintToPdfRequested?

[kmadhusu, 2012-05-11 21:36:02]

vandebo, dmichael@: Darin 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 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.

https://chromiumcodereview.appspot.com/10168013/diff/13001/chrome/renderer/mo...
File chrome/renderer/mock_printer.h (right):

https://chromiumcodereview.appspot.com/10168013/diff/13001/chrome/renderer/mo...
chrome/renderer/mock_printer.h:147: // True if we want to fit the html/pdf
source to paper size.
On 2012/04/23 21:42:19, vandebo wrote:
> nit: Update the comment, true doesn't make sense any more.

Done.

https://chromiumcodereview.appspot.com/10168013/diff/13001/chrome/renderer/pr...
File chrome/renderer/print_web_view_helper.cc (right):

https://chromiumcodereview.appspot.com/10168013/diff/13001/chrome/renderer/pr...
chrome/renderer/print_web_view_helper.cc:1428: WebKit::WebPrintScalingOption
scaling_option =
On 2012/04/23 21:42:19, vandebo wrote:
> nit: No need for a local variable, just set the one in params at the various
> points.

I have added a new function GetPrintScalingOption() and addressed this comment
there.

https://chromiumcodereview.appspot.com/10168013/diff/13001/chrome/renderer/pr...
chrome/renderer/print_web_view_helper.cc:1434:
job_settings->GetBoolean(printing::kSettingFitToPageEnabled,
On 2012/04/23 21:42:19, vandebo wrote:
> nit: Do you want to create another method like IsPrintToPdfRequested?

Added FitToPageEnabled() and GetPrintScalingOption() function.

https://chromiumcodereview.appspot.com/10168013/diff/18002/chrome/renderer/pr...
File chrome/renderer/print_web_view_helper.cc (right):

https://chromiumcodereview.appspot.com/10168013/diff/18002/chrome/renderer/pr...
chrome/renderer/print_web_view_helper.cc:771: expected_pages_count_ =
frame_->printBegin(webkit_print_params,
I constructed the |webkit_print_params| right before the function call because
the member variables of this struct are WebRect, WebSize, etc., All the params
in |print_params| are of type gfx::Rect and gfx::Size respectively. While trying
to compare these values in line 732, it didn't look good.

[kmadhusu, 2012-05-11 23:31:59]

vandebo@: Modified code to have WebPrintParams object as a member variable of
PrepareFrameAndViewForPrint class. PTAL.

Thanks.

[vandebo (ex-Chrome), 2012-05-14 17:45:46]

http://codereview.chromium.org/10168013/diff/30003/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/30003/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:747: void
PrepareFrameAndViewForPrint::StartPrinting(
It looks like this should take a web print params.  If that's so, then
ComputePrintParamsInDesiredDpi probably should fill out one of those too.

[kmadhusu, 2012-05-14 18:50:42]

http://codereview.chromium.org/10168013/diff/30003/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/30003/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:747: void
PrepareFrameAndViewForPrint::StartPrinting(
On 2012/05/14 17:45:46, vandebo wrote:
> It looks like this should take a web print params.  If that's so, then
> ComputePrintParamsInDesiredDpi probably should fill out one of those too.

Done.

[vandebo (ex-Chrome), 2012-05-16 17:41:32]

LGTM with nits.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:953: // Do not fit to paper size when
the user is saving the print contents as pdf.
nit: redundant comment.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:961: // Get the print scaling option
for the initiator renderer pdf.
nit: redundant comment

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:962: bool
print_scaling_disabled_for_plugin =
nit: you might be able to shorten this variable name to help formatting. i.e.
no_plugin_scaling

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:963:
print_preview_context_.frame()->isPrintScalingDisabledForPlugin(
nit: indent

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:966: // If this is the first preview
request, UI doesn't know about the print
nit: It looks like this comment just describes WHAT the code does.  If there's
an important WHY to explain, do that, otherwise just remove the comment.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1376: ignore_css_margins &&
nit: because of formatting, you might want to pull this out to a bool variable.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1404:
settings.params.print_scaling_option = fit_to_paper_size ?
nit: just use an if. assign SourceSize, if (fit_to_paper_size) update

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1417: // If the source is html, fit to
paper size else print the source pdf size.
nit: redundant comment.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1577: // Store the calculated print
scaling option value.
nit: Move the below comment up here as:
"PrintHostMsg_ScriptedPrint will reset print_scaling_option, so we save the
value before and restore it afterward."

[kmadhusu, 2012-05-16 18:22:40]

Fixed nits. Thanks.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:953: // Do not fit to paper size when
the user is saving the print contents as pdf.
On 2012/05/16 17:41:32, vandebo wrote:
> nit: redundant comment.

Done.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:961: // Get the print scaling option
for the initiator renderer pdf.
On 2012/05/16 17:41:32, vandebo wrote:
> nit: redundant comment

Done.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:962: bool
print_scaling_disabled_for_plugin =
On 2012/05/16 17:41:32, vandebo wrote:
> nit: you might be able to shorten this variable name to help formatting. i.e.
> no_plugin_scaling

Done.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:963:
print_preview_context_.frame()->isPrintScalingDisabledForPlugin(
On 2012/05/16 17:41:32, vandebo wrote:
> nit: indent

Done.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:966: // If this is the first preview
request, UI doesn't know about the print
On 2012/05/16 17:41:32, vandebo wrote:
> nit: It looks like this comment just describes WHAT the code does.  If there's
> an important WHY to explain, do that, otherwise just remove the comment.

Removed.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1376: ignore_css_margins &&
On 2012/05/16 17:41:32, vandebo wrote:
> nit: because of formatting, you might want to pull this out to a bool
variable.

Done.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1404:
settings.params.print_scaling_option = fit_to_paper_size ?
On 2012/05/16 17:41:32, vandebo wrote:
> nit: just use an if. assign SourceSize, if (fit_to_paper_size) update

Done.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1417: // If the source is html, fit to
paper size else print the source pdf size.
On 2012/05/16 17:41:32, vandebo wrote:
> nit: redundant comment.

Done.

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1577: // Store the calculated print
scaling option value.
On 2012/05/16 17:41:32, vandebo wrote:
> nit: Move the below comment up here as:
> "PrintHostMsg_ScriptedPrint will reset print_scaling_option, so we save the
> value before and restore it afterward."

Done.

[vandebo (ex-Chrome), 2012-05-16 18:30:47]

Update the issue description before committing - it still says WIP.

LGTM

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/25011/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:962: bool
print_scaling_disabled_for_plugin =
On 2012/05/16 18:22:41, kmadhusu wrote:
> On 2012/05/16 17:41:32, vandebo wrote:
> > nit: you might be able to shorten this variable name to help formatting.
i.e.
> > no_plugin_scaling
> 
> Done.

I guess it didn't help.

[kmadhusu, 2012-05-16 22:59:38]

vandebo: Rebased and fixed conflicts. Please review the latest patch.
darin: Please review webkit_glue.cc.

Thanks.

[vandebo (ex-Chrome), 2012-05-16 23:24:03]

LGTM

[darin (slow to review), 2012-05-17 19:13:02]

LGTM

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:76: using WebKit::WebDocument;
nit: add more 'using WebKit::WebFoo' here to avoid having to type WebKit:: down
below.

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:60: return
gfx::Size(web_print_params_.printContentArea.width,
WebSize can automagically be converted to gfx::Size.

http://codereview.chromium.org/10168013/diff/29029/webkit/glue/webkit_glue.cc
File webkit/glue/webkit_glue.cc (right):

http://codereview.chromium.org/10168013/diff/29029/webkit/glue/webkit_glue.cc...
webkit/glue/webkit_glue.cc:40: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebImage.h"
nit: bonus points: consider re-sorting headers here, which was never done when
we introduced the "platform/" subdir :-(

[kmadhusu, 2012-05-17 20:16:29]

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:76: using WebKit::WebDocument;
On 2012/05/17 19:13:03, darin wrote:
> nit: add more 'using WebKit::WebFoo' here to avoid having to type WebKit::
down
> below.

Done.

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:60: return
gfx::Size(web_print_params_.printContentArea.width,
On 2012/05/17 19:13:03, darin wrote:
> WebSize can automagically be converted to gfx::Size.

PrintContentArea is a WebRect. I didn't find an easy way to create/get gfx::Size
from WebRect. Is there any simple way to do that?

http://codereview.chromium.org/10168013/diff/29029/webkit/glue/webkit_glue.cc
File webkit/glue/webkit_glue.cc (right):

http://codereview.chromium.org/10168013/diff/29029/webkit/glue/webkit_glue.cc...
webkit/glue/webkit_glue.cc:40: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebImage.h"
On 2012/05/17 19:13:03, darin wrote:
> nit: bonus points: consider re-sorting headers here, which was never done when
> we introduced the "platform/" subdir :-(

Done.

[kmadhusu, 2012-05-17 21:24:44]

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10168013/diff/29029/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:60: return
gfx::Size(web_print_params_.printContentArea.width,
On 2012/05/17 20:16:29, kmadhusu wrote:
> On 2012/05/17 19:13:03, darin wrote:
> > WebSize can automagically be converted to gfx::Size.
> 
> PrintContentArea is a WebRect. I didn't find an easy way to create/get
gfx::Size
> from WebRect. Is there any simple way to do that?

Darin: In order to commit the webkit CL asap, I am going to check the CQ
checkbox now. If there is any better way to return gfx::Size, I will make those
changes in a separate CL.

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

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

[commit-bot: I haz the power, 2012-05-17 23:05:04]

Change committed as 137761