Bug 87006 - [chromium] Load custom fonts for layout test on Android.
Summary: [chromium] Load custom fonts for layout test on Android.
Status: RESOLVED DUPLICATE of bug 89721
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hao Zheng
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-05-21 04:55 PDT by Hao Zheng
Modified: 2012-06-24 01:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.39 KB, patch)
2012-05-21 05:06 PDT, Hao Zheng
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hao Zheng 2012-05-21 04:55:51 PDT
[chromium] Load custom fonts for layout test on Android.
Comment 1 Hao Zheng 2012-05-21 05:06:17 PDT
Created attachment 143004 [details]
Patch
Comment 2 Adam Barth 2012-05-21 08:52:40 PDT
Comment on attachment 143004 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:57
> +    static const char* kDeviceFontDirForTest = "/data/drt/fonts/";

It seems strange to have DRT related code in the production target.  Is this common for this file in other ports?  I would have expected this code to be in DumpRenderTree rather than WebCore.
Comment 3 Adam Barth 2012-05-21 08:53:37 PDT
Comment on attachment 143004 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:45
> +#include <string>

It's unusual to use std::string in WebCore.  Typically we use the string classes from WTF.
Comment 4 Eric Seidel (no email) 2012-05-21 14:22:37 PDT
Comment on attachment 143004 [details]
Patch

Seems reasonable.
Comment 5 Eric Seidel (no email) 2012-05-21 14:23:39 PDT
Comment on attachment 143004 [details]
Patch

OK.  NM.  Adam has convinced me.
Comment 6 Hao Zheng 2012-05-21 20:23:07 PDT
(In reply to comment #2)
> (From update of attachment 143004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143004&action=review
> 
> > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:57
> > +    static const char* kDeviceFontDirForTest = "/data/drt/fonts/";
> 
> It seems strange to have DRT related code in the production target.  Is this common for this file in other ports?  I would have expected this code to be in DumpRenderTree rather than WebCore.

Yes, you are right. Normally, font setup code for layout test is in DumpRenderTree (TestShellLinux and TestShellWin). DRT on other platforms calls system api (fontconfig on Linux and windows api on Win) to make sure fonts used by Skia are configured properly before layout test starts.

However, there is no system api to customize font config on Android. Skia reads /system/etc/system_fonts.xml and /system/etc/fallback_fonts.xml directly to get font config (see FontHostConfiguration_android.cpp). So if we don't put code in WebCore, there are 2 options:
1. Change Skia on Andoird to be able to customize font config. Maybe pass a customized xml file to init Skia.
2. Replace 2 xml files used by Skia with a customized one in NRWT script. However, if NRWT fails to restore the 2 files, Android font config is broken  for all apps. So this is less desirable.

At last, we found it easier to put this code in FontCacheAndroird.cpp. The code is behind check PlatformSupport::layoutTestMode(), so we don't expect it a heavy perf burdon for production code.
Comment 7 Hao Zheng 2012-05-22 21:00:16 PDT
(In reply to comment #6)
> (In reply to comment #2)
> > (From update of attachment 143004 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143004&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:57
> > > +    static const char* kDeviceFontDirForTest = "/data/drt/fonts/";
> > 
> > It seems strange to have DRT related code in the production target.  Is this common for this file in other ports?  I would have expected this code to be in DumpRenderTree rather than WebCore.
> 
> Yes, you are right. Normally, font setup code for layout test is in DumpRenderTree (TestShellLinux and TestShellWin). DRT on other platforms calls system api (fontconfig on Linux and windows api on Win) to make sure fonts used by Skia are configured properly before layout test starts.
> 
> However, there is no system api to customize font config on Android. Skia reads /system/etc/system_fonts.xml and /system/etc/fallback_fonts.xml directly to get font config (see FontHostConfiguration_android.cpp). So if we don't put code in WebCore, there are 2 options:
> 1. Change Skia on Andoird to be able to customize font config. Maybe pass a customized xml file to init Skia.
> 2. Replace 2 xml files used by Skia with a customized one in NRWT script. However, if NRWT fails to restore the 2 files, Android font config is broken  for all apps. So this is less desirable.
> 
> At last, we found it easier to put this code in FontCacheAndroird.cpp. The code is behind check PlatformSupport::layoutTestMode(), so we don't expect it a heavy perf burdon for production code.

Adam, does this sound reasonable to you? If yes, I could address your other comments.
Comment 8 Adam Barth 2012-06-01 08:10:51 PDT
It's not correct to put this testing code in the production build target.  From what you've said, it sounds like the easiest path is to provide a way for DumpRenderTree to configure this font information in WebKit via an API.  Then we can hardcode this test data in DumpRenderTree rather than in WebCore.
Comment 9 Adam Barth 2012-06-24 01:33:15 PDT
This got fixed a better way in Bug 89721.

*** This bug has been marked as a duplicate of bug 89721 ***