Implement limited paged media support for linux.

Implement limited paged media support for linux.

BUG=47277
TEST=none

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

[hamaji, 2010-06-23 12:22:37]

Note that we cannot land this patch yet because we have wrong default value in
WebCore/css/html.css .

[Yuzo, 2010-06-24 03:25:12]

http://codereview.chromium.org/2847025/diff/1/2
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/2847025/diff/1/2#newcode20
chrome/renderer/print_web_view_helper_linux.cc:20: static const int kDPI = 72;
This constant is defined and used in multiple places:
- here
- pdf_ps_metafile_cairo.cc
- browser/printing/print_dialog_cloud.cc
- renderer/print_web_view_helper.cc
- renderer/mock_printer.cc
- browser/sync/sync_setup_flow.cc

We might want to define it in
- printing/print_settings.h or
- printing/units.h
perhaps in a separate change.

http://codereview.chromium.org/2847025/diff/1/2#newcode22
chrome/renderer/print_web_view_helper_linux.cc:22: static double
ConvertPixelsToPoints(int pixels) {
I'm not sure, but perhaps we should define this in printing/units.h ?

http://codereview.chromium.org/2847025/diff/1/2#newcode25
chrome/renderer/print_web_view_helper_linux.cc:25: }
We might want to also define:
  static int ConvertPixelsTo*Int*Points
and use it below (line 55, 57),
instead of repeating static_cast<int> and ceil.

http://codereview.chromium.org/2847025/diff/1/4
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/2847025/diff/1/4#newcode58
printing/pdf_ps_metafile_cairo.h:58: double top_margin, double bottom_margin,
I'm not 100% sure, but we might want to order margins as:
  top, right, bottom, left
as CSS margin property does,
  http://www.w3.org/TR/CSS2/box.html#margin-properties
or
  left, right, top, bottom
as PageMargins in page_setup.h does.

[hamaji, 2010-06-28 07:35:16]

I've updated the patch to use the new API.

I need the following patch to run try jobs for this patch properly.

http://codereview.chromium.org/2877001/show

Could you take another look?

http://codereview.chromium.org/2847025/diff/1/2
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/2847025/diff/1/2#newcode20
chrome/renderer/print_web_view_helper_linux.cc:20: static const int kDPI = 72;
On 2010/06/24 03:25:12, Yuzo wrote:
> This constant is defined and used in multiple places:
> - here
> - pdf_ps_metafile_cairo.cc
> - browser/printing/print_dialog_cloud.cc
> - renderer/print_web_view_helper.cc
> - renderer/mock_printer.cc
> - browser/sync/sync_setup_flow.cc
> 
> We might want to define it in
> - printing/print_settings.h or
> - printing/units.h
> perhaps in a separate change.

I'm doing this change in http://codereview.chromium.org/2877001/show

http://codereview.chromium.org/2847025/diff/1/2#newcode22
chrome/renderer/print_web_view_helper_linux.cc:22: static double
ConvertPixelsToPoints(int pixels) {
On 2010/06/24 03:25:12, Yuzo wrote:
> I'm not sure, but perhaps we should define this in printing/units.h ?

I'm doing this change in http://codereview.chromium.org/2877001/show

http://codereview.chromium.org/2847025/diff/1/2#newcode25
chrome/renderer/print_web_view_helper_linux.cc:25: }
On 2010/06/24 03:25:12, Yuzo wrote:
> We might want to also define:
>   static int ConvertPixelsTo*Int*Points
> and use it below (line 55, 57),
> instead of repeating static_cast<int> and ceil.

I'm doing this change in http://codereview.chromium.org/2877001/show

http://codereview.chromium.org/2847025/diff/1/4
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/2847025/diff/1/4#newcode58
printing/pdf_ps_metafile_cairo.h:58: double top_margin, double bottom_margin,
On 2010/06/24 03:25:12, Yuzo wrote:
> I'm not 100% sure, but we might want to order margins as:
>   top, right, bottom, left
> as CSS margin property does,
>   http://www.w3.org/TR/CSS2/box.html#margin-properties
> or
>   left, right, top, bottom
> as PageMargins in page_setup.h does.

Done.

[Yuzo, 2010-06-28 09:38:00]

LGTM

http://codereview.chromium.org/2847025/diff/6001/7001
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/2847025/diff/6001/7001#newcode30
chrome/renderer/print_web_view_helper_linux.cc:30: // The paper size is US
Letter (8.5 in. by 11 in.).
Perhaps we should add a comment like:
  // TODO: We should query the platform about the page size.
? (Or FIXME?)

[hamaji, 2010-06-28 10:06:42]

Thanks for your review!

http://codereview.chromium.org/2847025/diff/6001/7001
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/2847025/diff/6001/7001#newcode30
chrome/renderer/print_web_view_helper_linux.cc:30: // The paper size is US
Letter (8.5 in. by 11 in.).
On 2010/06/28 09:38:00, Yuzo wrote:
> Perhaps we should add a comment like:
>   // TODO: We should query the platform about the page size.
> ? (Or FIXME?)

I moved an existed comment.

[Yuzo, 2010-06-28 12:15:24]

http://codereview.chromium.org/2847025/diff/16001/17001
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/2847025/diff/16001/17001#newcode174
chrome/renderer/print_web_view_helper_linux.cc:174: content_height_in_points,
Sorry, forgot to mention one thing.

What would happen if content_{width,height}_in_points is not equal to
canvas_size.{width,height}() (after converted to the same unit)?

[hamaji, 2010-06-29 07:00:33]

http://codereview.chromium.org/2847025/diff/16001/17001
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/2847025/diff/16001/17001#newcode174
chrome/renderer/print_web_view_helper_linux.cc:174: content_height_in_points,
On 2010/06/28 12:15:24, Yuzo wrote:
> Sorry, forgot to mention one thing.
> 
> What would happen if content_{width,height}_in_points is not equal to
> canvas_size.{width,height}() (after converted to the same unit)?

If content_width is larger than canvas_size.width, the extra width
(content_width - canvas_size.width) won't be utilized. If content_width is
smaller than canvas_size.width, the content will overflow the content_width the
user specified. I think this would be acceptable as CSS3 says the result of this
case is undefined. 

By the way, I found an assertion failure thanks to this comment. I updated my
patch.

[hamaji, 2010-06-29 07:02:16]

On 2010/06/29 07:00:33, hamaji wrote:
> http://codereview.chromium.org/2847025/diff/16001/17001
> File chrome/renderer/print_web_view_helper_linux.cc (right):
> 
> http://codereview.chromium.org/2847025/diff/16001/17001#newcode174
> chrome/renderer/print_web_view_helper_linux.cc:174: content_height_in_points,
> On 2010/06/28 12:15:24, Yuzo wrote:
> > Sorry, forgot to mention one thing.
> > 
> > What would happen if content_{width,height}_in_points is not equal to
> > canvas_size.{width,height}() (after converted to the same unit)?
> 
> If content_width is larger than canvas_size.width, the extra width
> (content_width - canvas_size.width) won't be utilized. If content_width is
> smaller than canvas_size.width, the content will overflow the content_width
the
> user specified. I think this would be acceptable as CSS3 says the result of
this
> case is undefined. 

I think we might eventually want to check Opera's behavior and fix our
implementation if their implementation seems nicer.

[Yuzo, 2010-06-29 07:09:16]

LGTM

Thank you for writing this patch!