Linux: fix printing somewhat....

Linux: fix printing somewhat.

BUG=29148
TEST=prints documents greater than one page (to real printers)


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

[Evan Martin, 2010-04-03 02:17:02]

I don't know this code very well.  I think Tony is on vacation.  I may take
another look Monday and try my best.

[darin (slow to review), 2010-04-03 04:56:09]

http://codereview.chromium.org/1520014/diff/1/5
File third_party/WebKit/WebKit/chromium/src/WebFrameImpl.cpp (right):

http://codereview.chromium.org/1520014/diff/1/5#newcode294
third_party/WebKit/WebKit/chromium/src/WebFrameImpl.cpp:294: // On Linux, we
don't have the problem with NativeTheme, hence we let WebKit
I wonder if we could let WebKit perform the scaling on Windows as well if we
made some changes to the WebThemeEngine API to expose the scaling factor.

[tony, 2010-04-05 04:07:06]

I don't know this code very well, but it looks like you're moving the scaling
from cairo to WebKit.  The approach seems fine to me, just some high level
questions.

Why does this fix printing documents greater than one page?

Are there other tradeoffs with having WebKit do the scale instead? I wonder if
when we start painting widgets if they will be scaled wrong as the comment seems
to suggest happens on Windows (this is probably fine for now).

[Evan Stade, 2010-04-05 18:44:17]

On 2010/04/05 04:07:06, tony wrote:
> I don't know this code very well, but it looks like you're moving the scaling
> from cairo to WebKit.  The approach seems fine to me, just some high level
> questions.
> 
> Why does this fix printing documents greater than one page?

I am not entirely sure. I started off just trying to understand
pdf_ps_metafile_cairo, then I rewrote some parts of it that I thought were
overly complicated. Then the problem magically went away.

This also fixes another problem which might be related: you can now select text
in the generated pdf. It seems the old approach just kind of copied the pdf as a
big image, not text + drawing commands + images etc.

> Are there other tradeoffs with having WebKit do the scale instead?

not that I know of. +maruel

> I wonder if when we start painting widgets if they will be scaled wrong as the
comment seems
> to suggest happens on Windows (this is probably fine for now).

it seems to work correctly. Although both with and without this patch, web
widgets are printed all black if you go straight to printer (but this doesn't
happen if you print to file).

[tony, 2010-04-06 00:01:12]

On 2010/04/05 18:44:17, Evan Stade wrote:
> This also fixes another problem which might be related: you can now select
text
> in the generated pdf. It seems the old approach just kind of copied the pdf as
a
> big image, not text + drawing commands + images etc.

I vaguely recall Min-yu had it working with embedding text (rather than glyphs)
before, but it didn't work with the sandbox.  Maybe that was a different
approach.

If this works with the sandbox, then this LGTM.

[Evan Stade, 2010-04-06 20:42:24]

yep, works in sandbox. will cook up the webkit patch now.

[Evan Stade, 2010-04-06 20:50:27]

On 2010/04/06 20:42:24, Evan Stade wrote:
> yep, works in sandbox. will cook up the webkit patch now.

https://bugs.webkit.org/show_bug.cgi?id=37168

[M-A Ruel, 2010-04-07 13:51:42]

The compile error on the linux try job looks genuine.

http://codereview.chromium.org/1520014/diff/1/3
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/1520014/diff/1/3#newcode21
printing/pdf_ps_metafile_cairo.cc:21: // The hardcoded margins, in points. These
values are based on 72 dpi,
I was surprised to see these margins hardcoded in the old code, I don't think
they are necessary at all, you know why they were there?

http://codereview.chromium.org/1520014/diff/1/3#newcode92
printing/pdf_ps_metafile_cairo.cc:92: case PDF:
nit: The {} were per style guide BTW but for such simple code, I don't care.

http://codereview.chromium.org/1520014/diff/1/3#newcode107
printing/pdf_ps_metafile_cairo.cc:107: // Dont let WebKit draw over the margins.
Don't

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

http://codereview.chromium.org/1520014/diff/1/2#newcode91
printing/pdf_ps_metafile_cairo.h:91: std::string data_;
I'm afraid this will lead to heavy memory fragmentation. Since printed data can
grow quite large (several tens of megs, up to hundredths) and the way
std::string grows is undetermined, it may lead to a mess in the heap. I'm not
sure what would be the best solution, a list of 4kb array?There's no such thing
in STL AFAIK.

http://codereview.chromium.org/1520014/diff/1/5
File third_party/WebKit/WebKit/chromium/src/WebFrameImpl.cpp (right):

http://codereview.chromium.org/1520014/diff/1/5#newcode294
third_party/WebKit/WebKit/chromium/src/WebFrameImpl.cpp:294: // On Linux, we
don't have the problem with NativeTheme, hence we let WebKit
On 2010/04/03 04:56:09, darin wrote:
> I wonder if we could let WebKit perform the scaling on Windows as well if we
> made some changes to the WebThemeEngine API to expose the scaling factor.

nit: I'm also a bit afraid about discrepancy between platforms but I will let it
go for now.

[M-A Ruel, 2010-04-07 13:53:35]

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

http://codereview.chromium.org/1520014/diff/1/2#newcode91
printing/pdf_ps_metafile_cairo.h:91: std::string data_;
 2010/04/07 13:51:42, Marc-Antoine Ruel wrote:
> I'm afraid this will lead to heavy memory fragmentation. Since printed data
can
> grow quite large (several tens of megs, up to hundredths) and the way
> std::string grows is undetermined, it may lead to a mess in the heap. I'm not
> sure what would be the best solution, a list of 4kb array?There's no such
thing
> in STL AFAIK.

BTW I see that the code was already doing this. I'm just wondering if it could
be changed to a
std::list<std::string> with each page being one string. It'd alleviate the
problem a bit especially for documents with a lot of pages. Any thoughts?

[Evan Stade, 2010-04-07 19:21:59]

test fixed.

http://codereview.chromium.org/1520014/diff/1/3
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/1520014/diff/1/3#newcode21
printing/pdf_ps_metafile_cairo.cc:21: // The hardcoded margins, in points. These
values are based on 72 dpi,
On 2010/04/07 13:51:42, Marc-Antoine Ruel wrote:
> I was surprised to see these margins hardcoded in the old code, I don't think
> they are necessary at all, you know why they were there?

yes, they are necessary. Without margins we get a pdf that is right up against
the edges.

http://codereview.chromium.org/1520014/diff/1/3#newcode92
printing/pdf_ps_metafile_cairo.cc:92: case PDF:
On 2010/04/07 13:51:42, Marc-Antoine Ruel wrote:
> nit: The {} were per style guide BTW but for such simple code, I don't care.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

The two options to fix the style here would be to move the breaks inside the
curlies or delete the curlies. I chose the latter.

http://codereview.chromium.org/1520014/diff/1/3#newcode107
printing/pdf_ps_metafile_cairo.cc:107: // Dont let WebKit draw over the margins.
On 2010/04/07 13:51:42, Marc-Antoine Ruel wrote:
> Don't

Done.

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

http://codereview.chromium.org/1520014/diff/1/2#newcode91
printing/pdf_ps_metafile_cairo.h:91: std::string data_;
it seems worthwhile but in the interest of keeping this change atomic I'd like
to defer doing it for now.

(however, I'd think that any reasonable string implementation would grow the
array exponentially, which would somewhat limit the damage.)

[M-A Ruel, 2010-04-07 19:27:31]

I'm still worried about the margins as it should use the one specified by the
browser but it's out of scope for this change.

lgtm

[Evan Stade, 2010-04-07 20:13:53]

we don't have any settings of that sort hooked up :-(