[chromium] don't call fontconfig twice in complex text path

• Component: New Bugs
• Status: RESOLVED FIXED

[Evan Martin]

[chromium] don't call fontconfig twice in complex text path

[Evan Martin]

Created attachment 55312 [details]
Patch

[WebKit Review Bot]

Attachment 55312 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Martin]

Created attachment 55313 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55313 [details]
Patch

I don't understand this change.  I think your ChangeLog could better explain what you're doing and why.

[Evan Martin]

Created attachment 55588 [details]
Patch

[Evan Martin]

New patch with more in the ChangeLog.

[Evan Martin]

Dave: you r+'d the identical patch (with a different ChangeLog) in https://bugs.webkit.org/show_bug.cgi?id=37904 .

[David Levin]

Comment on attachment 55588 [details]
Patch

Ok

Something to consider: Seems unfortunate that the code in FontCache::getFontDataForCharacters is so similar to the code in  FontCache::createFontPlatformData.

[WebKit Commit Bot]

Comment on attachment 55588 [details]
Patch

Rejecting patch 55588 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Last 500 characters of output:
 that name could be found
Testing 18336 test cases.
media/video-loop.html -> timed out
Sampling process 41497 for 10 seconds with 10 milliseconds of run time between samples
Sampling completed, processing symbols...
Sample analysis of process 41497 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt

Exiting early after 1 failures. 14932 tests run.
521.95s total testing time

14931 test cases (99%) succeeded
1 test case (<1%) timed out
12 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/2180152

[Evan Martin]

Comment on attachment 55588 [details]
Patch

Given that the failure was on Mac and this was a Linux-specific change, I'm going to run it through the commit bot again.

[Eric Seidel (no email)]

(In reply to comment #10)
> (From update of attachment 55588 [details])
> Given that the failure was on Mac and this was a Linux-specific change, I'm going to run it through the commit bot again.

The false positive was from bug 38912.

[WebKit Commit Bot]

Comment on attachment 55588 [details]
Patch

Clearing flags on attachment: 55588

Committed r59483: <http://trac.webkit.org/changeset/59483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Re-opening since this was rolled out.

[ningxin hu]

Created attachment 63206 [details]
Patch utilizes the PlatformFontDataCache to improve the performance.

[Tony Chang]

(In reply to comment #15)
> Created an attachment (id=63206) [details]
> Patch utilizes the PlatformFontDataCache to improve the performance.

Evan, can you review this?

[David Levin]

Adding Evan so that he will see this and look it over.

[Evan Martin]

I feel like this global:
  static SkTypeface* gTypefaceForChars = 0;
is kind of a hack.

I worry that there is a code path where we call ::getFontDataForCharacters once, set gTypefaceForChars to value A, then call ::getFontDataForCharacters again and set it to value B, then call ::createFontPlatformData with the info in A and get B back from the global.

[David Levin]

Comment on attachment 63206 [details]
Patch utilizes the PlatformFontDataCache to improve the performance.

r- due to Evan's comment.

[ningxin hu]

Evan, thanks for your review. Yes, it makes the getFontDataForCharacters non-reentrant. And it might not ease the performance issue. I will investigate more.

[Evan Martin]

I worry the correct fix to font loading in general is more complicated.

I think what we should do is put more of the control into WebKit:
- Call to fontconfig once in getFontDataForCharacters to resolve a font request all the way to a specific filename and font settings.  Never call fontconfig a second time for this font.  That may mean extending what FontDescription includes.
- createFontPlatformData could then just open that font file and give the opened font to Skia.


I added the SkTypeface::CreateFromName call recently (in the last six months) and I think it was maybe the wrong design.  I think Skia doesn't have enough information to be able to open the correct font on Linux if we only give it a font name.

[ningxin hu]

As my opinion, call SkTypeface::CreateForChars in getFontDataForCharacters is enough to match a correct font (just like what r59483 does). 

getFontDataForCharacters passes the characters and style in, SkTypeface::CreateForChars uses all of them to match a font, there is no information loss.

In previous implementation, getFontDataForCharacters uses ChromiumBridge::getFontFamilyForCharacters to map characters to font family. Then createFontPlatformData uses family name and style to call SkTypeface::CreateFromName to match a font, there is information loss, since fonts with different char coverage might have same family name. (case is http://code.google.com/p/chromium/issues/detail?id=32109)

The issue of r59483 is performance loss. I constructed a similar page_cycler test as page_cycler_intl1, and reproduced it. As my finding, the root cause is ChromiumBridge::getFontFamilyForCharacters has cache to avoid some IPC (in RendererWebKitClientImpl::SandboxSupport::getFontFamilyForCharacters). However, in r59483, each SkTypeface::CreateForChars involves IPC.

I wrote a patch to add a typeface cache into r59483, the performance loss disappears. For memory usage,  RendererWebKitClientImpl::SandboxSupport::getFontFamilyForCharacters cache is purged when PurgeRenderers. So I purge the typeface cache when FontCache invalidate. But it involves WebKit API change, other platforms need to modify too. At least, Windows platform (FontCacheChromiumWin) has a FontCmapCache which needs to purge too. 

Please review this patch. Your comments are appreciated.

[ningxin hu]

Created attachment 63537 [details]
add typeface cache

Please review. Thanks.

[WebKit Review Bot]

Attachment 63537 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/FontCacheLinux.cpp:67:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/platform/graphics/chromium/FontCacheLinux.cpp:85:  Declaration has space between type name and * in SkTypeface *tf  [whitespace/declaration] [3]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[ningxin hu]

Created attachment 63553 [details]
patch to add typeface cache

update for style check error

[Hajime Morrita]

Comment on attachment 63553 [details]
patch to add typeface cache

Hi, thank you for your patch! I'll try to take a look:

> --- a/WebCore/platform/graphics/FontCache.h
> +++ b/WebCore/platform/graphics/FontCache.h
> @@ -102,6 +102,7 @@ private:
>      // These methods are implemented by each platform.
>      SimpleFontData* getSimilarFontPlatformData(const Font&);
>      FontPlatformData* createFontPlatformData(const FontDescription&, const AtomicString& family);
> +    void invalidatePlatform();
Who call this? If chromium side, it's OK, if it will be called inside webkit, such code might be bette to stay in same patch.

> +typedef HashMap<String, SkTypeface*> TypefaceCache;
> +
> +static TypefaceCache *gTypefaceChache = 0;
> +
Because FontCache instance itself global, 
It can be a class member field instead of static variable.
Dynamically allocated static variable typically make valgrind unhappy, 
we should avoid them if possible.

[ningxin hu]

(In reply to comment #26)
Thanks for your comments. 

> (From update of attachment 63553 [details])
> Hi, thank you for your patch! I'll try to take a look:
> 
> > --- a/WebCore/platform/graphics/FontCache.h
> > +++ b/WebCore/platform/graphics/FontCache.h
> > @@ -102,6 +102,7 @@ private:
> >      // These methods are implemented by each platform.
> >      SimpleFontData* getSimilarFontPlatformData(const Font&);
> >      FontPlatformData* createFontPlatformData(const FontDescription&, const AtomicString& family);
> > +    void invalidatePlatform();
> Who call this? If chromium side, it's OK, if it will be called inside webkit, such code might be bette to stay in same patch.

In my early thoughts, it is called by FontCache::invalidate. However, FontCache::invalidate is called by MemoryPurger in chromium via task manager. So it is not a big deal. And it changes WebKit API, all platforms are impacted. So I prefer to remove it. In my page_cycler and other tests, the memory usage doesn't increase comparing to HEAD.  Render shutdown will release the typeface cache. 

> 
> > +typedef HashMap<String, SkTypeface*> TypefaceCache;
> > +
> > +static TypefaceCache *gTypefaceChache = 0;
> > +
> Because FontCache instance itself global, 
> It can be a class member field instead of static variable.
> Dynamically allocated static variable typically make valgrind unhappy, 
> we should avoid them if possible.

I just avoid to change WebKit API (FontCache.h). So by referring to FontCacheChromiumWin.cpp::fontCmapCache, I modify the patch in that way.

[ningxin hu]

Created attachment 63563 [details]
patch

Updated. Please review. Thanks.

[ningxin hu]

Comment on attachment 63563 [details]
patch

Could someone please review my updated patch? Thanks.

[Evan Martin]

This cache looks like it will grow without ever shrinking.
Won't it include a copy of every character ever rendered?
It's also not correct to not include the font information in the cache (for example, this cache will return the wrong font when asked for the same characters in bold or not bold).

More broadly, I think you need to spend some time understanding FontCache.  Maybe it has the appropriate setup to cache what you need.  (I don't understand it completely myself.)

[ningxin hu]

(In reply to comment #30)
> This cache looks like it will grow without ever shrinking.
> Won't it include a copy of every character ever rendered?
The memory usage of this typeface cache is similar to unicode_font_families_ cache in RendererWebKitClientImpl::SandboxSupport::getFontFamilyForCharacters which is currently used. They are destroyed with the render (usually switching to another site will destroy the render.)
In my page_cycler test, chromium with my patch doesn't increase memory usage comparing to HEAD. And I also tested different sites switch cases, the memory usage behavior is not different. 
For another instance, FontCacheChromiumWin also has a FontCmapCache which doesn't shrink. It is acceptable.

> It's also not correct to not include the font information in the cache (for example, this cache will return the wrong font when asked for the same characters in bold or not bold).
Yes. This is what I want to get feedbacks from you. To combine the style info to cache key is better. If you think OK, I will add it. 

> 
> More broadly, I think you need to spend some time understanding FontCache.  Maybe it has the appropriate setup to cache what you need.  (I don't understand it completely myself.)
I agree with you that I don't understand font cache well. But I just want to fix the bug. The complex font selection bug on Linux is annoying for non-English users. 
As my knowledge and test, I think your fix at r59483 works. But it is rolled out according to performance regression. I just tried to find the root cause and provide a fix. Besides that, I did the test to make sure my fix doesn't bring other regressions such as memory leaks.  
I did investigate other ways, such as passing the character info to createFontPlatformData, but it involves WebKit API change, other platforms are impacted. 
I submitted this patch for the three reasons:
1. fix the complex font selection problem (https://bugs.webkit.org/show_bug.cgi?id=37904), pass the test case for that. (the cache key need to update)
2. fix the performance loss of page_cycler
3. no more memory leaks comparing to current implementation. 

So could you share with me what your most concerns? And in your mind, what's the right way to fix it? Thanks in advance.

[David Levin]

Comment on attachment 63563 [details]
patch

Based on Evan's comment, it sounds like there are problems with this version of the patch so r-.

[ningxin hu]

Created attachment 64925 [details]
updated patch

Update patch based on Evan's comment to add style info to typeface cache. Please review. Thanks.

[ningxin hu]

Any comments about patch in Comment #33? Question again, could you share with me what your most concerns? And in your mind, what's the right way to fix it? Thanks in advance.

[Adam Barth]

Comment on attachment 64925 [details]
updated patch

> WebCore/ChangeLog:11
> +        [chromium] don't call fontconfig twice in complex text path
> +        https://bugs.webkit.org/show_bug.cgi?id=38701
We usually put this information just under the "Reviewed by" line.

> WebCore/platform/graphics/chromium/FontCacheLinux.cpp:61
>  const SimpleFontData* FontCache::getFontDataForCharacters(const Font& font,
>                                                            const UChar* characters,
>                                                            int length)
These should be one line.

> WebCore/platform/graphics/chromium/FontCacheLinux.cpp:69
> +    // This cache is just leaked on shutdown.
That's true for mosts of our caches like this.  We don't need a comment explaining that.

[ningxin hu]

Created attachment 65987 [details]
updated patch based on comment #35

Thanks for review. Updated patch based on comment #35.

[Evan Martin]

I'm sorry I haven't provided better review feedback, but this area of WebKit is  complicated enough that I can only start to remember how it works when I've been reading the code, and lately I've been working on other things.

I do know that the FontCache class itself is a cache of FontData which is a wrapper around PlatformFontData, which maps to an SkTypeface.  That is why I think adding a second cache inside the cache class is wrong.

[ningxin hu]

(In reply to comment #37)
> I do know that the FontCache class itself is a cache of FontData which is a wrapper around PlatformFontData, which maps to an SkTypeface.  That is why I think adding a second cache inside the cache class is wrong.

Evan, thanks for your comments. As my understanding, FontCache maps from family name (with style etc.,) to SkTypeface. However, the newly added typeface cache maps from character to SkTypeface. They are for different purpose.

There are two reasons to add this cache:
1. if use FontCahce only and ChromiumBridge::getFontFamilyForCharacters to map from character to family name, there is information loss, which causes bug https://bugs.webkit.org/show_bug.cgi?id=37904.
2. if use SkTypeface::CreateForChars without the typeface cache, each SkTypeface::CreateForChars will invoke a IPC, which causes the performance regression https://bugs.webkit.org/show_bug.cgi?id=39215.

Again, any comments are appreciated.

[ningxin hu]

Do you guys have any comments? Thanks in advance.

[Adam Barth]

Comment on attachment 65987 [details]
updated patch based on comment #35

This patch has been up for review for a long time.  None of the folks who are qualified to review this code seem that interested in this patch.  I don't mean to be unfriendly, but I think we should put this patch aside for a while.  If someone feels confident in their ability to review this patch, please don't let this review stand in your way.

[Kenichi Ishibashi]

Created attachment 101449 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #41)
> Created an attachment (id=101449) [details]
> Patch

This patch looks not so good, but as ningxin mentioned, WebKit API changes might be needed for more sophisticated solutions.  I'd like to ask suggestions and comments.

Thanks,

[WebKit Review Bot]

Comment on attachment 101449 [details]
Patch

Attachment 101449 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9199518

New failing tests:
fast/blockflow/japanese-rl-text.html
fast/repaint/japanese-rl-selection-repaint.html
fast/forms/implicit-submission.html
editing/selection/vertical-rl-ltr-extend-line-backward-p.html
fast/blockflow/japanese-ruby-vertical-rl.html
fast/ruby/base-shorter-than-text.html
editing/selection/vertical-rl-ltr-extend-line-forward-p.html
fast/blockflow/vertical-baseline-alignment.html
editing/selection/vertical-rl-ltr-extend-line-backward-br.html
editing/selection/vertical-rl-ltr-extend-line-forward-br.html
fast/blockflow/japanese-lr-text.html
editing/selection/vertical-lr-ltr-extend-line-forward-br.html
fast/dynamic/text-combine.html
editing/selection/vertical-rl-ltr-extend-line-forward-wrap.html
fast/repaint/japanese-rl-selection-clear.html
fast/blockflow/japanese-ruby-vertical-lr.html
fast/blockflow/vertical-align-table-baseline.html
editing/selection/vertical-rl-ltr-extend-line-backward-wrap.html
editing/selection/vertical-lr-ltr-extend-line-backward-br.html

[Hajime Morrita]

Comment on attachment 101449 [details]
Patch

r- at this time as cr-linux break.

[Kenichi Ishibashi]

Created attachment 101960 [details]
Patch (Fix breaking tests)

[Kenichi Ishibashi]

(In reply to comment #45)
> Created an attachment (id=101960) [details]
> Patch (Fix breaking tests)

Any comments/suggestions on this patch?

[Evan Martin]

If you look at the history of this, around comment 37 we discuss a previous patch that also introduces a cache.  I asked then: aren't there already caches for this kind of data in WebKit?

[Evan Martin]

Oh wait, I didn't understand the patch.  :(

I guess a better way of phrasing the question is: what is special about Chromiun Linux that requires additional API in FontCache?

[Kenichi Ishibashi]

Created attachment 106327 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #48)
> I guess a better way of phrasing the question is: what is special about Chromiun Linux that requires additional API in FontCache?

Sorry for late response. I changed the approach to fix the problem. In the revised patch, PlatformSupport::getFontFamilyForCharacters() returns the bold and italic properties of the retrieved font family so that WebKit can maintain the correct mapping of the given characters. This patch doesn't require additional API in FontCache.

Even though updating Chromium side is needed, DumpRenderTree should work with this patch and should pass fast/text/international/bold-bengali.html. Some tests might require updating expectations.

If this approach sounds reasonable, I'll create a patch of Chromium part (and will modify the bug title, which is no longer appropriate).

[WebKit Review Bot]

Comment on attachment 106327 [details]
Patch

Attachment 106327 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9594323

New failing tests:
fast/text/international/khmer-selection.html
fast/text/cg-fallback-bolding.html

[Kenichi Ishibashi]

(In reply to comment #50)
> If this approach sounds reasonable, I'll create a patch of Chromium part (and will modify the bug title, which is no longer appropriate).

Any comments or suggestions? I'll revise the patch and rebase expectations if the approach looks good.

[Kenichi Ishibashi]

Created attachment 113272 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Kenichi Ishibashi]

(In reply to comment #53)
> Created an attachment (id=113272) [details]
> Patch

Rebased to ToT. Following tests will need to rebase.
fast/text/international/khmer-selection.html
fast/text/cg-fallback-bolding.html

[Kenichi Ishibashi]

Created attachment 113273 [details]
patch for chromium side

[WebKit Review Bot]

Comment on attachment 113272 [details]
Patch

Attachment 113272 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10128488

New failing tests:
fast/text/international/khmer-selection.html
fast/text/cg-fallback-bolding.html

[Darin Fisher (:fishd, Google)]

Comment on attachment 113272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113272&action=review

> Source/WebCore/platform/chromium/PlatformSupport.h:158
> +    static String getFontFamilyForCharacters(const UChar*, size_t numCharacters, const char* preferredLocale, bool* isBold, bool* isItalic);

would it perhaps be better if this returned a FontFamily struct?

  struct FontFamily {
      String name;
      bool isBold;
      bool isItalic;
  };

This way you don't have to worry about mixing up the order of the bool* parameters at the callsite.

> Source/WebKit/chromium/public/linux/WebFontInfo.h:56
> +    WEBKIT_EXPORT static WebCString familyForChars(const WebUChar* characters, size_t numCharacters, const char* preferredLocale, bool* isBold = 0, bool* isItalic = 0);

it is perhaps interesting that this one returns WebCString but WebSandboxSupport's version returns WebString.
is there a reason why they are different?  perhaps we should invent a WebFontFamily struct for use by this
function and the one on WebSandboxSupport?

[Kenichi Ishibashi]

Created attachment 113297 [details]
Patch

[Kenichi Ishibashi]

Created attachment 113298 [details]
patch for chromium side

[Kenichi Ishibashi]

Comment on attachment 113272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113272&action=review

Thank you for the review!

>> Source/WebCore/platform/chromium/PlatformSupport.h:158
>> +    static String getFontFamilyForCharacters(const UChar*, size_t numCharacters, const char* preferredLocale, bool* isBold, bool* isItalic);
> 
> would it perhaps be better if this returned a FontFamily struct?
> 
>   struct FontFamily {
>       String name;
>       bool isBold;
>       bool isItalic;
>   };
> 
> This way you don't have to worry about mixing up the order of the bool* parameters at the callsite.

Done.

>> Source/WebKit/chromium/public/linux/WebFontInfo.h:56
>> +    WEBKIT_EXPORT static WebCString familyForChars(const WebUChar* characters, size_t numCharacters, const char* preferredLocale, bool* isBold = 0, bool* isItalic = 0);
> 
> it is perhaps interesting that this one returns WebCString but WebSandboxSupport's version returns WebString.
> is there a reason why they are different?  perhaps we should invent a WebFontFamily struct for use by this
> function and the one on WebSandboxSupport?

Indeed. In any case, font name is in WebFontFamily structure and the return value is void.

[WebKit Review Bot]

Comment on attachment 113297 [details]
Patch

Attachment 113297 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10148223

New failing tests:
fast/text/international/khmer-selection.html
fast/text/cg-fallback-bolding.html

[Tony Chang]

(In reply to comment #62)
> (From update of attachment 113297 [details])
> Attachment 113297 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10148223
> 
> New failing tests:
> fast/text/international/khmer-selection.html
> fast/text/cg-fallback-bolding.html

Can you upload the new results for these tests?  It's hard to know the correctness of the patch without seeing the before and after images.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113297&action=review

> Source/WebKit/chromium/public/linux/WebFontFamily.h:39
> +struct WEBKIT_EXPORT WebFontFamily {

I don't think you should need WEBKIT_EXPORT here.

> Source/WebKit/chromium/src/PlatformSupport.cpp:479
> +    family->name = String::fromUTF8(webFamily.name.data());

hmm... perhaps there should be a String::fromUTF8() variant that takes a |const CString&|?  if name happened to contain embedded nulls, this conversion would result in truncation.  that probably isn't going to happen in this case, but it always feels a bit risky to use .data() like this.

[Jungshik Shin]

Comment on attachment 113297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113297&action=review

> Source/WebKit/chromium/src/linux/WebFontInfo.cpp:112
> +        if (FcPatternGetInteger(current, FC_WEIGHT, 0, &weight) == FcResultMatch)

|familyForChars|  does not have any style info (bold or italic) so that fontconfig input pattern does not have that, either. That is, font selection is entirely based on characters and pref_locale, isn't it? If so, doesn't fontconfig return a regular weight font in virtually all cases (before bold / italic fonts)? So, isBold and isItalic will never bet set to true, will they? 

If the above is taken together with the change made in FontCache::getFontDataForCharacters (partly quoted below), don't we end up using fakeBold and fakeItalic for even characters for which we have real bold and italic fonts? 

 76    if (!family.isBold && description.weight() >= FontWeightBold) {
 77        shouldSetFakeBold = true;
 78        description.setWeight(FontWeightNormal);
 79    }

Hmm.. in that case, some layout test must have failed.. So, I guess I'm missing something, in which case please accept my apology. 

Anyway, if my reading is correct, shouldn't we pass style info to |familyForChars| and add that to fontconfig 'input pattern'?

[Kenichi Ishibashi]

Created attachment 113611 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #63)
> Can you upload the new results for these tests?  It's hard to know the correctness of the patch without seeing the before and after images.

The revised patch contains new test results.

[Kenichi Ishibashi]

Comment on attachment 113297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113297&action=review

>> Source/WebKit/chromium/public/linux/WebFontFamily.h:39
>> +struct WEBKIT_EXPORT WebFontFamily {
> 
> I don't think you should need WEBKIT_EXPORT here.

Done.

>> Source/WebKit/chromium/src/PlatformSupport.cpp:479
>> +    family->name = String::fromUTF8(webFamily.name.data());
> 
> hmm... perhaps there should be a String::fromUTF8() variant that takes a |const CString&|?  if name happened to contain embedded nulls, this conversion would result in truncation.  that probably isn't going to happen in this case, but it always feels a bit risky to use .data() like this.

Changed to use String::fromUTF8(const LChar*, size_t) variant, which might be equivalent to pass const CString&, if my understanding is correct.

>> Source/WebKit/chromium/src/linux/WebFontInfo.cpp:112
>> +        if (FcPatternGetInteger(current, FC_WEIGHT, 0, &weight) == FcResultMatch)
> 
> |familyForChars|  does not have any style info (bold or italic) so that fontconfig input pattern does not have that, either. That is, font selection is entirely based on characters and pref_locale, isn't it? If so, doesn't fontconfig return a regular weight font in virtually all cases (before bold / italic fonts)? So, isBold and isItalic will never bet set to true, will they? 
> 
> If the above is taken together with the change made in FontCache::getFontDataForCharacters (partly quoted below), don't we end up using fakeBold and fakeItalic for even characters for which we have real bold and italic fonts? 
> 
>  76    if (!family.isBold && description.weight() >= FontWeightBold) {
>  77        shouldSetFakeBold = true;
>  78        description.setWeight(FontWeightNormal);
>  79    }
> 
> Hmm.. in that case, some layout test must have failed.. So, I guess I'm missing something, in which case please accept my apology. 
> 
> Anyway, if my reading is correct, shouldn't we pass style info to |familyForChars| and add that to fontconfig 'input pattern'?

Could you read the Evan's original report on http://crbug.com/32109 ?

|family| is the output argument and isn't used for font selection. It just holds font name and style info which fontconfig selected.

[Kenichi Ishibashi]

Ping?

[Tony Chang]

Comment on attachment 113611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113611&action=review

This approach seems reasonable to me. r- because we need to figure out how to land this without breaking the chromium build.

> Source/WebKit/chromium/public/linux/WebFontInfo.h:36
>  #include "../WebCString.h"
> +#include "../linux/WebFontFamily.h"
>  #include "../linux/WebFontRenderStyle.h"

fishd may have a preference on whether the includes from the same dir should just be "WebFontFamily.h" or "../linux/WebFontFamily.h". I don't have a strong preference one way or another.

> Source/WebKit/chromium/public/linux/WebFontInfo.h:53
> -    WEBKIT_EXPORT static WebCString familyForChars(const WebUChar* characters, size_t numCharacters, const char* preferredLocale);
> +    // Returns: the font family instance. The instance has an empty font name if the request could not be satisfied.
> +    WEBKIT_EXPORT static void familyForChars(const WebUChar* characters, size_t numCharacters, const char* preferredLocale, WebFontFamily*);

Is changing this function going to break the chromium build?  It looks like this called from content/browser/renderer_host/render_sandbox_host_linux.cc:270.  I think you need to do a 3 sided patch to migrate to the new API.

> Source/WebKit/chromium/public/linux/WebSandboxSupport.h:55
> -    virtual WebString getFontFamilyForCharacters(const WebUChar* characters, size_t numCharacters, const char* preferredLocale)  = 0;
> +    // Returns a WebFontFamily instance with the font name. The instance has empty font name if the request cannot be satisfied.
> +    virtual void getFontFamilyForCharacters(const WebUChar* characters, size_t numCharacters, const char* preferredLocale, WebFontFamily*)  = 0;

Is changing this function going to break the chromium build?  It looks like this is implemented in content/renderer/renderer_webkitplatformsupport_impl.cc.

> Source/WebKit/chromium/src/PlatformSupport.cpp:441
> +    return;

Nit: remove return

> Source/WebKit/chromium/src/PlatformSupport.cpp:451
> +    return;

Nit: remove return

> Source/WebKit/chromium/src/linux/WebFontInfo.cpp:126
> +    return;

Nit: remove return

> LayoutTests/platform/chromium/test_expectations.txt:1969
> +BUGUKAI LINUX : fast/text/international/bold-bengali.html = PASS

This shouldn't be necessary.  The test should already be running on Linux.  What are you trying to do here?

[Kenichi Ishibashi]

Created attachment 115355 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 113611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113611&action=review

Hi Tony,

Thank you for review. Actually, this change will break the chromium build so I left the old API and enclose new behavior in #if 0 ... #endif. I'll remove old API and enable new behavior after Chromium side is done.

>> Source/WebKit/chromium/public/linux/WebFontInfo.h:36
>>  #include "../linux/WebFontRenderStyle.h"
> 
> fishd may have a preference on whether the includes from the same dir should just be "WebFontFamily.h" or "../linux/WebFontFamily.h". I don't have a strong preference one way or another.

Leaving them as-is for now.

>> Source/WebKit/chromium/public/linux/WebFontInfo.h:53
>> +    WEBKIT_EXPORT static void familyForChars(const WebUChar* characters, size_t numCharacters, const char* preferredLocale, WebFontFamily*);
> 
> Is changing this function going to break the chromium build?  It looks like this called from content/browser/renderer_host/render_sandbox_host_linux.cc:270.  I think you need to do a 3 sided patch to migrate to the new API.

I left the old API so that this change won't break the chromium build.

>> Source/WebKit/chromium/public/linux/WebSandboxSupport.h:55
>> +    virtual void getFontFamilyForCharacters(const WebUChar* characters, size_t numCharacters, const char* preferredLocale, WebFontFamily*)  = 0;
> 
> Is changing this function going to break the chromium build?  It looks like this is implemented in content/renderer/renderer_webkitplatformsupport_impl.cc.

Ditto.

>> Source/WebKit/chromium/src/PlatformSupport.cpp:441
>> +    return;
> 
> Nit: remove return

Done.

>> Source/WebKit/chromium/src/PlatformSupport.cpp:451
>> +    return;
> 
> Nit: remove return

Done.

>> Source/WebKit/chromium/src/linux/WebFontInfo.cpp:126
>> +    return;
> 
> Nit: remove return

Done.

>> LayoutTests/platform/chromium/test_expectations.txt:1969
>> +BUGUKAI LINUX : fast/text/international/bold-bengali.html = PASS
> 
> This shouldn't be necessary.  The test should already be running on Linux.  What are you trying to do here?

Removed.

[Tony Chang]

Comment on attachment 115355 [details]
Patch

Great, LGTM!

[Tony Chang]

Oh, you may want to give fishd a chance to review the WebKit API changes.

[Kenichi Ishibashi]

(In reply to comment #74)
> Oh, you may want to give fishd a chance to review the WebKit API changes.

Thanks for r+. I'll wait for fishd's response till tomorrow.

BTW, I've just noticed that the old API in WebSandboxSupport should be a non-pure virtual function so that we can migrate API in Chromium side after this patch is landed. I'll modify the API before landing this patch. (I tested it locally and didn't see any problem).

[Darin Fisher (:fishd, Google)]

Comment on attachment 115355 [details]
Patch

LGTM

[Kenichi Ishibashi]

Created attachment 115536 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 115536 [details]
Patch for landing

Rejecting attachment 115536 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10511094

[Kenichi Ishibashi]

Created attachment 115549 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 115549 [details]
Patch for landing

Clearing flags on attachment: 115549

Committed r100601: <http://trac.webkit.org/changeset/100601>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.