Implement limited paged media support for win.

Implement limited paged media support for win.

BUG=47277
TEST=none

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

[Yuzo, 2010-07-01 09:33:39]

Hi, thank you for writing this patch.

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

http://codereview.chromium.org/2859040/diff/1/4#newcode119
chrome/renderer/print_web_view_helper_win.cc:119:
static_cast<int>(ConvertUnitDouble(
Why do you use
  static_cast<int>(ConvertUnitDouble(...))
instead of
  ConvertUnit(...)
?

http://codereview.chromium.org/2859040/diff/1/4#newcode121
chrome/renderer/print_web_view_helper_win.cc:121: + margin_left_in_points +
margin_right_in_points,
"When expressions are wrapped, the operator should be on the end of the broken
line, not the start of the new line."
http://www.chromium.org/developers/coding-style

(I think I saw the same issue in the file for linux.)

http://codereview.chromium.org/2859040/diff/1/6
File printing/printed_document_win.cc (right):

http://codereview.chromium.org/2859040/diff/1/6#newcode64
printing/printed_document_win.cc:64: }
How about:
  extract this as a function,
  move it to printed_document.cc, and
  share it with the Mac code?

http://codereview.chromium.org/2859040/diff/1/7
File printing/printed_page.h (right):

http://codereview.chromium.org/2859040/diff/1/7#newcode34
printing/printed_page.h:34: const bool use_overlays() const { return
use_overlays_; }
Isn't this a bit confusing with PrintSettings::use_overlays?

Perhaps overlays_visible is better?

[hamaji, 2010-07-01 11:24:26]

Sorry, somehow my previous patch was wrong. So, there are a few more modified
files in the new patch.

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

http://codereview.chromium.org/2859040/diff/1/4#newcode119
chrome/renderer/print_web_view_helper_win.cc:119:
static_cast<int>(ConvertUnitDouble(
On 2010/07/01 09:33:40, Yuzo wrote:
> Why do you use
>   static_cast<int>(ConvertUnitDouble(...))
> instead of
>   ConvertUnit(...)
> ?

ConvertUnit takes an integer as its argument. So, rounding will happen twice if
we use ConvertUnit.

http://codereview.chromium.org/2859040/diff/1/4#newcode121
chrome/renderer/print_web_view_helper_win.cc:121: + margin_left_in_points +
margin_right_in_points,
On 2010/07/01 09:33:40, Yuzo wrote:
> "When expressions are wrapped, the operator should be on the end of the broken
> line, not the start of the new line."
> http://www.chromium.org/developers/coding-style
> 
> (I think I saw the same issue in the file for linux.)

Done.

http://codereview.chromium.org/2859040/diff/1/6
File printing/printed_document_win.cc (right):

http://codereview.chromium.org/2859040/diff/1/6#newcode64
printing/printed_document_win.cc:64: }
On 2010/07/01 09:33:40, Yuzo wrote:
> How about:
>   extract this as a function,
>   move it to printed_document.cc, and
>   share it with the Mac code? 

Thanks for this suggestion. I think we should do this refactoring. However, can
I do this in a separate change after this change is landed? I'm not good at
development in Windows so I'd like to create another patch on Mac.

http://codereview.chromium.org/2859040/diff/1/7
File printing/printed_page.h (right):

http://codereview.chromium.org/2859040/diff/1/7#newcode34
printing/printed_page.h:34: const bool use_overlays() const { return
use_overlays_; }
On 2010/07/01 09:33:40, Yuzo wrote:
> Isn't this a bit confusing with PrintSettings::use_overlays?
> 
> Perhaps overlays_visible is better?

Nice suggestion! Done. I used "has_visible_overlays" as its name.

[Yuzo, 2010-07-01 11:34:26]

On 2010/07/01 11:24:26, hamaji wrote:
> Sorry, somehow my previous patch was wrong. So, there are a few more modified
> files in the new patch.
> 
> http://codereview.chromium.org/2859040/diff/1/4
> File chrome/renderer/print_web_view_helper_win.cc (right):
> 
> http://codereview.chromium.org/2859040/diff/1/4#newcode119
> chrome/renderer/print_web_view_helper_win.cc:119:
> static_cast<int>(ConvertUnitDouble(
> On 2010/07/01 09:33:40, Yuzo wrote:
> > Why do you use
> >   static_cast<int>(ConvertUnitDouble(...))
> > instead of
> >   ConvertUnit(...)
> > ?
> 
> ConvertUnit takes an integer as its argument. So, rounding will happen twice
if
> we use ConvertUnit.

I see. Sad.

> 
> http://codereview.chromium.org/2859040/diff/1/4#newcode121
> chrome/renderer/print_web_view_helper_win.cc:121: + margin_left_in_points +
> margin_right_in_points,
> On 2010/07/01 09:33:40, Yuzo wrote:
> > "When expressions are wrapped, the operator should be on the end of the
broken
> > line, not the start of the new line."
> > http://www.chromium.org/developers/coding-style
> > 
> > (I think I saw the same issue in the file for linux.)
> 
> Done.
> 
> http://codereview.chromium.org/2859040/diff/1/6
> File printing/printed_document_win.cc (right):
> 
> http://codereview.chromium.org/2859040/diff/1/6#newcode64
> printing/printed_document_win.cc:64: }
> On 2010/07/01 09:33:40, Yuzo wrote:
> > How about:
> >   extract this as a function,
> >   move it to printed_document.cc, and
> >   share it with the Mac code? 
> 
> Thanks for this suggestion. I think we should do this refactoring. However,
can
> I do this in a separate change after this change is landed? I'm not good at
> development in Windows so I'd like to create another patch on Mac.
> 

Certainly you can.

> http://codereview.chromium.org/2859040/diff/1/7
> File printing/printed_page.h (right):
> 
> http://codereview.chromium.org/2859040/diff/1/7#newcode34
> printing/printed_page.h:34: const bool use_overlays() const { return
> use_overlays_; }
> On 2010/07/01 09:33:40, Yuzo wrote:
> > Isn't this a bit confusing with PrintSettings::use_overlays?
> > 
> > Perhaps overlays_visible is better?
> 
> Nice suggestion! Done. I used "has_visible_overlays" as its name.

Good choice.

I'll send other comments shortly.

[Yuzo, 2010-07-01 11:40:17]

LGTM

http://codereview.chromium.org/2859040/diff/6001/7002
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/2859040/diff/6001/7002#newcode465
chrome/common/render_messages.h:465: // False if the overlay is not visible in
this page.
s/overlay is/overlays are/ ?

I also think
"True if the page has visible overlays." is more straightforward than
"False ... not ..." .

[hamaji, 2010-07-01 11:43:20]

http://codereview.chromium.org/2859040/diff/6001/7002
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/2859040/diff/6001/7002#newcode465
chrome/common/render_messages.h:465: // False if the overlay is not visible in
this page.
On 2010/07/01 11:40:18, Yuzo wrote:
> s/overlay is/overlays are/ ?
> 
> I also think
> "True if the page has visible overlays." is more straightforward than
> "False ... not ..." .

Done.