PrintPreview: Honor the print media page size and margin values.

PrintPreview: Honor the print media page size and margin values.

BUG=104210, 100819
TEST=Added PrintWebViewHelperPreviewTests.

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

[kmadhusu, 2011-11-17 17:59:46]

Please review and let me know your comments. After your approval, I will submit
the webkit changes for review. 

Thanks.

[vandebo (ex-Chrome), 2011-11-17 23:05:23]

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:449: default_margins_requested_(false),
Should this default to true?

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:658: params.has_page_size_style =
PrintingFrameHasPageSizeStyle(
If we ask for the page size and margins in GetPageSizeAndMargins, but then
ignore them because the result isn't valid, this will give the wrong value. 
Maybe we need to store some state for that case???

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:954: default_margins_requested);
It seems like modifying webkit would be a bit much for what you're tying to
accomplish.  This method is GetPageSizeAndMarginsInPoints.  Seems like you could
also have GetPageSizeInPoints (maybe with a common implmentation), which only
sets page size.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:971: page_layout_in_points, true);
Seems like you could/should change default_margins_requested_ to true here, and
then you wouldn't need to pass it into GetPageSizeAndMarginsInPoints either,
just use the member variable.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1126:
UpdateDefaultMarginsRequested(*job_settings);
If you move default_margins_requested_ to PrintPreviewContext, you can do this
in PrintPreviewContext::OnPrintPreview.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:257: bool default_margins_requested);
No need to pass this down, just access the member var.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:308: bool default_margins_requested_;
Should this go in PrintPreviewContext since it's only used for PP?

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:354: bool default_margins_requested);
No need to pass this down, just access the member var.

[kmadhusu, 2011-12-01 02:15:41]

WIP on Win & Mac.

Please review the latest patch.

Thanks.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:449: default_margins_requested_(false),
On 2011/11/17 23:05:23, vandebo wrote:
> Should this default to true?

Done.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:658: params.has_page_size_style =
PrintingFrameHasPageSizeStyle(
On 2011/11/17 23:05:23, vandebo wrote:
> If we ask for the page size and margins in GetPageSizeAndMargins, but then
> ignore them because the result isn't valid, this will give the wrong value. 
> Maybe we need to store some state for that case???

(repeating our conversation for reference): This is a corner case and hence we
ignore it as of now.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:954: default_margins_requested);
On 2011/11/17 23:05:23, vandebo wrote:
> It seems like modifying webkit would be a bit much for what you're tying to
> accomplish.  This method is GetPageSizeAndMarginsInPoints.  Seems like you
could
> also have GetPageSizeInPoints (maybe with a common implmentation), which only
> sets page size.

As we discussed, I am not doing to modify |pageSizeAndMarginsInPixels| function.
Instead, I made changes to this function.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:971: page_layout_in_points, true);
On 2011/11/17 23:05:23, vandebo wrote:
> Seems like you could/should change default_margins_requested_ to true here,
and
> then you wouldn't need to pass it into GetPageSizeAndMarginsInPoints either,
> just use the member variable.

Done.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1126:
UpdateDefaultMarginsRequested(*job_settings);
On 2011/11/17 23:05:23, vandebo wrote:
> If you move default_margins_requested_ to PrintPreviewContext, you can do this
> in PrintPreviewContext::OnPrintPreview.

Having the member variable in PrintWebViewHelper, I can reuse it for the native
printing workflow.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:257: bool default_margins_requested);
On 2011/11/17 23:05:23, vandebo wrote:
> No need to pass this down, just access the member var.

This is a static function.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:257: bool default_margins_requested);
On 2011/11/17 23:05:23, vandebo wrote:
> No need to pass this down, just access the member var.

(as we discussed) I am ignoring this comment.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:308: bool default_margins_requested_;
On 2011/11/17 23:05:23, vandebo wrote:
> Should this go in PrintPreviewContext since it's only used for PP?

(as we discussed) I am ignoring this comment.

http://codereview.chromium.org/8585017/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:354: bool default_margins_requested);
On 2011/11/17 23:05:23, vandebo wrote:
> No need to pass this down, just access the member var.

(as we discussed) I am ignoring this comment.

[vandebo (ex-Chrome), 2011-12-02 01:28:19]

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

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:951: if (frame) {
If I'm reading this correctly, if ignore_frame_margins_css and fit_to_page are
true, then you don't need to call frame->pageSizeAndMarginsInPixels().

So this can be if (frame && !(ignore_frame_margins_css && fit_to_page))

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:971: double new_printable_width =
is this printable width, page width, or content width?  I think content width

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1017: double ratio_width =
default_printable_width / new_printable_width;
I think we want to scale the CSS page box into the printable area / minimum
margins on the physical page.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1025: margin_top_in_points = 
default_page_size_height/2 - content_height/2;
(default_page_size_height - content_height) / 2;

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1025: margin_top_in_points = 
default_page_size_height/2 - content_height/2;
If we scale the CSS page box into the printable area, then the top margin should
be the printable area top margin plus the css top margin times the scale factor.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1032: int margin_top_bottom =
margin_top_in_points + margin_bottom_in_points;
This doesn't seem to work out.  The formula we worked out for bottom (which
generalized to all of the margins) is:

(physical height - css height * scale factor)/2 + (css bottom margin * scale
factor)

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1062: PrintMsg_Print_Params* params,
pass by value, since we want to change it in the body anyway.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1198:
UpdateFrameMarginsCssInfo(*job_settings);
You can put both of these lines in a !print_for_preview block and remove it from
the assignment.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1248: settings.params.print_to_pdf =
IsPrintToPdfRequested(*job_settings);
I think this can go inside the !print_for_preview block

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

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:315: // True if the previewing/printing
source is html.
Don't we already have ways to figure that out here?

[kmadhusu, 2011-12-02 23:33:19]

Win & Mac => WIP

Addressed review comments.

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

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:951: if (frame) {
On 2011/12/02 01:28:19, vandebo wrote:
> If I'm reading this correctly, if ignore_frame_margins_css and fit_to_page are
> true, then you don't need to call frame->pageSizeAndMarginsInPixels().
> 
> So this can be if (frame && !(ignore_frame_margins_css && fit_to_page))

Done.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:971: double new_printable_width =
On 2011/12/02 01:28:19, vandebo wrote:
> is this printable width, page width, or content width?  I think content width

Done.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1017: double ratio_width =
default_printable_width / new_printable_width;
On 2011/12/02 01:28:19, vandebo wrote:
> I think we want to scale the CSS page box into the printable area / minimum
> margins on the physical page.

Done.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1017: double ratio_width =
default_printable_width / new_printable_width;
On 2011/12/02 01:28:19, vandebo wrote:
> I think we want to scale the CSS page box into the printable area / minimum
> margins on the physical page.

Done.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1025: margin_top_in_points = 
default_page_size_height/2 - content_height/2;
On 2011/12/02 01:28:19, vandebo wrote:
> (default_page_size_height - content_height) / 2;

Done.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1032: int margin_top_bottom =
margin_top_in_points + margin_bottom_in_points;
On 2011/12/02 01:28:19, vandebo wrote:
> This doesn't seem to work out.  The formula we worked out for bottom (which
> generalized to all of the margins) is:
> 
> (physical height - css height * scale factor)/2 + (css bottom margin * scale
> factor)

This is the formula for scaling. I have also updated the centering formula.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1062: PrintMsg_Print_Params* params,
On 2011/12/02 01:28:19, vandebo wrote:
> pass by value, since we want to change it in the body anyway.

Done.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1198:
UpdateFrameMarginsCssInfo(*job_settings);
On 2011/12/02 01:28:19, vandebo wrote:
> You can put both of these lines in a !print_for_preview block and remove it
from
> the assignment.

Done.

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1248: settings.params.print_to_pdf =
IsPrintToPdfRequested(*job_settings);
On 2011/12/02 01:28:19, vandebo wrote:
> I think this can go inside the !print_for_preview block

It is already within that block.

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

http://codereview.chromium.org/8585017/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:315: // True if the previewing/printing
source is html.
On 2011/12/02 01:28:19, vandebo wrote:
> Don't we already have ways to figure that out here?

whoops.. I meant to write "True if we need to auto fit to page else false. When
we print to pdf, we don't fit to page." 

Updated comment.

[vandebo (ex-Chrome), 2011-12-03 02:01:45]

http://codereview.chromium.org/8585017/diff/32001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/8585017/diff/32001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:47:
params->printable_size.SetSize(
You're going to rename this in a different CL?

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/mock_printe...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.cc:55: printable_area_.SetSize(margin_left_,
margin_top_()
I would make these smaller than the margins in the mock printer.

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

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:993: new_content_width =
default_content_width;
I think you don't need these anymore, since you added the clause to the if above

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1015: if (fit_to_page &&
(default_content_height != new_page_box_height ||
I don't think we want to compare the content height and the box (page) height.
If the physical page size is different from the CSS page size, we need to do
*something*

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1034: if (scale_factor)
There are probably some edge cases here where scale factor ends up being greater
than one.  We can probably tweak the conditionals so this doesn't apply.

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1066: (default_content_height -
new_page_box_height)/2  +
Looking at this in code, it looks like just a variant of the above formula with
a factor of 1.  Maybe we can combine them.

[kmadhusu, 2011-12-04 01:55:38]

Win and mac changes are ready for review. Addressed previous review comments. 

As we discussed, Mac+Skia will honor the css settings whereas mac+CG does not
honor the css settings. 

I tried to submit a try job with the modified deps file. But it failed to apply
the patch. I am not sure why. (Ref:
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/6184). 

Thanks.

http://codereview.chromium.org/8585017/diff/32001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/8585017/diff/32001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:47:
params->printable_size.SetSize(
On 2011/12/03 02:01:45, vandebo wrote:
> You're going to rename this in a different CL?

Yes. (http://codereview.chromium.org/8784012/)

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/mock_printe...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.cc:55: printable_area_.SetSize(margin_left_,
margin_top_()
On 2011/12/03 02:01:45, vandebo wrote:
> I would make these smaller than the margins in the mock printer.

Done.

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

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:993: new_content_width =
default_content_width;
On 2011/12/03 02:01:45, vandebo wrote:
> I think you don't need these anymore, since you added the clause to the if
above

Done.

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1015: if (fit_to_page &&
(default_content_height != new_page_box_height ||
On 2011/12/03 02:01:45, vandebo wrote:
> I don't think we want to compare the content height and the box (page) height.
> If the physical page size is different from the CSS page size, we need to do
> *something*

Done.

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1034: if (scale_factor)
On 2011/12/03 02:01:45, vandebo wrote:
> There are probably some edge cases here where scale factor ends up being
greater
> than one.  We can probably tweak the conditionals so this doesn't apply.

When you say "scale factor ends up being greater than one", Did you mean
"factor" value can be greater than one? or "scale_factor" which we got from the
calling function can be greater than 1?

http://codereview.chromium.org/8585017/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1066: (default_content_height -
new_page_box_height)/2  +
On 2011/12/03 02:01:45, vandebo wrote:
> Looking at this in code, it looks like just a variant of the above formula
with
> a factor of 1.  Maybe we can combine them.

Done.

[vandebo (ex-Chrome), 2011-12-04 22:20:34]

Sorry this isn't just a L.G.T.M. with comments.  Just noticed some of the things
this time around.  Give me a call if you want to talk about anything.

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

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:924: void
PrintWebViewHelper::GetPageSizeAndMarginsInPoints(
The variable names in this method get confusing because in one case we're just
updating the values based on a different information source, where as in the
other case we're ignore the other information source's literal values, but using
it to calculate third values.  I wonder if it would make sense to basically
leave this method alone and to create a different method
"FitCssPageSizeAndMarginsToPaperSize()" or something like that, that would take
two PrintMsg_Print_Params - one for the physical page parameters and one for the
CSS parameters.  You would then just call that new function that you've already
modified to call this method with new parameters. In some of them, you might not
need to call the new method, if you know that it's only used in the native flow.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:976: margin_left_in_pixels -
margin_right_in_pixels);
When we are ignoring the css margins, I think these margins
(margin_left_in_pixels, etc) are based on the full page size, not the css page
size, right?  Maybe you don't want to do this step when we're ignoring the css
margins, I'm not sure?

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

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:257: static void
UpdatePrintableSizeInPrintParameters(
The name of this function now needs to be changed....
UpdatePageSizeAndMarginsInFrameAndView ?

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:314: bool ignore_frame_margins_css_;
nit: ignore_frame_css_margins_ or just ignore_css_margins_ ?

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:192:
page_layout_in_points.margin_right +
nit: I'd leave the indent the way it was - it's clearer.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:43: #if defined(USE_SKIA)
I thought that the mac-skia flow was already different.  Is there a reason not
to use this new computation on the mac-cg flow?

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:48: page_size = gfx::Size(
Will this calculation of page_size come out to the same value as the previous
value of page_size?  I suspect it will, but I'm not sure.  If it will, then we
don't need to recompute it.  If it won't, I'll need some help understanding
what's going on.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:95: #if defined(USE_SKIA)
Should this get pulled out into a common function?  Or should we just compute
content_area inside RenderPage?

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:96: //  scale_factor = 1.0f;
extra

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

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:193: width =
page_layout_in_points.content_width +
Why is the width no longer just params.page_size_width()?

[kmadhusu, 2011-12-05 09:06:54]

Addressed review comments. 

Win & Mac => WIP ( Need to test the latest patch)

Thanks.

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

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:924: void
PrintWebViewHelper::GetPageSizeAndMarginsInPoints(
On 2011/12/04 22:20:34, vandebo wrote:
> The variable names in this method get confusing because in one case we're just
> updating the values based on a different information source, where as in the
> other case we're ignore the other information source's literal values, but
using
> it to calculate third values.  I wonder if it would make sense to basically
> leave this method alone and to create a different method
> "FitCssPageSizeAndMarginsToPaperSize()" or something like that, that would
take
> two PrintMsg_Print_Params - one for the physical page parameters and one for
the
> CSS parameters.  You would then just call that new function that you've
already
> modified to call this method with new parameters. In some of them, you might
not
> need to call the new method, if you know that it's only used in the native
flow.

Done.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:976: margin_left_in_pixels -
margin_right_in_pixels);
On 2011/12/04 22:20:34, vandebo wrote:
> When we are ignoring the css margins, I think these margins
> (margin_left_in_pixels, etc) are based on the full page size, not the css page
> size, right?  Maybe you don't want to do this step when we're ignoring the css
> margins, I'm not sure?

Done. 

When the selected printer is "PRINT_TO_PDF", we don't have the physical paper
size information. Therefore, there is no "MINIMUM" margins. We should disable
the minimum margins option.

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

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:257: static void
UpdatePrintableSizeInPrintParameters(
On 2011/12/04 22:20:34, vandebo wrote:
> The name of this function now needs to be changed....
> UpdatePageSizeAndMarginsInFrameAndView ?

Done.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:314: bool ignore_frame_margins_css_;
On 2011/12/04 22:20:34, vandebo wrote:
> nit: ignore_frame_css_margins_ or just ignore_css_margins_ ?

Done. (ignore_frame_margins_css_ => ignore_css_margins_)

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:192:
page_layout_in_points.margin_right +
On 2011/12/04 22:20:34, vandebo wrote:
> nit: I'd leave the indent the way it was - it's clearer.

Done.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:43: #if defined(USE_SKIA)
On 2011/12/04 22:20:34, vandebo wrote:
> I thought that the mac-skia flow was already different.  Is there a reason not
> to use this new computation on the mac-cg flow?

In the mac-cg workflow, we are doing some scaling and translation while
preparing a frame for printing. I will fix this issue later. Currently,  I have
made changes to support mac-skia workflow.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:48: page_size = gfx::Size(
On 2011/12/04 22:20:34, vandebo wrote:
> Will this calculation of page_size come out to the same value as the previous
> value of page_size?  I suspect it will, but I'm not sure.  If it will, then we
> don't need to recompute it.  If it won't, I'll need some help understanding
> what's going on.

When source is html (with a special margin & size css) and the destination
printer is "PRINT_TO_PDF", the page_size is not the same as printer specified
default page size. That is the main reason for calculating the page size here.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:95: #if defined(USE_SKIA)
On 2011/12/04 22:20:34, vandebo wrote:
> Should this get pulled out into a common function?  Or should we just compute
> content_area inside RenderPage?

I created a helper function to update the page size and content area from
page_layout_in_points. In line #75 and #74, we are using the calculated values,
therefore I am little hesitant to have a common function.

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:96: //  scale_factor = 1.0f;
On 2011/12/04 22:20:34, vandebo wrote:
> extra

Removed.

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

http://codereview.chromium.org/8585017/diff/42001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:193: width =
page_layout_in_points.content_width +
On 2011/12/04 22:20:34, vandebo wrote:
> Why is the width no longer just params.page_size_width()?

(same here) 
When source is html (with a special margin & size css) and the destination
printer is "PRINT_TO_PDF", the page_size is not the same as printer specified
default page size. That is the main reason for calculating the page size here.

[vandebo (ex-Chrome), 2011-12-14 19:05:38]

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

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:148: bool ignore_css_margins,
I was hoping to separate the fit/ignore logic from the retrieval of the CSS
information.  i.e. this is the old GetPageSizeAndMargins plus some stuff.  Can
we keep the old GetPageSizeAndMargins intact (regardless of what we call it) and
do the fit to page calculations in a self contained place?

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:246: const PageSizeMargins&
css_page_layout_in_points,
As is, I think you can pass a single PageSizeMargins*

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1218: ignore_css_margins_,
These two will always be false here, just use false?

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

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:122: void
UpdatePageSizeAndContentAreaFromPageLayout(
Should this be GetPageSize... instead of UpdatePageSize... ?

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:256: static void
GetPageSizeAndMarginsInPoints(
Should this name change?  ComputePageLayout?

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:264: 
Add a comment here saying that the page must be prepared for printing before
calling this method, otherwise the print media CSS selectors won't be honored.

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:265: static void
UpdatePageSizeAndMarginsInFrameAndView(
I think a still better name would be
"UpdateFrameAndViewFromCssPageSizeAndMargins"  What do you think?

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:322: bool ignore_css_margins_;
Should these two be in print_preview_context_ ?  They should only ever be true
while doing print preview, right?

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:43: #if defined(USE_SKIA)
Probably should note the bug number that will track the rest of the work on Mac.

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

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:184: *scale_factor =
frame->getPrintPageShrink(page_number);
Don't we need both scale factors?  GetPageSizeANdMarginsInPoints will overwrite
this scale factor.

[kmadhusu, 2011-12-19 09:52:19]

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

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:148: bool ignore_css_margins,
On 2011/12/14 19:05:38, vandebo wrote:
> I was hoping to separate the fit/ignore logic from the retrieval of the CSS
> information.  i.e. this is the old GetPageSizeAndMargins plus some stuff.  Can
> we keep the old GetPageSizeAndMargins intact (regardless of what we call it)
and
> do the fit to page calculations in a self contained place?

Done.

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:246: const PageSizeMargins&
css_page_layout_in_points,
On 2011/12/14 19:05:38, vandebo wrote:
> As is, I think you can pass a single PageSizeMargins*

Done.

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1218: ignore_css_margins_,
On 2011/12/14 19:05:38, vandebo wrote:
> These two will always be false here, just use false?

ignore_css_margins_ is false and fit_to_page_ is true. 

Fixed.

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

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:122: void
UpdatePageSizeAndContentAreaFromPageLayout(
On 2011/12/14 19:05:38, vandebo wrote:
> Should this be GetPageSize... instead of UpdatePageSize... ?

Done.

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:256: static void
GetPageSizeAndMarginsInPoints(
On 2011/12/14 19:05:38, vandebo wrote:
> Should this name change?  ComputePageLayout?

Done.

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:264: 
On 2011/12/14 19:05:38, vandebo wrote:
> Add a comment here saying that the page must be prepared for printing before
> calling this method, otherwise the print media CSS selectors won't be honored.

Done.

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:265: static void
UpdatePageSizeAndMarginsInFrameAndView(
On 2011/12/14 19:05:38, vandebo wrote:
> I think a still better name would be
> "UpdateFrameAndViewFromCssPageSizeAndMargins"  What do you think?

UpdatePageSizeAndMarginsInFrameAndView => 
UpdateFrameAndViewFromCssPageLayout

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:322: bool ignore_css_margins_;
On 2011/12/14 19:05:38, vandebo wrote:
> Should these two be in print_preview_context_ ?  They should only ever be true
> while doing print preview, right?

I prefer to declare these variables here. During the native printing workflow, I
am not happy about calling the getter function from print_preview_context to
access these values. (I think print_preview_context is only for preview
printing).

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:43: #if defined(USE_SKIA)
On 2011/12/14 19:05:38, vandebo wrote:
> Probably should note the bug number that will track the rest of the work on
Mac.

I have made code changes to honor the css settings in Mac+CG

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

http://codereview.chromium.org/8585017/diff/61002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:184: *scale_factor =
frame->getPrintPageShrink(page_number);
On 2011/12/14 19:05:38, vandebo wrote:
> Don't we need both scale factors?  GetPageSizeANdMarginsInPoints will
overwrite
> this scale factor.

Fixed.

[vandebo (ex-Chrome), 2012-01-03 21:55:12]

Sorry for the long delay.  This looks good with just a few details to clean up.

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

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:140: void GetPageCssParams(
nit: GetCssPrintParams ?
nit: return the page_css_params ?

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:225: page_css_params->content_size =
gfx::Size(
Is the value calculated here just default_params.content_size ?

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:233: void
UpdateCssParamsToFitPaperSize(
double/*scale_factor*/ FitPrintParamsToPage(const gfx::Size& page_size, const
gfx::Size& printable_area, PrintMsg_Print_Params* params) ?

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:237: double content_width =
(double)page_css_params->content_size.width();
I don't think a cast is needed, but if it is, use a C++ style cast.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:239: int default_page_size_height =
default_params.page_size.height();
It looks like you don't need the variables introduced in line 239-244. i.e. they
are just aliases.  It's easier to understand what's going on with fewer aliases.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:246: if (default_params.page_size !=
page_css_params->page_size) {
negate this and push it up to the top:
if (default_params.page_size == page_css_params->page_size)
  return;
Should scale_factor be set to 1 before the return?

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:273: void
CalculatePageLayoutFromCssParams(
CalculatePageLayoutFromPrintParams (this isn't CSS specific)

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:833: &print_params, pages,
ignore_css_margins_,
nit: don't need to indent four - will fit equally as well starting on line 832

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1117: PrintMsg_Print_Params
page_css_params = default_params;
I don't think we need to copy it here, since GetPageCssParams will fill it in.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1117: PrintMsg_Print_Params
page_css_params = default_params;
Instead of calling it page_css_params (which is only true until you start
updating it) just call it params?

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1118: GetPageCssParams(frame,
page_index, default_params, &page_css_params);
Is it correct that we don't need to call this if ignore_css_margins &&
fit_to_page?  Then you could do:
if (ignore_css_margins && fit_to_page) {
  page_css_params = default_params;
} else {
  GetPageCssParams(...);
}

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1120:
IgnoreCssMarginsAndUpdateParams(default_params, fit_to_page,
nit: I'd probably put the contents of this method inline.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1143:
PrintWebViewHelper::ComputePageLayout(frame, 0, current_params,
It seems that this caller of ComputePageLayout actually wants
PrintMsg_Print_Params and not PageSizeMargins.  Should we change it to return
PrintMsg_Print_Params and convert to PageSizeMargins using your new function at
the other call sites?

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

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:232: double* scale_factor, int
page_number,
output args should be at the end of the arg list.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:48: page_params.content_area =
gfx::Rect(0, 0, page_size.width(),
Does this still work with the native flow?

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:175: #if !defined(USE_SKIA)
Combine ifdef block with previous.

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

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:198: *page_size_in_dpi = gfx::Size(
is this the same as params.params.page_size in the caller?  If so, we can just
set it there.  Similarly for content_area_in_dpi.

http://codereview.chromium.org/8585017/diff/91002/printing/printed_document.cc
File printing/printed_document.cc (left):

http://codereview.chromium.org/8585017/diff/91002/printing/printed_document.c...
printing/printed_document.cc:90: DCHECK_EQ(mutable_.shrink_factor, shrink);
I think this DCHECK is saying that shrink must be consistent across all pages. 
Are we changing that assumption?

[kmadhusu, 2012-01-04 16:55:35]

vandebo: Addressed most of the review comments. Still working on the mac code
review comment and mutable_.shrink_factor review comment. 

Thanks.

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

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:140: void GetPageCssParams(
On 2012/01/03 21:55:12, vandebo wrote:
> nit: GetCssPrintParams ?
> nit: return the page_css_params ?

Done.

PrintMsg_Print_Params GetCssPageParams(...);

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:225: page_css_params->content_size =
gfx::Size(
On 2012/01/03 21:55:12, vandebo wrote:
> Is the value calculated here just default_params.content_size ?

No. page_css_params->page_size != default_params.page_size.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:233: void
UpdateCssParamsToFitPaperSize(
On 2012/01/03 21:55:12, vandebo wrote:
> double/*scale_factor*/ FitPrintParamsToPage(const gfx::Size& page_size, const
> gfx::Size& printable_area, PrintMsg_Print_Params* params) ?

Done.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:237: double content_width =
(double)page_css_params->content_size.width();
On 2012/01/03 21:55:12, vandebo wrote:
> I don't think a cast is needed, but if it is, use a C++ style cast.

Done. (double) => static_cast<double>()

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:239: int default_page_size_height =
default_params.page_size.height();
On 2012/01/03 21:55:12, vandebo wrote:
> It looks like you don't need the variables introduced in line 239-244. i.e.
they
> are just aliases.  It's easier to understand what's going on with fewer
aliases.

Removed |margin_top| and |margin_left|.
|new_page_box_height| => |css_page_size_height|
|new_page_box_width| => |css_page_size_width|

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:246: if (default_params.page_size !=
page_css_params->page_size) {
On 2012/01/03 21:55:12, vandebo wrote:
> negate this and push it up to the top:
> if (default_params.page_size == page_css_params->page_size)
>   return;

Done.

> Should scale_factor be set to 1 before the return?

scale_factor is always set at the calling site.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:273: void
CalculatePageLayoutFromCssParams(
On 2012/01/03 21:55:12, vandebo wrote:
> CalculatePageLayoutFromPrintParams (this isn't CSS specific)

Done.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:833: &print_params, pages,
ignore_css_margins_,
On 2012/01/03 21:55:12, vandebo wrote:
> nit: don't need to indent four - will fit equally as well starting on line 832

Done.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1117: PrintMsg_Print_Params
page_css_params = default_params;
On 2012/01/03 21:55:12, vandebo wrote:
> Instead of calling it page_css_params (which is only true until you start
> updating it) just call it params?

Done. page_css_params => params

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1117: PrintMsg_Print_Params
page_css_params = default_params;
On 2012/01/03 21:55:12, vandebo wrote:
> I don't think we need to copy it here, since GetPageCssParams will fill it in.

GetPageCssParams only fills "page_size, content_size, margin_left and
margin_top" member variables. Therefore, I initialize the struct variable with
default_params values.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1118: GetPageCssParams(frame,
page_index, default_params, &page_css_params);
On 2012/01/03 21:55:12, vandebo wrote:
> Is it correct that we don't need to call this if ignore_css_margins &&
> fit_to_page?  Then you could do:
> if (ignore_css_margins && fit_to_page) {
>   page_css_params = default_params;
> } else {
>   GetPageCssParams(...);
> }

Done.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1120:
IgnoreCssMarginsAndUpdateParams(default_params, fit_to_page,
On 2012/01/03 21:55:12, vandebo wrote:
> nit: I'd probably put the contents of this method inline.

Done.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1143:
PrintWebViewHelper::ComputePageLayout(frame, 0, current_params,
On 2012/01/03 21:55:12, vandebo wrote:
> It seems that this caller of ComputePageLayout actually wants
> PrintMsg_Print_Params and not PageSizeMargins.  Should we change it to return
> PrintMsg_Print_Params and convert to PageSizeMargins using your new function
at
> the other call sites?

ComputePageLayout is divided into two different functions:
(1) CalculatePrintParams()
(2) CalculatePageLayoutFromPrintParams().

From here, I am calling CalculatePrintParams().

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

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:232: double* scale_factor, int
page_number,
On 2012/01/03 21:55:12, vandebo wrote:
> output args should be at the end of the arg list.

oops.. fixed.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:48: page_params.content_area =
gfx::Rect(0, 0, page_size.width(),
On 2012/01/03 21:55:12, vandebo wrote:
> Does this still work with the native flow?

Yes. Still working on a fix to address this issue.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:175: #if !defined(USE_SKIA)
On 2012/01/03 21:55:12, vandebo wrote:
> Combine ifdef block with previous.

Done.

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

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:198: *page_size_in_dpi = gfx::Size(
On 2012/01/03 21:55:12, vandebo wrote:
> is this the same as params.params.page_size in the caller?  If so, we can just
> set it there.  Similarly for content_area_in_dpi.

page_size != params.params.page_size. We set |page_size| values in
GetPageSizeAndContentAreaFromPageLayout() function. |page_size_in_dpi| is NULL
for the preview workflow. It is set only during the printing workflow. 

Similarly for |content_area_in_dpi|

http://codereview.chromium.org/8585017/diff/91002/printing/printed_document.cc
File printing/printed_document.cc (left):

http://codereview.chromium.org/8585017/diff/91002/printing/printed_document.c...
printing/printed_document.cc:90: DCHECK_EQ(mutable_.shrink_factor, shrink);
On 2012/01/03 21:55:12, vandebo wrote:
> I think this DCHECK is saying that shrink must be consistent across all pages.

> Are we changing that assumption?

Still working on this.

[kmadhusu, 2012-01-05 01:15:47]

vandebo: Please review the latest patch. Addressed win and mac issues.

Thanks.

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/91002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:48: page_params.content_area =
gfx::Rect(0, 0, page_size.width(),
On 2012/01/04 16:55:35, kmadhusu wrote:
> On 2012/01/03 21:55:12, vandebo wrote:
> > Does this still work with the native flow?
> 
> Yes. Still working on a fix to address this issue.

Fixed. Modified pdf_metafile_cg_mac::RenderPage() function to support the
change.

http://codereview.chromium.org/8585017/diff/91002/printing/printed_document.cc
File printing/printed_document.cc (left):

http://codereview.chromium.org/8585017/diff/91002/printing/printed_document.c...
printing/printed_document.cc:90: DCHECK_EQ(mutable_.shrink_factor, shrink);
On 2012/01/04 16:55:35, kmadhusu wrote:
> On 2012/01/03 21:55:12, vandebo wrote:
> > I think this DCHECK is saying that shrink must be consistent across all
pages.
> 
> > Are we changing that assumption?
> 
> Still working on this.

Moved shrink_factor to PrintedPage class.

[vandebo (ex-Chrome), 2012-01-07 00:23:19]

Except for the mac comments, it's all nits.

Is there a way that we can easily add tests for the CSS centering/scaling code?

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

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:210: PrintMsg_Print_Params*
page_css_params) {
nit: default_params -> page_params ?
nit: page_css_params -> params_to_fit or fit_params ?

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1084: PrintMsg_Print_Params
CalculatePrintParams(
So when we have no CSS, this is a no-op, right?  Should we then call it
CalculatePrintParamsForCss?

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1084: PrintMsg_Print_Params
CalculatePrintParams(
Move this to the anonymous namespace.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1093: } else {
Since the if body returns, don't use an else, just continue from there.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1103: if (fit_to_page) {
This if isn't needed, because you handeld the ignore_css_margins & fit_to_page
case above.  Add a DCHECK when you remove the if?

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1107: int default_margin_right=
default_params.page_size.width() -
nit space: default_margin_right =

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1119: if (fit_to_page) {
note: This could be an else if because the case where they are both true is
already handled.  (But don't do that.)

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1129: void
PrintWebViewHelper::ComputePageLayoutInPoints(
ComputePageLayoutInPointsForCss

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

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:231: printing::Metafile*
RenderPage(const PrintMsg_Print_Params& params,
It looks like the Win and Mac prototypes can be unforked now... except we don't
actually need scale_factor on Mac, nevermind.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:232: int page_number, bool is_preview,
match the parameter order to the Mac function (i.e. swap is_preview and frame.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:47: page_params.actual_shrink =
scale_factor;
Since this isn't used, can we clean up the RenderPgae call by not passing in the
scale_factor?

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:191: webkit_scale_factor =
frame->printPage(page_number, canvas_ptr);
We don't need the return value...

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_win.cc:184: WebFrame* frame, Metafile*
metafile, double* scale_factor,
Since there are a lot of factors, lets make the names a bit more explicit:
scale_factor -> actual_shrink or final_scale_factor
shrink_factor -> css_scale_factor

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_win.cc:195: if (!is_preview) {
Remove this if, since you already have the NULL protection.

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
File printing/pdf_metafile_cg_mac.cc (left):

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
printing/pdf_metafile_cg_mac.cc:218: y_offset += rect.size.height -
(source_rect.size.height * scaling_factor);
I think you still need this code.

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
File printing/pdf_metafile_cg_mac.cc (right):

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
printing/pdf_metafile_cg_mac.cc:205: float x_offset = source_rect.origin.x *
scaling_factor;
You changed the sign.  These should be negative.

http://codereview.chromium.org/8585017/diff/105022/printing/printed_page.h
File printing/printed_page.h (right):

http://codereview.chromium.org/8585017/diff/105022/printing/printed_page.h#ne...
printing/printed_page.h:61: // Shrink done in comparison to desired_dpi. To
support multiple shrink
I would remove the "To..." sentence.  It only makes sense if you know that
shrink_factor used to be in printed document

[kmadhusu, 2012-01-09 17:15:55]

Addressed review comments.

Thanks.

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

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:210: PrintMsg_Print_Params*
page_css_params) {
On 2012/01/07 00:23:19, vandebo wrote:
> nit: default_params -> page_params ?
> nit: page_css_params -> params_to_fit or fit_params ?

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1084: PrintMsg_Print_Params
CalculatePrintParams(
On 2012/01/07 00:23:19, vandebo wrote:
> Move this to the anonymous namespace.

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1084: PrintMsg_Print_Params
CalculatePrintParams(
On 2012/01/07 00:23:19, vandebo wrote:
> So when we have no CSS, this is a no-op, right?  Should we then call it
> CalculatePrintParamsForCss?

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1093: } else {
On 2012/01/07 00:23:19, vandebo wrote:
> Since the if body returns, don't use an else, just continue from there.

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1103: if (fit_to_page) {
On 2012/01/07 00:23:19, vandebo wrote:
> This if isn't needed, because you handeld the ignore_css_margins & fit_to_page
> case above.  Add a DCHECK when you remove the if?

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1107: int default_margin_right=
default_params.page_size.width() -
On 2012/01/07 00:23:19, vandebo wrote:
> nit space: default_margin_right =

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1119: if (fit_to_page) {
On 2012/01/07 00:23:19, vandebo wrote:
> note: This could be an else if because the case where they are both true is
> already handled.  (But don't do that.)

ok

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1129: void
PrintWebViewHelper::ComputePageLayoutInPoints(
On 2012/01/07 00:23:19, vandebo wrote:
> ComputePageLayoutInPointsForCss

Done.

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

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:231: printing::Metafile*
RenderPage(const PrintMsg_Print_Params& params,
On 2012/01/07 00:23:19, vandebo wrote:
> It looks like the Win and Mac prototypes can be unforked now... except we
don't
> actually need scale_factor on Mac, nevermind.

okay.
Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:232: int page_number, bool is_preview,
On 2012/01/07 00:23:19, vandebo wrote:
> match the parameter order to the Mac function (i.e. swap is_preview and frame.

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:47: page_params.actual_shrink =
scale_factor;
On 2012/01/07 00:23:19, vandebo wrote:
> Since this isn't used, can we clean up the RenderPgae call by not passing in
the
> scale_factor?

Removed scale_factor in RenderPage params list. 

page_params.actual_shrink = 1.0f;

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:191: webkit_scale_factor =
frame->printPage(page_number, canvas_ptr);
On 2012/01/07 00:23:19, vandebo wrote:
> We don't need the return value...

Fixed.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_win.cc:184: WebFrame* frame, Metafile*
metafile, double* scale_factor,
On 2012/01/07 00:23:19, vandebo wrote:
> Since there are a lot of factors, lets make the names a bit more explicit:
> scale_factor -> actual_shrink or final_scale_factor
> shrink_factor -> css_scale_factor

Done.

http://codereview.chromium.org/8585017/diff/105022/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_win.cc:195: if (!is_preview) {
On 2012/01/07 00:23:19, vandebo wrote:
> Remove this if, since you already have the NULL protection.

Moved page_size_in_dpi and content_area_in_dpi out of if(!is_preview)

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
File printing/pdf_metafile_cg_mac.cc (left):

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
printing/pdf_metafile_cg_mac.cc:218: y_offset += rect.size.height -
(source_rect.size.height * scaling_factor);
On 2012/01/07 00:23:19, vandebo wrote:
> I think you still need this code.

This code is not required with the new changes.

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
File printing/pdf_metafile_cg_mac.cc (right):

http://codereview.chromium.org/8585017/diff/105022/printing/pdf_metafile_cg_m...
printing/pdf_metafile_cg_mac.cc:205: float x_offset = source_rect.origin.x *
scaling_factor;
On 2012/01/07 00:23:19, vandebo wrote:
> You changed the sign.  These should be negative.

Done.

http://codereview.chromium.org/8585017/diff/105022/printing/printed_page.h
File printing/printed_page.h (right):

http://codereview.chromium.org/8585017/diff/105022/printing/printed_page.h#ne...
printing/printed_page.h:61: // Shrink done in comparison to desired_dpi. To
support multiple shrink
On 2012/01/07 00:23:19, vandebo wrote:
> I would remove the "To..." sentence.  It only makes sense if you know that
> shrink_factor used to be in printed document

Done.

[vandebo (ex-Chrome), 2012-01-09 19:04:02]

Lets get in what ever testing can be done and then this is good to go.

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

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:144: PrintMsg_Print_Params
params_to_fit = page_params;
nit: this method isn't related to fit to page.

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:249: const PrintMsg_Print_Params&
params_to_fit,
nit: params ?

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:408: const PrintMsg_Print_Params&
page_params,
nit: we generally use params instead of page_params

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1125: const PrintMsg_Print_Params&
page_params,
nit: and here.

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:46: page_params.actual_shrink =
1.0f;
nit: I think you said Linux doesn't even set this value.  Lets do the same here.

[kmadhusu, 2012-01-09 23:49:26]

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

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:144: PrintMsg_Print_Params
params_to_fit = page_params;
On 2012/01/09 19:04:02, vandebo wrote:
> nit: this method isn't related to fit to page.

params_to_fit => page_css_params

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:249: const PrintMsg_Print_Params&
params_to_fit,
On 2012/01/09 19:04:02, vandebo wrote:
> nit: params ?

Done.

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:408: const PrintMsg_Print_Params&
page_params,
On 2012/01/09 19:04:02, vandebo wrote:
> nit: we generally use params instead of page_params

We already use params in line 415. So leaving this as it is.

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1125: const PrintMsg_Print_Params&
page_params,
On 2012/01/09 19:04:02, vandebo wrote:
> nit: and here.

We already use params in line 1130. So leaving this as it is.

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/8585017/diff/125001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:46: page_params.actual_shrink =
1.0f;
On 2012/01/09 19:04:02, vandebo wrote:
> nit: I think you said Linux doesn't even set this value.  Lets do the same
here.

Done.

[vandebo (ex-Chrome), 2012-01-10 00:59:55]

LGTM

http://codereview.chromium.org/8585017/diff/128004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/8585017/diff/128004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_browsertest.cc:404: void
VerifyDefaultPageLayout(int expected_width, int expected_height,
nit: content_width, content_height

http://codereview.chromium.org/8585017/diff/128004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_browsertest.cc:566: }
How about a test with CSS page size larger than the page?

[kmadhusu, 2012-01-10 17:35:45]

http://codereview.chromium.org/8585017/diff/128004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/8585017/diff/128004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_browsertest.cc:404: void
VerifyDefaultPageLayout(int expected_width, int expected_height,
On 2012/01/10 00:59:55, vandebo wrote:
> nit: content_width, content_height

Done.

http://codereview.chromium.org/8585017/diff/128004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_browsertest.cc:566: }
On 2012/01/10 00:59:55, vandebo wrote:
> How about a test with CSS page size larger than the page?

sure. Done.

[vandebo (ex-Chrome), 2012-01-10 17:49:14]

LGTM