Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113563003:
Implement CGdiPrinterDriver::DrawDeviceText(). (Closed)
DescriptionImplement 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
Committed: https://pdfium.googlesource.com/pdfium/+/fdb35ffb8d4bd283dd8f5905936f5c400fea3394
Patch Set 1 #Patch Set 2 : override #Patch Set 3 : comments #
Total comments: 7
Patch Set 4 : Experimental public APIs #Patch Set 5 : rebase #Patch Set 6 : Generalize transforms #Patch Set 7 : Detect font mismatches, clean up code #Patch Set 8 : Self review #
Total comments: 2
Patch Set 9 : rebase #
Messages
Total messages: 36 (12 generated)
thestig@chromium.org changed reviewers: + bungeman@google.com, dsinclair@google.com
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_win3... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win3... 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 was changed from ========== 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 ========== to ========== 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 ==========
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org - dsinclair@google.com
https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win3... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win3... 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_win3... 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_win3... core/fxge/win32/fx_win32_print.cpp:218: // obviously do not work for arbitrary PDFs. nit: s/do/does/
https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win3... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2113563003/diff/40001/core/fxge/win32/fx_win3... 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_win3... 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_win3... 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.
lgtm
Reminder to self from bungeman: Test and see how this handles web fonts.
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.
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 ).
On 2016/07/08 19:08:35, bungeman-skia wrote: > 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 > ). 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.
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.
Given all the recent GDI fuss, +kulshin@ for this PDFium CL. See my initial comment for context.
kulshin@chromium.org changed reviewers: + kulshin@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.
jschuh@chromium.org changed reviewers: + forshaw@chromium.org
On 2016/07/12 23:05:11, Ilya Kulshin wrote: > 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. 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.
On 2016/07/12 23:18:07, Lei Zhang wrote: > On 2016/07/12 23:05:11, Ilya Kulshin wrote: > > 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. > > 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. 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/diff/140001/core/fxge/win32/fx_win... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2113563003/diff/140001/core/fxge/win32/fx_win... 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/diff/140001/core/fxge/win32/fx_win... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2113563003/diff/140001/core/fxge/win32/fx_win... 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.
bungeman: Can you take another look?
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?
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.
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.
On 2016/07/15 20:03:20, bungeman-skia wrote: > 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. Yes, I know. I'll land this Monday unless someone objects.
On 2016/07/15 20:42:06, Lei Zhang wrote: > On 2016/07/15 20:03:20, bungeman-skia wrote: > > 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. > > Yes, I know. I'll land this Monday unless someone objects. 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).
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?
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org Link to the patchset: https://codereview.chromium.org/2113563003/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://pdfium.googlesource.com/pdfium/+/fdb35ffb8d4bd283dd8f5905936f5c400fea... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://pdfium.googlesource.com/pdfium/+/fdb35ffb8d4bd283dd8f5905936f5c400fea... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
