Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197553002:
Fix CGdiPrinterDriver::DrawDeviceText() to draw multiple characters. (Closed)
DescriptionFix CGdiPrinterDriver::DrawDeviceText() to draw multiple characters.
Previously, Skia generated PDFs would only output one character at a
time, but now they output whole strings. Handle this case.
BUG=409472
Committed: https://pdfium.googlesource.com/pdfium/+/2ba53c176df1bc5bb963901e7cc42c6a60af24eb
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase #Patch Set 3 : Fix kerning #Messages
Total messages: 27 (12 generated)
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...
thestig@chromium.org changed reviewers: + bungeman@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bungeman@google.com changed reviewers: + caryclark@google.com, halcanary@google.com
I think Hal or Cary would probably be better to take a look at this.
https://codereview.chromium.org/2197553002/diff/1/core/fxge/win32/fx_win32_pr... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2197553002/diff/1/core/fxge/win32/fx_win32_pr... core/fxge/win32/fx_win32_print.cpp:295: ASSERT(charpos.m_OriginY == 0); why is this true?
https://codereview.chromium.org/2197553002/diff/1/core/fxge/win32/fx_win32_pr... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2197553002/diff/1/core/fxge/win32/fx_win32_pr... core/fxge/win32/fx_win32_print.cpp:295: ASSERT(charpos.m_OriginY == 0); On 2016/07/29 15:26:21, Hal Canary wrote: > why is this true? It's held up so far with my test cases. Do you have an example that breaks this assertion? If so, I can work with that and fix this more.
vertical text maybe?
On 2016/07/29 16:42:46, Hal Canary wrote: > vertical text maybe? Oh, I have been testing with "vertical text" [1] which is really rotated text. The ModifyWorldTransform() handles that just fine. Then I went looking and found some real vertical text, [2] so let me do some more testing... [1] https://davidwalsh.name/demo/css-vertical-text.php [2] https://zh.wikipedia.org/wiki/%E6%9D%B1%E4%BA%9E%E6%96%87%E5%AD%97%E6%8E%92%E...
On 2016/07/29 19:26:31, Lei Zhang wrote: > On 2016/07/29 16:42:46, Hal Canary wrote: > > vertical text maybe? > > Oh, I have been testing with "vertical text" [1] which is really rotated text. > The ModifyWorldTransform() handles that just fine. Then I went looking and found > some real vertical text, [2] so let me do some more testing... > > [1] https://davidwalsh.name/demo/css-vertical-text.php > [2] > https://zh.wikipedia.org/wiki/%E6%9D%B1%E4%BA%9E%E6%96%87%E5%AD%97%E6%8E%92%E... I don't test vertical test well enough inside SkPDF, so send any good small test cases my way.
On 2016/07/29 19:28:17, Hal Canary wrote: > I don't test vertical test well enough inside SkPDF, so send any good small test > cases my way. The table just below the anchor I linked to in [2] is a good example. The 4 columns are different ways to do vertical text in CSS. Chromium does the first 3 correctly, but not the last column. The rows are Traditional Chinese, Simplified Chinese, and Japanese. You don't have to be able to read it, just make sure it renders correctly.
Printing page 12 of [2] (which has the table of vertical text) via print preview worked on Canary, but when I tried it from my ~2 day old debug build, I hit the assert in SkUTF16_FromUnichar(). Does that sound familiar? I'll try commenting it out to see what happens if I blow past it. https://codereview.chromium.org/2197553002/diff/1/core/fxge/win32/fx_win32_pr... File core/fxge/win32/fx_win32_print.cpp (right): https://codereview.chromium.org/2197553002/diff/1/core/fxge/win32/fx_win32_pr... core/fxge/win32/fx_win32_print.cpp:300: spacing[i] = FXSYS_round(fPixelSpacing); I printed a random page from Wikipedia and there are places where the rounding causes bad kerning. I guess I'll have to try MM_TWIPS or something other map mode to get more accurate spacing.
I'm away on vacation. I'll be back August 8th.
On 2016/07/29 19:28:17, Hal Canary wrote: > On 2016/07/29 19:26:31, Lei Zhang wrote: > > On 2016/07/29 16:42:46, Hal Canary wrote: > > > vertical text maybe? > > > I don't test vertical test well enough inside SkPDF, so send any good small test > cases my way. Easy small test case: <div style="writing-mode:tb-r1">TEST FOO BAR ABC</div> Works fine with this CL, surprisingly. The vertical text still has 0 for |m_OriginY|.
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.
This is ready for another look.
lgtm
The CQ bit was checked by thestig@chromium.org
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 ========== Fix CGdiPrinterDriver::DrawDeviceText() to draw multiple characters. Previously, Skia generated PDFs would only output one character at a time, but now they output whole strings. Handle this case. BUG=409472 ========== to ========== Fix CGdiPrinterDriver::DrawDeviceText() to draw multiple characters. Previously, Skia generated PDFs would only output one character at a time, but now they output whole strings. Handle this case. BUG=409472 Committed: https://pdfium.googlesource.com/pdfium/+/2ba53c176df1bc5bb963901e7cc42c6a60af... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/2ba53c176df1bc5bb963901e7cc42c6a60af... |
