Bug 38701 - [chromium] don't call fontconfig twice in complex text path
Summary: [chromium] don't call fontconfig twice in complex text path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on: 39215
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-06 15:48 PDT by Evan Martin
Modified: 2011-11-17 04:17 PST (History)
16 users (show)

See Also:


Attachments
Patch (3.78 KB, patch)
2010-05-06 15:55 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (3.80 KB, patch)
2010-05-06 16:01 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (4.06 KB, patch)
2010-05-10 12:13 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch utilizes the PlatformFontDataCache to improve the performance. (4.24 KB, patch)
2010-08-02 06:01 PDT, ningxin hu
levin: review-
Details | Formatted Diff | Diff
add typeface cache (4.25 KB, patch)
2010-08-04 20:21 PDT, ningxin hu
no flags Details | Formatted Diff | Diff
patch to add typeface cache (4.25 KB, patch)
2010-08-04 22:33 PDT, ningxin hu
no flags Details | Formatted Diff | Diff
patch (3.33 KB, patch)
2010-08-05 01:20 PDT, ningxin hu
levin: review-
Details | Formatted Diff | Diff
updated patch (3.38 KB, patch)
2010-08-19 22:41 PDT, ningxin hu
no flags Details | Formatted Diff | Diff
updated patch based on comment #35 (3.44 KB, patch)
2010-08-30 17:57 PDT, ningxin hu
no flags Details | Formatted Diff | Diff
Patch (7.57 KB, patch)
2011-07-20 04:39 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (Fix breaking tests) (9.16 KB, patch)
2011-07-25 19:07 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (11.78 KB, patch)
2011-09-05 06:15 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (12.87 KB, patch)
2011-11-01 19:44 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
patch for chromium side (6.25 KB, text/plain)
2011-11-01 19:49 PDT, Kenichi Ishibashi
no flags Details
Patch (17.28 KB, patch)
2011-11-02 03:33 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
patch for chromium side (10.42 KB, text/plain)
2011-11-02 03:35 PDT, Kenichi Ishibashi
no flags Details
Patch (79.98 KB, patch)
2011-11-03 20:00 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (16.26 KB, patch)
2011-11-16 03:14 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (16.38 KB, patch)
2011-11-16 23:56 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (16.37 KB, patch)
2011-11-17 02:48 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Martin 2010-05-06 15:48:07 PDT
[chromium] don't call fontconfig twice in complex text path
Comment 1 Evan Martin 2010-05-06 15:55:17 PDT
Created attachment 55312 [details]
Patch
Comment 2 WebKit Review Bot 2010-05-06 15:58:44 PDT
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.
Comment 3 Evan Martin 2010-05-06 16:01:23 PDT
Created attachment 55313 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-05-07 21:59:10 PDT
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.
Comment 5 Evan Martin 2010-05-10 12:13:14 PDT
Created attachment 55588 [details]
Patch
Comment 6 Evan Martin 2010-05-10 12:14:42 PDT
New patch with more in the ChangeLog.
Comment 7 Evan Martin 2010-05-10 12:16:18 PDT
Dave: you r+'d the identical patch (with a different ChangeLog) in https://bugs.webkit.org/show_bug.cgi?id=37904 .
Comment 8 David Levin 2010-05-10 13:11:01 PDT
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.
Comment 9 WebKit Commit Bot 2010-05-12 08:48:52 PDT
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
Comment 10 Evan Martin 2010-05-13 00:04:44 PDT
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.
Comment 11 Eric Seidel (no email) 2010-05-13 05:46:01 PDT
(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.
Comment 12 WebKit Commit Bot 2010-05-14 13:16:23 PDT
Comment on attachment 55588 [details]
Patch

Clearing flags on attachment: 55588

Committed r59483: <http://trac.webkit.org/changeset/59483>
Comment 13 WebKit Commit Bot 2010-05-14 13:16:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Eric Seidel (no email) 2010-05-17 11:10:27 PDT
Re-opening since this was rolled out.
Comment 15 ningxin hu 2010-08-02 06:01:59 PDT
Created attachment 63206 [details]
Patch utilizes the PlatformFontDataCache to improve the performance.
Comment 16 Tony Chang 2010-08-02 18:09:25 PDT
(In reply to comment #15)
> Created an attachment (id=63206) [details]
> Patch utilizes the PlatformFontDataCache to improve the performance.

Evan, can you review this?
Comment 17 David Levin 2010-08-03 14:13:16 PDT
Adding Evan so that he will see this and look it over.
Comment 18 Evan Martin 2010-08-03 14:21:26 PDT
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.
Comment 19 David Levin 2010-08-03 17:01:37 PDT
Comment on attachment 63206 [details]
Patch utilizes the PlatformFontDataCache to improve the performance.

r- due to Evan's comment.
Comment 20 ningxin hu 2010-08-03 18:11:34 PDT
Evan, thanks for your review. Yes, it makes the getFontDataForCharacters non-reentrant. And it might not ease the performance issue. I will investigate more.
Comment 21 Evan Martin 2010-08-03 18:19:31 PDT
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.
Comment 22 ningxin hu 2010-08-04 19:33:21 PDT
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.
Comment 23 ningxin hu 2010-08-04 20:21:20 PDT
Created attachment 63537 [details]
add typeface cache

Please review. Thanks.
Comment 24 WebKit Review Bot 2010-08-04 20:22:09 PDT
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.
Comment 25 ningxin hu 2010-08-04 22:33:11 PDT
Created attachment 63553 [details]
patch to add typeface cache

update for style check error
Comment 26 Hajime Morrita 2010-08-04 22:51:56 PDT
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.
Comment 27 ningxin hu 2010-08-05 01:19:24 PDT
(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.
Comment 28 ningxin hu 2010-08-05 01:20:31 PDT
Created attachment 63563 [details]
patch

Updated. Please review. Thanks.
Comment 29 ningxin hu 2010-08-06 20:29:25 PDT
Comment on attachment 63563 [details]
patch

Could someone please review my updated patch? Thanks.
Comment 30 Evan Martin 2010-08-09 11:35:58 PDT
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.)
Comment 31 ningxin hu 2010-08-09 18:44:23 PDT
(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.
Comment 32 David Levin 2010-08-11 16:28:18 PDT
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-.
Comment 33 ningxin hu 2010-08-19 22:41:22 PDT
Created attachment 64925 [details]
updated patch

Update patch based on Evan's comment to add style info to typeface cache. Please review. Thanks.
Comment 34 ningxin hu 2010-08-29 17:27:31 PDT
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.
Comment 35 Adam Barth 2010-08-30 16:26:25 PDT
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.
Comment 36 ningxin hu 2010-08-30 17:57:13 PDT
Created attachment 65987 [details]
updated patch based on comment #35

Thanks for review. Updated patch based on comment #35.
Comment 37 Evan Martin 2010-08-30 18:14:14 PDT
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.
Comment 38 ningxin hu 2010-08-31 00:25:34 PDT
(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.
Comment 39 ningxin hu 2010-09-02 18:45:07 PDT
Do you guys have any comments? Thanks in advance.
Comment 40 Adam Barth 2010-12-23 00:23:22 PST
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.
Comment 41 Kenichi Ishibashi 2011-07-20 04:39:14 PDT
Created attachment 101449 [details]
Patch
Comment 42 Kenichi Ishibashi 2011-07-20 04:43:08 PDT
(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,
Comment 43 WebKit Review Bot 2011-07-20 18:26:31 PDT
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
Comment 44 Hajime Morrita 2011-07-24 22:16:42 PDT
Comment on attachment 101449 [details]
Patch

r- at this time as cr-linux break.
Comment 45 Kenichi Ishibashi 2011-07-25 19:07:07 PDT
Created attachment 101960 [details]
Patch (Fix breaking tests)
Comment 46 Kenichi Ishibashi 2011-08-16 20:19:50 PDT
(In reply to comment #45)
> Created an attachment (id=101960) [details]
> Patch (Fix breaking tests)

Any comments/suggestions on this patch?
Comment 47 Evan Martin 2011-08-17 08:06:29 PDT
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?
Comment 48 Evan Martin 2011-08-17 08:09:02 PDT
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?
Comment 49 Kenichi Ishibashi 2011-09-05 06:15:27 PDT
Created attachment 106327 [details]
Patch
Comment 50 Kenichi Ishibashi 2011-09-05 06:17:26 PDT
(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).
Comment 51 WebKit Review Bot 2011-09-05 07:07:36 PDT
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
Comment 52 Kenichi Ishibashi 2011-09-20 23:22:05 PDT
(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.
Comment 53 Kenichi Ishibashi 2011-11-01 19:44:57 PDT
Created attachment 113272 [details]
Patch
Comment 54 WebKit Review Bot 2011-11-01 19:46:33 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 55 Kenichi Ishibashi 2011-11-01 19:48:10 PDT
(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
Comment 56 Kenichi Ishibashi 2011-11-01 19:49:57 PDT
Created attachment 113273 [details]
patch for chromium side
Comment 57 WebKit Review Bot 2011-11-01 20:43:52 PDT
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
Comment 58 Darin Fisher (:fishd, Google) 2011-11-01 21:20:17 PDT
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?
Comment 59 Kenichi Ishibashi 2011-11-02 03:33:37 PDT
Created attachment 113297 [details]
Patch
Comment 60 Kenichi Ishibashi 2011-11-02 03:35:14 PDT
Created attachment 113298 [details]
patch for chromium side
Comment 61 Kenichi Ishibashi 2011-11-02 03:39:53 PDT
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.
Comment 62 WebKit Review Bot 2011-11-02 04:23:53 PDT
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
Comment 63 Tony Chang 2011-11-02 09:29:10 PDT
(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.
Comment 64 Darin Fisher (:fishd, Google) 2011-11-02 10:29:28 PDT
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.
Comment 65 Jungshik Shin 2011-11-02 11:58:58 PDT
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'?
Comment 66 Kenichi Ishibashi 2011-11-03 20:00:27 PDT
Created attachment 113611 [details]
Patch
Comment 67 Kenichi Ishibashi 2011-11-03 20:02:09 PDT
(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.
Comment 68 Kenichi Ishibashi 2011-11-03 20:09:13 PDT
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.
Comment 69 Kenichi Ishibashi 2011-11-06 21:39:57 PST
Ping?
Comment 70 Tony Chang 2011-11-15 12:23:41 PST
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?
Comment 71 Kenichi Ishibashi 2011-11-16 03:14:37 PST
Created attachment 115355 [details]
Patch
Comment 72 Kenichi Ishibashi 2011-11-16 03:21:43 PST
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.
Comment 73 Tony Chang 2011-11-16 09:32:52 PST
Comment on attachment 115355 [details]
Patch

Great, LGTM!
Comment 74 Tony Chang 2011-11-16 09:33:27 PST
Oh, you may want to give fishd a chance to review the WebKit API changes.
Comment 75 Kenichi Ishibashi 2011-11-16 22:37:55 PST
(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).
Comment 76 Darin Fisher (:fishd, Google) 2011-11-16 22:55:53 PST
Comment on attachment 115355 [details]
Patch

LGTM
Comment 77 Kenichi Ishibashi 2011-11-16 23:56:59 PST
Created attachment 115536 [details]
Patch for landing
Comment 78 WebKit Review Bot 2011-11-17 01:29:52 PST
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
Comment 79 Kenichi Ishibashi 2011-11-17 02:48:03 PST
Created attachment 115549 [details]
Patch for landing
Comment 80 WebKit Review Bot 2011-11-17 04:17:16 PST
Comment on attachment 115549 [details]
Patch for landing

Clearing flags on attachment: 115549

Committed r100601: <http://trac.webkit.org/changeset/100601>
Comment 81 WebKit Review Bot 2011-11-17 04:17:25 PST
All reviewed patches have been landed.  Closing bug.