Created attachment 143004 [details] Patch

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 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 on attachment 143004 [details] Patch Seems reasonable.

Comment on attachment 143004 [details] Patch OK. NM. Adam has convinced me.

(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.

(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.

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.

This got fixed a better way in Bug 89721. *** This bug has been marked as a duplicate of bug 89721 ***