Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(252)

Issue 11363008: Fix for 128506: Random Chinese/Japanese characters are missing in documents printed via the syst... (Closed)

Created:
8 years, 1 month ago by edisonn
Modified:
8 years, 1 month ago
CC:
reed1, chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Visibility:
Public.

Description

This is a fix for http://code.google.com/p/chromium/issues/detail?id=128506 - Random Chinese/Japanese characters are missing in documents printed via the system print dialog on Windows XP SP3 The cause of the bug is that ensureFontLoaded just does not work for the printing thread because GetTextMetrics(font) is not guaranteed to load the TrueType font for an HDC build from CreateEnhMetaFile. The only way I found to force font loading is to create a dummy HDC with CreateEnhMetaFile and then print the offending character(s). This change contains: - wirings for foo_CacheFontCharacters similar with foo_CacheFont, but with dispatch this message in RenderMessageFilter and defined in view_messages.h - SkFontHost::EnsureTypefaceCharactersAccessible similar with SkFontHost::EnsureTypefaceAccessible - Small refactoring of ExtTextOut call which would - Call ExtTextOut - If failed, calls SkFontHost::EnsureTypefaceCharactersAccessible - call ExtTextOutAgain and return success/failure - the calller will default to a skia paintPath (lower quality, but correct) if above fails Notice: No tests for now, lets's make sure the design is right, then I will add tests too. Contributed by edisonn@google.com Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168943

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Total comments: 3

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Total comments: 7

Patch Set 12 : #

Total comments: 4

Patch Set 13 : #

Total comments: 2

Patch Set 14 : #

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 : #

Patch Set 18 : #

Total comments: 1

Patch Set 19 : #

Patch Set 20 : #

Patch Set 21 : #

Patch Set 22 : #

Patch Set 23 : #

Patch Set 24 : #

Patch Set 25 : #

Total comments: 6

Patch Set 26 : #

Patch Set 27 : #

Patch Set 28 : #

Patch Set 29 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+146 lines, -8 lines) Patch
M content/browser/renderer_host/render_message_filter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +34 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +10 lines, -0 lines 0 comments Download
M content/renderer/render_thread_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +5 lines, -0 lines 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +6 lines, -0 lines 0 comments Download
M content/renderer/renderer_main_platform_delegate_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +16 lines, -0 lines 0 comments Download
M skia/ext/vector_platform_device_emf_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +6 lines, -0 lines 0 comments Download
M skia/ext/vector_platform_device_emf_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 5 chunks +64 lines, -8 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
edisonn
Can you have a first stab at this cl? Tests are not ready, as I ...
8 years, 1 month ago (2012-10-31 14:22:41 UTC) #1
Scott Byer
This looks pretty good to me. On 2012/10/31 14:22:41, edisonn wrote: > Can you have ...
8 years, 1 month ago (2012-10-31 16:28:17 UTC) #2
edisonn
Steve, can you review skia/... changes (listed in OWNERS)? John, can you review content/... changes ...
8 years, 1 month ago (2012-11-01 19:59:54 UTC) #3
vandebo (ex-Chrome)
skia/ LGTM with the following nits and suggestions https://chromiumcodereview.appspot.com/11363008/diff/7010/skia/ext/vector_platform_device_emf_win.cc File skia/ext/vector_platform_device_emf_win.cc (right): https://chromiumcodereview.appspot.com/11363008/diff/7010/skia/ext/vector_platform_device_emf_win.cc#newcode7 skia/ext/vector_platform_device_emf_win.cc:7: #include ...
8 years, 1 month ago (2012-11-01 20:24:23 UTC) #4
vandebo (ex-Chrome)
Also, in your issue description please add at the bottom BUG=128506 and remove the link ...
8 years, 1 month ago (2012-11-01 20:27:02 UTC) #5
jam
There's o need to change content api (content\public) for this. that's only when src\chrome needs ...
8 years, 1 month ago (2012-11-02 00:41:20 UTC) #6
jam
There's o need to change content api (content\public) for this. that's only when src\chrome needs ...
8 years, 1 month ago (2012-11-02 00:41:21 UTC) #7
edisonn
On 2012/11/02 00:41:21, John Abd-El-Malek wrote: > There's o need to change content api (content\public) ...
8 years, 1 month ago (2012-11-08 21:12:11 UTC) #8
jam
On 2012/11/08 21:12:11, edisonn wrote: > On 2012/11/02 00:41:21, John Abd-El-Malek wrote: > > There's ...
8 years, 1 month ago (2012-11-09 01:44:13 UTC) #9
edisonn
I put the cache font characters code directly in RenderMessageFilter, as putting it in FontCache ...
8 years, 1 month ago (2012-11-09 17:48:57 UTC) #10
jam
lgtm with nits https://codereview.chromium.org/11363008/diff/8013/content/common/view_messages.h File content/common/view_messages.h (right): https://codereview.chromium.org/11363008/diff/8013/content/common/view_messages.h#newcode2488 content/common/view_messages.h:2488: LOGFONT, /* font data */ nit: ...
8 years, 1 month ago (2012-11-09 23:20:41 UTC) #11
edisonn
What kind of tests recommend to write? On Nov 9, 2012 6:20 PM, <jam@chromium.org> wrote: ...
8 years, 1 month ago (2012-11-10 00:50:19 UTC) #12
edisonn
Existing PreCacheFont related functions have no tests. e.g. mock_render_thread.cc If anyone can suggest a form ...
8 years, 1 month ago (2012-11-12 13:26:40 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edisonn@google.com/11363008/9031
8 years, 1 month ago (2012-11-14 21:14:35 UTC) #14
commit-bot: I haz the power
Failed to apply patch for content/browser/renderer_host/render_message_filter.cc: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file ...
8 years, 1 month ago (2012-11-14 21:14:45 UTC) #15
Lei Zhang
Dharani asked me to help land this CL, but it needs to be rebased. https://chromiumcodereview.appspot.com/11363008/diff/9031/skia/ext/vector_platform_device_emf_win.cc ...
8 years, 1 month ago (2012-11-14 21:16:58 UTC) #16
edisonn
Done. Can you upload a try now? Thanks. https://chromiumcodereview.appspot.com/11363008/diff/9031/skia/ext/vector_platform_device_emf_win.cc File skia/ext/vector_platform_device_emf_win.cc (right): https://chromiumcodereview.appspot.com/11363008/diff/9031/skia/ext/vector_platform_device_emf_win.cc#newcode7 skia/ext/vector_platform_device_emf_win.cc:7: #include ...
8 years, 1 month ago (2012-11-14 23:37:35 UTC) #17
Lei Zhang
Can you fix the trybot failure? Looks like you forgot to define EnsureTypefaceCharactersAccessible() in skia_sandbox_support_win.h ...
8 years, 1 month ago (2012-11-15 02:07:35 UTC) #18
edisonn
On 2012/11/15 02:07:35, Lei Zhang wrote: > Can you fix the trybot failure? Looks like ...
8 years, 1 month ago (2012-11-15 14:28:01 UTC) #19
edisonn
On 2012/11/15 14:28:01, edisonn wrote: > On 2012/11/15 02:07:35, Lei Zhang wrote: > > Can ...
8 years, 1 month ago (2012-11-15 22:34:45 UTC) #20
Lei Zhang
https://chromiumcodereview.appspot.com/11363008/diff/27002/skia/ext/vector_platform_device_emf_win.cc File skia/ext/vector_platform_device_emf_win.cc (right): https://chromiumcodereview.appspot.com/11363008/diff/27002/skia/ext/vector_platform_device_emf_win.cc#newcode12 skia/ext/vector_platform_device_emf_win.cc:12: #include "content/renderer/render_thread_impl.h" jam: Is skia allowed to #include content ...
8 years, 1 month ago (2012-11-16 00:14:03 UTC) #21
edisonn
see comment #7: https://chromiumcodereview.appspot.com/11363008/#msg7 On 2012/11/16 00:14:03, Lei Zhang wrote: > https://chromiumcodereview.appspot.com/11363008/diff/27002/skia/ext/vector_platform_device_emf_win.cc > File skia/ext/vector_platform_device_emf_win.cc ...
8 years, 1 month ago (2012-11-16 12:05:47 UTC) #22
Lei Zhang
https://chromiumcodereview.appspot.com/11363008/diff/26008/skia/ext/vector_platform_device_emf_win.h File skia/ext/vector_platform_device_emf_win.h (right): https://chromiumcodereview.appspot.com/11363008/diff/26008/skia/ext/vector_platform_device_emf_win.h#newcode133 skia/ext/vector_platform_device_emf_win.h:133: void SetSkiaEnsureTypefaceCharactersAccessible( This needs to be exposed with SK_API. ...
8 years, 1 month ago (2012-11-17 02:33:27 UTC) #23
edisonn
https://chromiumcodereview.appspot.com/11363008/diff/26008/skia/ext/vector_platform_device_emf_win.h File skia/ext/vector_platform_device_emf_win.h (right): https://chromiumcodereview.appspot.com/11363008/diff/26008/skia/ext/vector_platform_device_emf_win.h#newcode133 skia/ext/vector_platform_device_emf_win.h:133: void SetSkiaEnsureTypefaceCharactersAccessible( On 2012/11/17 02:33:28, Lei Zhang wrote: > ...
8 years, 1 month ago (2012-11-17 22:16:40 UTC) #24
edisonn
8 years, 1 month ago (2012-11-17 22:17:06 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edisonn@google.com/11363008/20022
8 years, 1 month ago (2012-11-19 00:15:21 UTC) #26
commit-bot: I haz the power
Presubmit check for 11363008-20022 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 1 month ago (2012-11-19 00:15:25 UTC) #27
Lei Zhang
+jschuh to review the content/common/view_messages.h change. For the wstring warning - does it apply since ...
8 years, 1 month ago (2012-11-19 01:23:42 UTC) #28
jschuh
On 2012/11/19 01:23:42, Lei Zhang wrote: > +jschuh to review the content/common/view_messages.h change. LGTM, but ...
8 years, 1 month ago (2012-11-19 23:26:44 UTC) #29
edisonn
Should this cl wait your fix to be in or we can proceed with check ...
8 years, 1 month ago (2012-11-20 00:39:48 UTC) #30
jschuh
On 2012/11/20 00:39:48, edisonn wrote: > Should this cl wait your fix to be in ...
8 years, 1 month ago (2012-11-20 03:34:06 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edisonn@google.com/11363008/20022
8 years, 1 month ago (2012-11-20 21:22:22 UTC) #32
commit-bot: I haz the power
Failed to apply patch for content/common/view_messages.h: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file ...
8 years, 1 month ago (2012-11-20 21:22:25 UTC) #33
edisonn
Some new chunk of code was added at the same line. I fixed the conflict ...
8 years, 1 month ago (2012-11-20 22:19:46 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edisonn@google.com/11363008/25015
8 years, 1 month ago (2012-11-20 22:28:47 UTC) #35
commit-bot: I haz the power
Presubmit check for 11363008-25015 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 1 month ago (2012-11-20 22:28:56 UTC) #36
edisonn
updated code to use string16 On 2012/11/20 22:28:56, I haz the power (commit-bot) wrote: > ...
8 years, 1 month ago (2012-11-20 23:19:16 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edisonn@google.com/11363008/25017
8 years, 1 month ago (2012-11-20 23:21:37 UTC) #38
commit-bot: I haz the power
8 years, 1 month ago (2012-11-21 01:15:01 UTC) #39
Change committed as 168943

Powered by Google App Engine
This is Rietveld 408576698