Refactor win printing workflow to support print preview.

Refactor win printing workflow to support print preview.

Made code changes to create a single metafile for the entire printing document.

Implemented the following function:
1. CreatePreviewDocument(): Create a single metafile and render all the pages(entire printing document) into the same metafile.

Note: Although the structure of PrintPage() function looks similar to CreatePreviewDocument(), merging of these functions is difficult for the following reasons:
  1. As of now, we are using EMF metafile to create a preview document. In future we are going to use Skia->PDF metafile. So construction and destruction of the metafile will be different.
  2. The message for preview workflow(ViewHostMsg_PagesReadyForPreview) is different from normal printing workflow(ViewHostMsg_DidPrintPage).
  3. The param struct which is sent along with the message for preview workflow is different from printing workflow.
  4. Finally, the semantics of PrintPage() function is to print a page per metafile which is completely different from the semantics of CreatePreviewDocument() function.

Once Skia->Pdf work is completed, Modify CreatePreviewDocument() function to use this PDF metafile.

BUG=64121
TEST=printing works after code changes.

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

[Lei Zhang, 2011-01-26 08:06:51]

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:67: const ViewMsg_Print_Params&
params, WebFrame* frame, int page_number) {
Have you considered putting this code in RenderPage()? Its return value isn't
used anywhere else. Just pass params.params to RenderPage() instead of page_size
and params.params.supports_alpha_blend.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:142: ViewMsg_Print_Params
printParams = params.params;
You still use params.params in several places below.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:142: ViewMsg_Print_Params
printParams = params.params;
Having both |print_params| and |printParams| is confusing. Can you give them
better names? Same goes for print_web_view_helper_mac.mm. Maybe do this in a
separate CL.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:162: gfx::Size page_size =
GetUpdatedPageSizeForRendering(printParams, frame,
The third argument to GetUpdatedPageSizeForRendering() is the page number, not
the total page count. You probably should call in the for loop below.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:164: float scale_factor;
declare this in the scope in which it is used.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:167: float shrink =
static_cast<float>(params.params.desired_dpi /
but I thought you wanted to get rid of variables named "shrink".

[kmadhusu, 2011-01-26 18:08:28]

Made code changes as per comments. Please review.

Thanks.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:67: const ViewMsg_Print_Params&
params, WebFrame* frame, int page_number) {
On 2011/01/26 08:06:52, Lei Zhang wrote:
> Have you considered putting this code in RenderPage()? Its return value isn't
> used anywhere else. Just pass params.params to RenderPage() instead of
page_size
> and params.params.supports_alpha_blend.

Moved this code to RenderPage().

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:142: ViewMsg_Print_Params
printParams = params.params;
On 2011/01/26 08:06:52, Lei Zhang wrote:
> Having both |print_params| and |printParams| is confusing. Can you give them
> better names? Same goes for print_web_view_helper_mac.mm. Maybe do this in a
> separate CL.

Renamed |print_params| to |preview_params|.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:142: ViewMsg_Print_Params
printParams = params.params;
On 2011/01/26 08:06:52, Lei Zhang wrote:
> You still use params.params in several places below.

To get the expected page count,  |printParams| values are scaled based on dpi in
UpdatePrintableSizeInPrintParameters() function. That is the reason why I did
not use |printParams| in code below.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:162: gfx::Size page_size =
GetUpdatedPageSizeForRendering(printParams, frame,
On 2011/01/26 08:06:52, Lei Zhang wrote:
> The third argument to GetUpdatedPageSizeForRendering() is the page number, not
> the total page count. You probably should call in the for loop below.

Fixed.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:164: float scale_factor;
On 2011/01/26 08:06:52, Lei Zhang wrote:
> declare this in the scope in which it is used.

Done.

http://codereview.chromium.org/6261025/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:167: float shrink =
static_cast<float>(params.params.desired_dpi /
On 2011/01/26 08:06:52, Lei Zhang wrote:
> but I thought you wanted to get rid of variables named "shrink".

Yeah. Since I am using |scale_factor| variable within the loop, its ok to have a
temp variable with name "shrink".

[Lei Zhang, 2011-01-26 19:45:26]

LGTM with nit below.

http://codereview.chromium.org/6261025/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6261025/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:122: ViewMsg_Print_Params
printParams = params.params;
Now that you've renamed print_params to preview_params, can you change
printParams to print_params to be consistent with all the other variables?

[kmadhusu, 2011-01-26 20:00:51]

http://codereview.chromium.org/6261025/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6261025/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:122: ViewMsg_Print_Params
printParams = params.params;
On 2011/01/26 19:45:26, Lei Zhang wrote:
> Now that you've renamed print_params to preview_params, can you change
> printParams to print_params to be consistent with all the other variables?

Done.

[vandebo (ex-Chrome), 2011-01-26 23:38:54]

LGTM

Looking at the larger context just a bit, unless there's a good reason to not
send the PagesReadyForPreview message in CreatePreviewDocument(), it might be
better to move it into there from RenderPagesForPreview.  This would mirror the
PrintPage flow more closely and give you only a single params argument to
CreatePreviewDocument.

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

http://codereview.chromium.org/6261025/diff/13001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:518: // TODO(kmadhusu): Implement this
function for windows & linux.
nit: Update your TODO

http://codereview.chromium.org/6261025/diff/13001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6261025/diff/13001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:118: void
PrintWebViewHelper::CreatePreviewDocument(
nit: It'd be good to document the parameters to this method in the header.
Specifically because most methods tend to take one params struct so pointing out
what they are in this case would be useful.

[kmadhusu, 2011-01-27 00:13:54]

Thanks for the comments.

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

http://codereview.chromium.org/6261025/diff/13001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:518: // TODO(kmadhusu): Implement this
function for windows & linux.
On 2011/01/26 23:38:54, vandebo wrote:
> nit: Update your TODO

Done.

http://codereview.chromium.org/6261025/diff/13001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6261025/diff/13001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:118: void
PrintWebViewHelper::CreatePreviewDocument(
On 2011/01/26 23:38:54, vandebo wrote:
> nit: It'd be good to document the parameters to this method in the header.
> Specifically because most methods tend to take one params struct so pointing
out
> what they are in this case would be useful.

Done.