Implement CGdiPrinterDriver::DrawDeviceText(). (issue 2113563003 by thestig@chromium.org)

[the...@chromium.org]

Reviewers: bungeman-skia, dsinclair1
CL: https://codereview.chromium.org/2113563003/

Message:
I hope eventually we'll move to a world with DirectWrite and XPS output, but in
the meanwhile, we are doing GDI + EMF. This is the PDFium side and it needs to
land first. The Chromium side is https://codereview.chromium.org/2114583002 and
it needs this CL and a PDFium DEPS roll.


https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp
File core/fxge/win32/fx_win32_print.cpp (right):

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp#newcode22
core/fxge/win32/fx_win32_print.cpp:22: bool g_pdfium_print_text_with_gdi
= false;
dsinclair: Given the somewhat experimental status, I don't want to add a
public/ API for these. Instead, I'm using the awful technique of
declaring "extern bool g_pdfium_print_text_with_gdi" elsewhere. WDYT?

Description:
Implement CGdiPrinterDriver::DrawDeviceText().

This is sufficient to print text with GDI for PDFs generated by Chromium
and cannot print any arbitrary PDF. Text that cannot be printed will be
drawn as glyphs as before.

BUG=409472

Base URL: https://pdfium.googlesource.com/pdfium@master

Affected files (+124, -10 lines):
M core/fxge/win32/fx_win32_print.cpp
M core/fxge/win32/win32_int.h

[dsin...@chromium.org]

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp
File core/fxge/win32/fx_win32_print.cpp (right):

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp#newcode22
core/fxge/win32/fx_win32_print.cpp:22: bool g_pdfium_print_text_with_gdi
= false;

On 2016/06/30 07:54:08, Lei Zhang wrote:
> dsinclair: Given the somewhat experimental status, I don't want to add
a public/
> API for these. Instead, I'm using the awful technique of declaring
"extern bool
> g_pdfium_print_text_with_gdi" elsewhere. WDYT?

Why not make it a compile option that's off by default. As far as I
know, we aren't guaranteed to keep API consistency with things that
aren't compiled by default, yea?

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp#newcode191
core/fxge/win32/fx_win32_print.cpp:191: // Note that |pFont| has the
actual font to render with embedded within, but
This sounds like more of a TODO?

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp#newcode218
core/fxge/win32/fx_win32_print.cpp:218: // obviously do not work for
arbitrary PDFs.
nit: s/do/does/

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp
File core/fxge/win32/fx_win32_print.cpp (right):

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp#newcode22
core/fxge/win32/fx_win32_print.cpp:22: bool g_pdfium_print_text_with_gdi
= false;

On 2016/06/30 13:31:57, dsinclair wrote:
> On 2016/06/30 07:54:08, Lei Zhang wrote:
> > dsinclair: Given the somewhat experimental status, I don't want to
add a
> public/
> > API for these. Instead, I'm using the awful technique of declaring
"extern
> bool
> > g_pdfium_print_text_with_gdi" elsewhere. WDYT?
>
>
> Why not make it a compile option that's off by default. As far as I
know, we
> aren't guaranteed to keep API consistency with things that aren't
compiled by
> default, yea?

It's more complicated, but done in patch set 2. The ensure typeface
function pointer know takes a const LOGFONT pointer, because the public
API is C, so no const refs.

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp#newcode191
core/fxge/win32/fx_win32_print.cpp:191: // Note that |pFont| has the
actual font to render with embedded within, but

On 2016/06/30 13:31:56, dsinclair wrote:
> This sounds like more of a TODO?

Done.

https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win32_print.cpp#newcode218
core/fxge/win32/fx_win32_print.cpp:218: // obviously do not work for
arbitrary PDFs.

On 2016/06/30 13:31:57, dsinclair wrote:
> nit: s/do/does/

Done.

https://codereview.chromium.org/2113563003/

[dsin...@chromium.org]

lgtm



https://codereview.chromium.org/2113563003/

[the...@chromium.org]

Reminder to self from bungeman: Test and see how this handles web fonts.

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

On 2016/07/07 19:01:49, Lei Zhang wrote:
> Reminder to self from bungeman: Test and see how this handles web fonts.

... and fonts.google.com renders badly. I guess I need to either get
AddFontMemResourceEx() working, or figure out how to detect web fonts and skip
them.

https://codereview.chromium.org/2113563003/

[bung...@google.com]

On 2016/07/08 00:21:50, Lei Zhang wrote:
> On 2016/07/07 19:01:49, Lei Zhang wrote:
> > Reminder to self from bungeman: Test and see how this handles web fonts.
>

> ... and http://fonts.google.com renders badly. I guess I need to either get

> AddFontMemResourceEx() working, or figure out how to detect web fonts and skip
> them.

I don't think AddFontMemResourceEx works anymore in the sandbox. I think
Chromium is now using DisableNonSystemFonts (see
https://msdn.microsoft.com/en-us/library/windows/desktop/mt706244(v=vs.85).aspx
).

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

Thanks. That's good to know. On to plan B then. I also remembered I'm not
handling vertical text, so I need to do that too.

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

bungeman: PTAL

I fixed transforms. Previously, I didn't realize xform.eM21 needed to be
negative GetC(), so GDI kept failing. I tested with some arbitrary CSS text
rotations.

For WebFonts, I'm checking the selecting font's name against the requested font
name. That's the best heuristic I came up with.

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

Given all the recent GDI fuss, +kulshin@ for this PDFium CL. See my initial
comment for context.

https://codereview.chromium.org/2113563003/

[kul...@chromium.org]

We're generally trying to move away from GDI. Admittedly I don't
know nearly as much about pdfium as I'd like, but unless this is
a super short term thing I'd prefer to not see more GDI
dependencies. Can you provide a bit more background on exactly
what it is that GDI does that allows text to render as text, and
do we have any options for doing something similar with DWrite?

+jschuh@, forashaw@ who might have additional comments.

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

I've admitted we want to move away from GDI, but I don't have a better short
term solution. BTW, this code will end up running in an utility process in
Chromium.

Skia team can give better estimates on how long it will take to move PDFium to
Skia, and have the XPS backend ready for production. An alternative path that
still involves Skia is to output directly ot XPS in some way and not bother with
PDF in the middle.

See bug 409472, and other bugs that mention that bug for the user complaints.
Currently, without GDI, text gets rendered as Bezier curves, or some other form
of paths. This causes print jobs to bloat up in size, especially for large
documents. Instead of referring to embedded fonts, the output document has
duplicate paths for every bit of text. Printing to a PDF virtual printer also
results in text that cannot be highlighted. There's also some specialty printers
gets confused because they are expecting text.


https://codereview.chromium.org/2113563003/

[kul...@chromium.org]

Ok, sounds like this is the best approach at this point. I don't have
any other objections, but I feel like I don't know enough about pdfium
to l-g-t-m. If you are comfortable with other approvals, feel free to
proceed without mine.

https://codereview.chromium.org/2113563003/

[kul...@chromium.org]

https://codereview.chromium.org/2113563003/diff/140001/core/fxge/win32/fx_win32_print.cpp
File core/fxge/win32/fx_win32_print.cpp (right):

https://codereview.chromium.org/2113563003/diff/140001/core/fxge/win32/fx_win32_print.cpp#newcode217
core/fxge/win32/fx_win32_print.cpp:217: // assume the printing is
happening on the machine that generated the PDF, so
> In the meanwhile, assume the printing is happening on the machine that
generated the PDF

This doesn't sound like a safe assumption, especially if the generation
is happening on a machine that has an extended set of fonts, and the
resulting PDF is then widely distributed. Could that happen? What would
end up happening in that scenario?

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

https://codereview.chromium.org/2113563003/diff/140001/core/fxge/win32/fx_win32_print.cpp
File core/fxge/win32/fx_win32_print.cpp (right):

https://codereview.chromium.org/2113563003/diff/140001/core/fxge/win32/fx_win32_print.cpp#newcode217
core/fxge/win32/fx_win32_print.cpp:217: // assume the printing is
happening on the machine that generated the PDF, so

On 2016/07/12 23:40:56, Ilya Kulshin wrote:
> > In the meanwhile, assume the printing is happening on the machine
that
> generated the PDF
>
> This doesn't sound like a safe assumption, especially if the
generation is
> happening on a machine that has an extended set of fonts, and the
resulting PDF
> is then widely distributed. Could that happen? What would end up
happening in
> that scenario?

This code path is gated on |g_pdfium_print_text_with_gdi|. The Chromium
CL - https://codereview.chromium.org/2114583002 - only flips it on when
the source page is HTML, which means the generated PDF that we are
working with here is from Skia's PDF backend. The resulting PDF only
exists in memory and is only used to feed the printer.

If the printer is some kind of cloud printer, then the PDF still has the
fonts embedded and the receiving end can handle that. If the receiving
end is a Chrome Cloud Print Connector, then it still has
|g_pdfium_print_text_with_gdi| set to off, so this assumption still
holds.

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

bungeman: Can you take another look?

https://codereview.chromium.org/2113563003/

[bung...@google.com]

On 2016/07/15 00:06:14, Lei Zhang wrote:
> bungeman: Can you take another look?

Still have the same question, what does this do with web fonts?

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

On 2016/07/15 14:30:02, bungeman-skia wrote:
> On 2016/07/15 00:06:14, Lei Zhang wrote:
> > bungeman: Can you take another look?
>
> Still have the same question, what does this do with web fonts?

Ignores them and returns false to draw them as glyphs. See patch set 6 -> 7.

https://codereview.chromium.org/2113563003/

[bung...@google.com]

On 2016/07/15 18:05:07, Lei Zhang wrote:
> On 2016/07/15 14:30:02, bungeman-skia wrote:
> > On 2016/07/15 00:06:14, Lei Zhang wrote:
> > > bungeman: Can you take another look?
> >
> > Still have the same question, what does this do with web fonts?
>
> Ignores them and returns false to draw them as glyphs. See patch set 6 -> 7.

Ok it's ugly, but I don't see a better way to do this at the moment and this is
better than what was happening before. I can't really object to anything here.

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

Yes, I know. I'll land this Monday unless someone objects.

https://codereview.chromium.org/2113563003/

[bung...@google.com]

As a note (and you may or may not want to make a comment about this) this
depends on the font sub-setter used by the pdf generator not changing glyph ids.
Currently it does not, but that might change in the future (at which point this
will start emitting gibberish).

https://codereview.chromium.org/2113563003/

[the...@chromium.org]

On 2016/07/15 20:44:56, bungeman-skia wrote:
> As a note (and you may or may not want to make a comment about this) this
> depends on the font sub-setter used by the pdf generator not changing glyph
ids.
> Currently it does not, but that might change in the future (at which point
this
> will start emitting gibberish).

Noted. Are you specifically talking about changes to sfntly or how Skia uses
sfntly? Is there a bug for such changes that I can star and follow?

https://codereview.chromium.org/2113563003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2113563003/160001


https://codereview.chromium.org/2113563003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:160001) as
https://pdfium.googlesource.com/pdfium/+/fdb35ffb8d4bd283dd8f5905936f5c400fea3394

https://codereview.chromium.org/2113563003/