Bug 89133 – [Chromium-Android] Initialize font rendering in DumpRenderTree

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Xianzhu Wang]

Chromium-android use chromium-linux as the baseline of layout test, so we need to configure it to let the layout test results match those of chromium-linux as much as possible.

In downstream, we initialize font rendering for Android in WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp for both DumpRenderTree and Chromium, which is not good.
We should initialize font rendering in DumpRenderTree code through WebKit API.

[Xianzhu Wang]

Created attachment 147664 [details]
patch

[Adam Barth]

Comment on attachment 147664 [details]
patch

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

> Source/WebKit/chromium/src/linuxish/WebFontRendering.cpp:74
> +#if OS(LINUX)
> +    WebFontInfo::setSubpixelPositioning(useSubpixelPositioning);
> +#endif

Is there non-test code that needs to be updated here as well?

[Xianzhu Wang]

Comment on attachment 147664 [details]
patch

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

>> Source/WebKit/chromium/src/linuxish/WebFontRendering.cpp:74
>> +#endif
> 
> Is there non-test code that needs to be updated here as well?

No except Chromium. I'll update the patch in http://codereview.chromium.org/10544103/ and request review.

[Tony Chang]

Comment on attachment 147664 [details]
patch

FWIW, the name linuxish is ambiguous to me.  Maybe we should make a fontconfig directory instead?

[Xianzhu Wang]

(In reply to comment #4)
> (From update of attachment 147664 [details])
> FWIW, the name linuxish is ambiguous to me.  Maybe we should make a fontconfig directory instead?

In Chromium's build/filename_rules.gyp, 'linuxish' is used to indicate directories or files used when 'OS=="android" or OS=="linux" or OS=="openbsd" or OS=="freebsd"'. The implementation of WebFontRendering depends on HarfBuzz and/or Freetype port of Skia (don't know much details of it). In WebKit the harfbuzz directory is included when 'use_x11=1 or OS=="android"' so it seems to equal to 'linuxish' in Chromium's term.

[James Robinson]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 147664 [details] [details])
> > FWIW, the name linuxish is ambiguous to me.  Maybe we should make a fontconfig directory instead?
> 
> In Chromium's build/filename_rules.gyp, 'linuxish' is used to indicate directories or files used when 'OS=="android" or OS=="linux" or OS=="openbsd" or OS=="freebsd"'. The implementation of WebFontRendering depends on HarfBuzz and/or Freetype port of Skia (don't know much details of it). In WebKit the harfbuzz directory is included when 'use_x11=1 or OS=="android"' so it seems to equal to 'linuxish' in Chromium's term.

Drop the "ish". Just "linux", it's simpler.

[Xianzhu Wang]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 147664 [details] [details] [details])
> > > FWIW, the name linuxish is ambiguous to me.  Maybe we should make a fontconfig directory instead?
> > 
> > In Chromium's build/filename_rules.gyp, 'linuxish' is used to indicate directories or files used when 'OS=="android" or OS=="linux" or OS=="openbsd" or OS=="freebsd"'. The implementation of WebFontRendering depends on HarfBuzz and/or Freetype port of Skia (don't know much details of it). In WebKit the harfbuzz directory is included when 'use_x11=1 or OS=="android"' so it seems to equal to 'linuxish' in Chromium's term.
> 
> Drop the "ish". Just "linux", it's simpler.

No, in Chromium's build/filename_rules.gyp, files containing "_linux" and directories named "linux", and it's not possible to include them back in particular gyp with normal 'sources/' under conditions. Tricky 'target_conditions' must be used to override the rules. I changed the directory to "linuxish" to meet the Chromium's file name rule.

[James Robinson]

There has to be a better solution than saying "linuxish".

[Xianzhu Wang]

cq- because of the concerns of naming. We should make a decision of it. I'll send a mail to chromium-dev for it.

[Adam Barth]

> There has to be a better solution than saying "linuxish".

IMHO, we should follow Chromium's conventions here for now.  If we'd like to improve on linuxish, we should probably start by talking with the folks who picked that name.  I suspect the choice might be more constrained that we might guess at first.

Sharing this code between Linux and Android is valuable.  We can move forward with this patch and debate project-wide naming issues in parallel.

[Xianzhu Wang]

Comment on attachment 147664 [details]
patch

cq+ now. I'd be glad to make change based on any new decision.

[WebKit Review Bot]

Comment on attachment 147664 [details]
patch

Clearing flags on attachment: 147664

Committed r120394: <http://trac.webkit.org/changeset/120394>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89162