Create font with DEFAULT_CHARSET instead of ANSI_CHARSET.

Create font with DEFAULT_CHARSET instead of ANSI_CHARSET.
Users with non ANSI locale likely need non ANSI charset.
BUG=152893

[Vitaly Buka (NO REVIEWS), 2012-10-08 23:18:40]

https://codereview.chromium.org/11087016/diff/3001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11087016/diff/3001/ui/gfx/render_text_win.cc#...
ui/gfx/render_text_win.cc:648: switch(hr) {
This error happens on JA XP for some fonts.

[Vitaly Buka (NO REVIEWS), 2012-10-08 23:20:02]

Hi Alexei, seems you are the author of both pieces of code. Maybe you know
better solution?

[Vitaly Buka (NO REVIEWS), 2012-10-08 23:32:51]

https://codereview.chromium.org/11087016/diff/7001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11087016/diff/7001/ui/gfx/render_text_win.cc#...
ui/gfx/render_text_win.cc:649: if (hr == USP_E_SCRIPT_NOT_IN_FONT ||
I don't know why, but this error happens on JA XP for some fonts.

[Vitaly Buka (NO REVIEWS), 2012-10-09 06:09:42]

https://codereview.chromium.org/11087016/diff/9014/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/11087016/diff/9014/ui/gfx/platform_font_win.c...
ui/gfx/platform_font_win.cc:201: int font_size) {
This also fixes hang and makes visible work.
I still think we should fix DeriveFontWithHeight to avoid infinite loop.

[Alexei Svitkine (slow), 2012-10-12 08:40:24]

Sorry, I'm on vacation. Please try a different reviewer, for e.g. msw

[Vitaly Buka (NO REVIEWS), 2012-10-12 17:48:51]

Hi Xiaomei and Mike, can you take a look at this CL?
Alexei on vacation for another week.

[msw, 2012-10-12 20:50:56]

Thanks for digging into these issues.

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/platform_font_win.cc...
ui/gfx/platform_font_win.cc:202: HFONT hf = ::CreateFont(-font_size, 0, 0, 0, 0,
0, 0, 0, DEFAULT_CHARSET, 0,
MSDN indicates that this picks a char set "based on the current system locale",
which sounds good, but goes on to say "To ensure consistent results when
creating a font, do not specify... DEFAULT_CHARSET" and "make sure that the
fdwCharSet value matches the character set of the typeface specified in
lpszFace". Do you think this might fail on font-name/charset conflict or
similar? How is a NULL HFONT return value ultimately handled? (it looks like
it's not a special case...) Can we add some tests for this?

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:650: hr ==
__HRESULT_FROM_WIN32(ERROR_INVALID_WINDOW_HANDLE)) {
1) Why is this in the same patch? It looks like a separate issue that should
probably be fixed by trashing the invalid HWND/DC (not picking another font). Do
you have a repro for this failure? I wonder if it's related to
http://crbug.com/141702 (eroman mentioned a corrupted DC).

2) Use the HRESULT_FROM_WIN32 function, not the __HRESULT_FROM_WIN32 macro:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms680746(v=vs.85).aspx

[Vitaly Buka (NO REVIEWS), 2012-10-12 21:11:32]

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/platform_font_win.cc...
ui/gfx/platform_font_win.cc:202: HFONT hf = ::CreateFont(-font_size, 0, 0, 0, 0,
0, 0, 0, DEFAULT_CHARSET, 0,
I guess with ANSI_CHARSET it's consistent in sense that it does not work on any
system for JA chars.
With DEFAULT_CHARSET it works at least where system locale is set.

On 2012/10/12 20:50:57, msw wrote:
> MSDN indicates that this picks a char set "based on the current system
locale",
> which sounds good, but goes on to say "To ensure consistent results when
> creating a font, do not specify... DEFAULT_CHARSET" and "make sure that the
> fdwCharSet value matches the character set of the typeface specified in
> lpszFace". Do you think this might fail on font-name/charset conflict or
> similar? How is a NULL HFONT return value ultimately handled? (it looks like
> it's not a special case...) Can we add some tests for this?

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:650: hr ==
__HRESULT_FROM_WIN32(ERROR_INVALID_WINDOW_HANDLE)) {
On 2012/10/12 20:50:57, msw wrote:
> 1) Why is this in the same patch? It looks like a separate issue that should
> probably be fixed by trashing the invalid HWND/DC (not picking another font).
Do
> you have a repro for this failure? I wonder if it's related to
> http://crbug.com/141702 (eroman mentioned a corrupted DC).
> 

Because this happens always when I reproduce bug attached to issue.


> 2) Use the HRESULT_FROM_WIN32 function, not the __HRESULT_FROM_WIN32 macro:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms680746%28v=vs.85%29...

Done.

[Vitaly Buka (NO REVIEWS), 2012-10-12 21:14:19]

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/11087016/diff/9014/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:650: hr ==
__HRESULT_FROM_WIN32(ERROR_INVALID_WINDOW_HANDLE)) {
>> http://crbug.com/141702 
I never see crash like that

[msw, 2012-10-12 21:34:55]

Thanks for splitting into codereview.chromium.org/11143002
The other parts of this CL are likely still quite valuable.
I'm just trying to make sure we do the right thing.

http://codereview.chromium.org/11087016/diff/15001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

http://codereview.chromium.org/11087016/diff/15001/ui/gfx/platform_font_win.c...
ui/gfx/platform_font_win.cc:202: HFONT hf = ::CreateFont(-font_size, 0, 0, 0, 0,
0, 0, 0, DEFAULT_CHARSET, 0,
1) Will this fail on font-name/charset conflict or similar?
2) Are NULL HFONT return values handled okay?
3) Can we add some tests for this?

http://codereview.chromium.org/11087016/diff/15001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/11087016/diff/15001/ui/gfx/render_text_win.cc#...
ui/gfx/render_text_win.cc:650: hr ==
HRESULT_FROM_WIN32(ERROR_INVALID_WINDOW_HANDLE)) {
It seems like this should trash the invalid |cached_hdc_|.
Why ignore the failure and just try more fonts, etc.?

[Vitaly Buka (NO REVIEWS), 2012-10-12 22:54:02]

http://codereview.chromium.org/11087016/diff/15001/ui/gfx/platform_font_win.c...
> ui/gfx/platform_font_win.cc:202: HFONT hf = ::CreateFont(-font_size, 0, 0, 0,
0,
> 0, 0, 0, DEFAULT_CHARSET, 0,
> 1) Will this fail on font-name/charset conflict or similar?
If it would fail, then it fails now, but just for font-name/charset variants.
I believe the change provides users with more useful result. Actually now it's
fixing printing of headers for any non Latin charset.

> 2) Are NULL HFONT return values handled okay?

I tried different combinations, even nonexistent font names, and it always
returns some font. But just in case we can fallback to GetBaseFontRef

> 3) Can we add some tests for this?

Not really sure what is useful test here.
We are just changing behavior
"if we don't know what user needs just create font good for ANSI chars" to 
"if we don't know what user needs just create font good for DEFAULT chars"

[msw, 2012-10-12 23:37:53]

Note: I'm not an expert here, just trying to cover bases, get tests.

(1) The system locale may not reflect the charset of text content.
Also, ex/ "Arial" may differ for ANSI/DEFAULT on some locales.
If so, this could regress prominent fonts for those locales.
Please test a few prominent charsets with ANSI/i18n text.

(2) sgtm

(3) Test the return value['s LOG_FONT] for the purported fix of (1):
"it's fixing printing of headers for any non Latin charset."
It may be tough to test on all bot configs, but it's worth trying.

[Vitaly Buka (NO REVIEWS), 2012-10-23 16:31:11]

Alexei, can you take a look?
Is this in general acceptable approach to fix the issue?

[Alexei Svitkine (slow), 2012-10-23 16:40:04]

Hi Vitaly,

I may have missed some of the conversation behind this, so I have some questions
(specifically about this remaining change and not the RenderText changes.)

Ultimately, this change causes a potential difference in the underlying hfont
that's created. What is actually different between them?

Can you explain under what circumstances it would be different and why this is
needed? (This would probably have been clearer for me if you included more info
the cr description or in a TEST= line.)

Thanks.

[Vitaly Buka (NO REVIEWS), 2012-10-23 17:05:27]

On 2012/10/23 16:40:04, Alexei Svitkine wrote:
> Hi Vitaly,
> 
> I may have missed some of the conversation behind this, so I have some
questions
> (specifically about this remaining change and not the RenderText changes.)

Problem is that, if we create font by name and expect to use that to print Asian
or Russian text ANSI_CHARSET returns font without required glyphs. You can open
site with non ANSI title and open print preview. Check header.

Problem with DEFAULT_CHARSET is that we still can't print anything on any
machine. E.g. if default is Russian, we can't print Chinese.

So I suspect maybe better solution is to change fallback fonts. 

> 
> Ultimately, this change causes a potential difference in the underlying hfont
> that's created. What is actually different between them?
> 
> Can you explain under what circumstances it would be different and why this is

I afraid I can't, I don't understand enough this stuff.

> needed? (This would probably have been clearer for me if you included more
info
> the cr description or in a TEST= line.)

Let's finish CL itself after we agreed on approach in general.

> 
> Thanks.

[Alexei Svitkine (slow), 2012-10-23 17:14:36]

On Tue, Oct 23, 2012 at 1:05 PM, <vitalybuka@chromium.org> wrote:

> On 2012/10/23 16:40:04, Alexei Svitkine wrote:
>
>> Hi Vitaly,
>>
>
>  I may have missed some of the conversation behind this, so I have some
>>
> questions
>
>> (specifically about this remaining change and not the RenderText changes.)
>>
>
> Problem is that, if we create font by name and expect to use that to print
> Asian
> or Russian text ANSI_CHARSET returns font without required glyphs. You can
> open
> site with non ANSI title and open print preview. Check header.
>
> Problem with DEFAULT_CHARSET is that we still can't print anything on any
> machine. E.g. if default is Russian, we can't print Chinese.
>
> So I suspect maybe better solution is to change fallback fonts.


Right, the solution would be to find the correct fallback font - for this,
there's already logic in render_text_win.cc and font_fallback_win.cc.

For CJK text, I'm fairly confident that the issue is that the font linking
code from that touches the registry can't run in the sandbox. So I think
the solution there would be changes to the architecture - i.e. some IPC to
get the linked fonts from the registry from the browser process and then
set those on the RenderText instance (via a FontList, the support for which
would also need to be added to RenderTextWin).

For Russian, I would expect things to work already, since that would be
covered by the "uniscribe fallback" path in RenderTextWin, which works even
in the sandbox. But perhaps that is not the case?

So let me ask, which specific problems are you looking to fix here? Is it
CJK text resulting in boxes shown or something else? What's the repro?


>
>
>
>  Ultimately, this change causes a potential difference in the underlying
>> hfont
>> that's created. What is actually different between them?
>>
>
>  Can you explain under what circumstances it would be different and why
>> this is
>>
>
> I afraid I can't, I don't understand enough this stuff.
>
>
>  needed? (This would probably have been clearer for me if you included more
>>
> info
>
>> the cr description or in a TEST= line.)
>>
>
> Let's finish CL itself after we agreed on approach in general.
>
>
>  Thanks.
>>
>
>
>
https://codereview.chromium.**org/11087016/<https://codereview.chromium.org/1...
>

[Vitaly Buka (NO REVIEWS), 2012-10-23 17:33:41]

Chrome + windows 7 or Chrome + Win XP Ja
1. Open http://www.nikkei.com/
2. Ctrl+P
3. Observe print preview and make sure that Headers and footers are enabled.

Result:
  Squares in header.

For Russian: Chrome + Win XP Ja.
1. Open http://ya.ru/
2. Ctrl+P
3. Observe print preview and make sure that Headers and footers are enabled.

Result:
  Some nonsense symbols in header with very small font size.

On Tue, Oct 23, 2012 at 10:13 AM, Alexei Svitkine
<asvitkine@chromium.org>wrote:

>
>
> On Tue, Oct 23, 2012 at 1:05 PM, <vitalybuka@chromium.org> wrote:
>
>> On 2012/10/23 16:40:04, Alexei Svitkine wrote:
>>
>>> Hi Vitaly,
>>>
>>
>>  I may have missed some of the conversation behind this, so I have some
>>>
>> questions
>>
>>> (specifically about this remaining change and not the RenderText
>>> changes.)
>>>
>>
>> Problem is that, if we create font by name and expect to use that to
>> print Asian
>> or Russian text ANSI_CHARSET returns font without required glyphs. You
>> can open
>> site with non ANSI title and open print preview. Check header.
>>
>> Problem with DEFAULT_CHARSET is that we still can't print anything on any
>> machine. E.g. if default is Russian, we can't print Chinese.
>>
>> So I suspect maybe better solution is to change fallback fonts.
>
>
> Right, the solution would be to find the correct fallback font - for this,
> there's already logic in render_text_win.cc and font_fallback_win.cc.
>
> For CJK text, I'm fairly confident that the issue is that the font linking
> code from that touches the registry can't run in the sandbox. So I think
> the solution there would be changes to the architecture - i.e. some IPC to
> get the linked fonts from the registry from the browser process and then
> set those on the RenderText instance (via a FontList, the support for which
> would also need to be added to RenderTextWin).
>
> For Russian, I would expect things to work already, since that would be
> covered by the "uniscribe fallback" path in RenderTextWin, which works even
> in the sandbox. But perhaps that is not the case?
>
> So let me ask, which specific problems are you looking to fix here? Is it
> CJK text resulting in boxes shown or something else? What's the repro?
>
>
>>
>>
>>
>>  Ultimately, this change causes a potential difference in the underlying
>>> hfont
>>> that's created. What is actually different between them?
>>>
>>
>>  Can you explain under what circumstances it would be different and why
>>> this is
>>>
>>
>> I afraid I can't, I don't understand enough this stuff.
>>
>>
>>  needed? (This would probably have been clearer for me if you included
>>> more
>>>
>> info
>>
>>> the cr description or in a TEST= line.)
>>>
>>
>> Let's finish CL itself after we agreed on approach in general.
>>
>>
>>  Thanks.
>>>
>>
>>
>>
https://codereview.chromium.**org/11087016/<https://codereview.chromium.org/1...
>>
>
>

[Vitaly Buka (NO REVIEWS), 2012-10-23 17:37:22]

Regarding redesign.
Maybe it's more convenient way is to print headers/footers with webkit and
leave all font related issue to them?

On Tue, Oct 23, 2012 at 10:33 AM, Vitaly Buka <vitalybuka@chromium.org>wrote:

> Chrome + windows 7 or Chrome + Win XP Ja
> 1. Open http://www.nikkei.com/
> 2. Ctrl+P
> 3. Observe print preview and make sure that Headers and footers are
> enabled.
>
> Result:
>   Squares in header.
>
> For Russian: Chrome + Win XP Ja.
> 1. Open http://ya.ru/
> 2. Ctrl+P
> 3. Observe print preview and make sure that Headers and footers are
> enabled.
>
> Result:
>   Some nonsense symbols in header with very small font size.
>
> On Tue, Oct 23, 2012 at 10:13 AM, Alexei Svitkine
<asvitkine@chromium.org>wrote:
>
>>
>>
>> On Tue, Oct 23, 2012 at 1:05 PM, <vitalybuka@chromium.org> wrote:
>>
>>> On 2012/10/23 16:40:04, Alexei Svitkine wrote:
>>>
>>>> Hi Vitaly,
>>>>
>>>
>>>  I may have missed some of the conversation behind this, so I have some
>>>>
>>> questions
>>>
>>>> (specifically about this remaining change and not the RenderText
>>>> changes.)
>>>>
>>>
>>> Problem is that, if we create font by name and expect to use that to
>>> print Asian
>>> or Russian text ANSI_CHARSET returns font without required glyphs. You
>>> can open
>>> site with non ANSI title and open print preview. Check header.
>>>
>>> Problem with DEFAULT_CHARSET is that we still can't print anything on any
>>> machine. E.g. if default is Russian, we can't print Chinese.
>>>
>>> So I suspect maybe better solution is to change fallback fonts.
>>
>>
>> Right, the solution would be to find the correct fallback font - for
>> this, there's already logic in render_text_win.cc and font_fallback_win.cc.
>>
>> For CJK text, I'm fairly confident that the issue is that the font
>> linking code from that touches the registry can't run in the sandbox. So I
>> think the solution there would be changes to the architecture - i.e. some
>> IPC to get the linked fonts from the registry from the browser process and
>> then set those on the RenderText instance (via a FontList, the support for
>> which would also need to be added to RenderTextWin).
>>
>> For Russian, I would expect things to work already, since that would be
>> covered by the "uniscribe fallback" path in RenderTextWin, which works even
>> in the sandbox. But perhaps that is not the case?
>>
>> So let me ask, which specific problems are you looking to fix here? Is it
>> CJK text resulting in boxes shown or something else? What's the repro?
>>
>>
>>>
>>>
>>>
>>>  Ultimately, this change causes a potential difference in the underlying
>>>> hfont
>>>> that's created. What is actually different between them?
>>>>
>>>
>>>  Can you explain under what circumstances it would be different and why
>>>> this is
>>>>
>>>
>>> I afraid I can't, I don't understand enough this stuff.
>>>
>>>
>>>  needed? (This would probably have been clearer for me if you included
>>>> more
>>>>
>>> info
>>>
>>>> the cr description or in a TEST= line.)
>>>>
>>>
>>> Let's finish CL itself after we agreed on approach in general.
>>>
>>>
>>>  Thanks.
>>>>
>>>
>>>
>>>
https://codereview.chromium.**org/11087016/<https://codereview.chromium.org/1...
>>>
>>
>>
>

[Alexei Svitkine (slow), 2012-10-23 18:46:49]

(Oops, we lost the cc's a few emails ago, re-adding them.)

On Tue, Oct 23, 2012 at 2:29 PM, Vitaly Buka <vitalybuka@chromium.org>wrote:

> --*no*-*sandbox **resolved** both CJK and RU issues.*


Interesting. I understand why it would resolve CJK, but not RU - because
I'm pretty certain it's not the same as CJK.

I guess the question is what's the difference in the behavior
of ChooseFallbackFont() in render_text_win.cc between sandbox and no
sandbox. Presumably, a different font is the result between the two? Can
you debug to find that out?

-Alexei


>
>
> On Tue, Oct 23, 2012 at 10:58 AM, Alexei Svitkine
<asvitkine@chromium.org>wrote:
>
>>
>>
>> On Tue, Oct 23, 2012 at 1:33 PM, Vitaly Buka <vitalybuka@chromium.org>wrote:
>>
>>> Chrome + windows 7 or Chrome + Win XP Ja
>>> 1. Open http://www.nikkei.com/
>>> 2. Ctrl+P
>>> 3. Observe print preview and make sure that Headers and footers are
>>> enabled.
>>>
>>> Result:
>>>   Squares in header.
>>>
>>
>> So this is most certainly the CJK issue I've mentioned. I have a good
>> idea how to solve this, which I've briefly mentioned in the previous email.
>> I was considering taking this on this quarter myself, actually - although
>> it's lower priority than my other commitments at the moment.
>>
>>
>>> For Russian: Chrome + Win XP Ja.
>>> 1. Open http://ya.ru/
>>> 2. Ctrl+P
>>> 3. Observe print preview and make sure that Headers and footers are
>>> enabled.
>>>
>>> Result:
>>>   Some nonsense symbols in header with very small font size.
>>>
>>
>> This one is new to me - and I do not believe it is the same issue as the
>> previous one. We'd have to dig into what's actually happening there, e.g.
>> by adding logging to RenderTextWin code to see which fonts it considers for
>> fallback and then figure out why those aren't working right. There may be a
>> disconnect with the font name we get from the HFONT and what we pass to
>> Skia, for example. Once we've dug deeper into that and understand what's
>> actually happening there, we can evaluate what the right solution is.
>>
>> -Alexei
>>
>>
>>
>>> On Tue, Oct 23, 2012 at 10:13 AM, Alexei Svitkine <
>>> asvitkine@chromium.org> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Oct 23, 2012 at 1:05 PM, <vitalybuka@chromium.org> wrote:
>>>>
>>>>> On 2012/10/23 16:40:04, Alexei Svitkine wrote:
>>>>>
>>>>>> Hi Vitaly,
>>>>>>
>>>>>
>>>>>  I may have missed some of the conversation behind this, so I have some
>>>>>>
>>>>> questions
>>>>>
>>>>>> (specifically about this remaining change and not the RenderText
>>>>>> changes.)
>>>>>>
>>>>>
>>>>> Problem is that, if we create font by name and expect to use that to
>>>>> print Asian
>>>>> or Russian text ANSI_CHARSET returns font without required glyphs. You
>>>>> can open
>>>>> site with non ANSI title and open print preview. Check header.
>>>>>
>>>>> Problem with DEFAULT_CHARSET is that we still can't print anything on
>>>>> any
>>>>> machine. E.g. if default is Russian, we can't print Chinese.
>>>>>
>>>>> So I suspect maybe better solution is to change fallback fonts.
>>>>
>>>>
>>>> Right, the solution would be to find the correct fallback font - for
>>>> this, there's already logic in render_text_win.cc and
font_fallback_win.cc.
>>>>
>>>> For CJK text, I'm fairly confident that the issue is that the font
>>>> linking code from that touches the registry can't run in the sandbox. So I
>>>> think the solution there would be changes to the architecture - i.e. some
>>>> IPC to get the linked fonts from the registry from the browser process and
>>>> then set those on the RenderText instance (via a FontList, the support for
>>>> which would also need to be added to RenderTextWin).
>>>>
>>>> For Russian, I would expect things to work already, since that would be
>>>> covered by the "uniscribe fallback" path in RenderTextWin, which works
even
>>>> in the sandbox. But perhaps that is not the case?
>>>>
>>>> So let me ask, which specific problems are you looking to fix here? Is
>>>> it CJK text resulting in boxes shown or something else? What's the repro?
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>  Ultimately, this change causes a potential difference in the
>>>>>> underlying hfont
>>>>>> that's created. What is actually different between them?
>>>>>>
>>>>>
>>>>>  Can you explain under what circumstances it would be different and
>>>>>> why this is
>>>>>>
>>>>>
>>>>> I afraid I can't, I don't understand enough this stuff.
>>>>>
>>>>>
>>>>>  needed? (This would probably have been clearer for me if you included
>>>>>> more
>>>>>>
>>>>> info
>>>>>
>>>>>> the cr description or in a TEST= line.)
>>>>>>
>>>>>
>>>>> Let's finish CL itself after we agreed on approach in general.
>>>>>
>>>>>
>>>>>  Thanks.
>>>>>>
>>>>>
>>>>>
>>>>>
https://codereview.chromium.**org/11087016/<https://codereview.chromium.org/1...
>>>>>
>>>>
>>>>
>>>
>>
>

[Vitaly Buka (NO REVIEWS), 2012-10-23 18:52:14]

On Tue, Oct 23, 2012 at 11:46 AM, Alexei Svitkine
<asvitkine@chromium.org>wrote:

> (Oops, we lost the cc's a few emails ago, re-adding them.)
>
> On Tue, Oct 23, 2012 at 2:29 PM, Vitaly Buka <vitalybuka@chromium.org>wrote:
>
>> --*no*-*sandbox **resolved** both CJK and RU issues.*
>
>
> Interesting. I understand why it would resolve CJK, but not RU - because
> I'm pretty certain it's not the same as CJK.
>

> I guess the question is what's the difference in the behavior
> of ChooseFallbackFont() in render_text_win.cc between sandbox and no
> sandbox. Presumably, a different font is the result between the two? Can
> you debug to find that out?
>

Actually it resolved Russian part of text, but Latin chars and number still
broken.

Why it's so surprising for you. I didn't know that sandbox change
fallback behavior.
As I understand Russian also can fallback to wrong font.

[Alexei Svitkine (slow), 2012-10-23 19:01:14]

On Tue, Oct 23, 2012 at 2:51 PM, Vitaly Buka <vitalybuka@chromium.org>wrote:

>
>
> On Tue, Oct 23, 2012 at 11:46 AM, Alexei Svitkine
<asvitkine@chromium.org>wrote:
>
>> (Oops, we lost the cc's a few emails ago, re-adding them.)
>>
>> On Tue, Oct 23, 2012 at 2:29 PM, Vitaly Buka <vitalybuka@chromium.org>wrote:
>>
>>> --*no*-*sandbox **resolved** both CJK and RU issues.*
>>
>>
>> Interesting. I understand why it would resolve CJK, but not RU - because
>> I'm pretty certain it's not the same as CJK.
>>
>
>> I guess the question is what's the difference in the behavior
>> of ChooseFallbackFont() in render_text_win.cc between sandbox and no
>> sandbox. Presumably, a different font is the result between the two? Can
>> you debug to find that out?
>>
>
> Actually it resolved Russian part of text, but Latin chars and number
> still broken.
>

The implementation of RenderTextWin has logic to split the text into
different runs - and different runs will have different fonts when they
contain different languages between them. The fallback for each run is done
independently, so it's curious why Latin chars are affected here. We need
to understand what exactly is happening (how the runs are being split up
for the string and which fonts are chosen for which runs), and why this
behaviour differs sandbox vs. non-sandbox. If you want, I can do this
investigation myself - but it may take me some time to get to it, since
I've got a number of things on my plate.


> Why it's so surprising for you. I didn't know that sandbox change
> fallback behavior.
>

Well, sandbox prevents the font_fallback_win.cc code from accessing the
registry. That needs to be overcome as I mentioned earlier for CJK to work
(CJK font fallback on Windows is done via "font linking" which needs info
out of the registry.) The fallback mechanism for non-CJK fonts is different
(we basically try the font used by Uniscribe for the fallback.)


> As I understand Russian also can fallback to wrong font.
>

[Vitaly Buka (NO REVIEWS), 2012-10-23 20:38:46]

On Tue, Oct 23, 2012 at 12:00 PM, Alexei Svitkine
<asvitkine@chromium.org>wrote:

>
>
> On Tue, Oct 23, 2012 at 2:51 PM, Vitaly Buka <vitalybuka@chromium.org>wrote:
>
>>
>>
>> On Tue, Oct 23, 2012 at 11:46 AM, Alexei Svitkine <asvitkine@chromium.org
>> > wrote:
>>
>>> (Oops, we lost the cc's a few emails ago, re-adding them.)
>>>
>>> On Tue, Oct 23, 2012 at 2:29 PM, Vitaly Buka
<vitalybuka@chromium.org>wrote:
>>>
>>>> --*no*-*sandbox **resolved** both CJK and RU issues.*
>>>
>>>
>>> Interesting. I understand why it would resolve CJK, but not RU - because
>>> I'm pretty certain it's not the same as CJK.
>>>
>>
>>> I guess the question is what's the difference in the behavior
>>> of ChooseFallbackFont() in render_text_win.cc between sandbox and no
>>> sandbox. Presumably, a different font is the result between the two? Can
>>> you debug to find that out?
>>>
>>
>> Actually it resolved Russian part of text, but Latin chars and number
>> still broken.
>>
>
> The implementation of RenderTextWin has logic to split the text into
> different runs - and different runs will have different fonts when they
> contain different languages between them. The fallback for each run is done
> independently, so it's curious why Latin chars are affected here. We need
> to understand what exactly is happening (how the runs are being split up
> for the string and which fonts are chosen for which runs), and why this
> behaviour differs sandbox vs. non-sandbox. If you want, I can do this
> investigation myself - but it may take me some time to get to it, since
> I've got a number of things on my plate.
>

Same for me. But it seems worth fixing for at least early M24 beta.


>
>
>> Why it's so surprising for you. I didn't know that sandbox change
>> fallback behavior.
>>
>
> Well, sandbox prevents the font_fallback_win.cc code from accessing the
> registry. That needs to be overcome as I mentioned earlier for CJK to work
> (CJK font fallback on Windows is done via "font linking" which needs info
> out of the registry.) The fallback mechanism for non-CJK fonts is different
> (we basically try the font used by Uniscribe for the fallback.)
>

i've got it


>
>
>> As I understand Russian also can fallback to wrong font.
>>
>
>
And thanks for information.

[Vitaly Buka (NO REVIEWS), 2012-10-25 20:59:03]

Alexei, I just tried to change print_web_view_helper.cc to render header and
footer just by creating WebKit::WebView and feeding that with required text.
It works and resolve font issues. Also looks like this approach would allow to
remove a lot of code from print_web_view_helper.
I'd like to go this way. Do I miss something?

[Alexei Svitkine (slow), 2012-10-25 21:04:12]

On 2012/10/25 20:59:03, Vitaly Buka wrote:
> Alexei, I just tried to change print_web_view_helper.cc to render header and
> footer just by creating WebKit::WebView and feeding that with required text.
> It works and resolve font issues. Also looks like this approach would allow to
> remove a lot of code from print_web_view_helper.
> I'd like to go this way. Do I miss something?

+cc some other folks who've worked on this code. I'm not sure about the history
behind the print header / footer code - perhaps Arthur or Kausalya may know why
that approach wasn't tried before.

I had assumed that there may be some obstacles involved in doing it that way,
but it sounds like you've got the hang of it?

I agree if WebKit can take care of it, it makes sense to let it.

[kmadhusu, 2012-10-25 21:34:26]

I am sorry, I don't know much about these code. Adding vandebo@ to the reviewer
list.

[Vitaly Buka (NO REVIEWS), 2012-10-25 21:41:58]

vandebo@ It's not review. Ignore that code.

Just asking, if there is any objection to render header/footer in print preview
using Webkit instead of direct calls to Skia.

On 2012/10/25 21:34:26, kmadhusu wrote:
> I am sorry, I don't know much about these code. Adding vandebo@ to the
reviewer
> list.

[vandebo (ex-Chrome), 2012-10-25 21:42:46]

mm, Windows and fonts.. deferring to Arthur

[Vitaly Buka (NO REVIEWS), 2012-10-25 21:45:34]

In prev message skia -> gfx::RenderText

On 2012/10/25 21:41:58, Vitaly Buka wrote:
> vandebo@ It's not review. Ignore that code.
> 
> Just asking, if there is any objection to render header/footer in print
preview
> using Webkit instead of direct calls to Skia.
> 
> On 2012/10/25 21:34:26, kmadhusu wrote:
> > I am sorry, I don't know much about these code. Adding vandebo@ to the
> reviewer
> > list.

[vandebo (ex-Chrome), 2012-10-25 21:48:53]

On 2012/10/25 21:41:58, Vitaly Buka wrote:
> vandebo@ It's not review. Ignore that code.
> 
> Just asking, if there is any objection to render header/footer in print
preview
> using Webkit instead of direct calls to Skia.

Ohh, I have no objection to that.  We had an intern that did the header/footer
code and at some point we talked about using WebKit to draw the headers and
footers and it wasn't clear how to do it.  If there's some advantage to drawing
the headers/footers in WebKit, that's fine.

Recalling a little more, WebKit doesn't normally draw in the margin area, so
it's not clear to me if you can get WebKit to draw in the negative coordinate
space; see also setDrawingArea(SkPDFDevice::kMargin_DrawingArea)

> 
> On 2012/10/25 21:34:26, kmadhusu wrote:
> > I am sorry, I don't know much about these code. Adding vandebo@ to the
> reviewer
> > list.

[arthurhsu, 2012-10-25 21:49:58]

On 2012/10/25 21:48:53, vandebo wrote:
> On 2012/10/25 21:41:58, Vitaly Buka wrote:
> > vandebo@ It's not review. Ignore that code.
> > 
> > Just asking, if there is any objection to render header/footer in print
> preview
> > using Webkit instead of direct calls to Skia.
> 
> Ohh, I have no objection to that.  We had an intern that did the header/footer
> code and at some point we talked about using WebKit to draw the headers and
> footers and it wasn't clear how to do it.  If there's some advantage to
drawing
> the headers/footers in WebKit, that's fine.
> 
> Recalling a little more, WebKit doesn't normally draw in the margin area, so
> it's not clear to me if you can get WebKit to draw in the negative coordinate
> space; see also setDrawingArea(SkPDFDevice::kMargin_DrawingArea)
> 
> > 
> > On 2012/10/25 21:34:26, kmadhusu wrote:
> > > I am sorry, I don't know much about these code. Adding vandebo@ to the
> > reviewer
> > > list.

LGTM