Bug 88802 - [Chromium] Let Android and Linux share WebFontRendering decl/impl
Summary: [Chromium] Let Android and Linux share WebFontRendering decl/impl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on: 89021
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-06-11 13:47 PDT by Xianzhu Wang
Modified: 2012-06-13 20:28 PDT (History)
8 users (show)

See Also:


Attachments
patch (71.67 KB, patch)
2012-06-11 15:32 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v2 (try to fix patch issue) (90.66 KB, patch)
2012-06-11 16:31 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v3 (remove changes after move which break patch on bot) (87.15 KB, patch)
2012-06-11 16:49 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
a much smaller patch (14.13 KB, patch)
2012-06-12 17:13 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch for re-applying with some changes in WebKit.gyp reverted (13.47 KB, patch)
2012-06-13 11:19 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2012-06-11 13:47:00 PDT
Some Linux-specific APIs under the following directories:

Source/WebKit/chromium/public/linux
Source/WebKit/chromium/public/platform/linux
Source/Platform/chromium/public/linux

can be shared with Android.

At least we need WebFontRendering to set some font rendering configurations from chromium on Android.
Other interfaces like WebFontRenderStyle, WebSandboxSupport, WebThemeEngine, WebFontInfo, etc. can also be shared.

We have some Android-specific changes in WebKit/WebCore for font configurations, but with the above APIs, we can move those changes into Chromium.

For example, in FontPlatformDataHarfBuzz.cpp:

#if OS(ANDROID)
// Slight hinting renders much better than normal hinting on Android.
static SkPaint::Hinting skiaHinting = SkPaint::kSlight_Hinting;
#else
static SkPaint::Hinting skiaHinting = SkPaint::kNormal_Hinting;
#endif

the change can be removed if we can use Source/WebKit/chromium/public/linux/WebFontRendering.h and Source/WebKit/chromium/src/linux/WebFontRendering.cpp on Android.

*/linux/* files are excluded on Android. Adding them back in gyp needs a tricky 'target_conditions' section.

According to the rules defined in Chromium's build/filename_rules.gypi, we should name the directories 'linuxish' if shared between Android and Linux.
Comment 1 Xianzhu Wang 2012-06-11 15:32:23 PDT
Created attachment 146930 [details]
patch

Wondering if this is the correct way.
Comment 2 Xianzhu Wang 2012-06-11 15:36:06 PDT
Comment on attachment 146930 [details]
patch

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

Wondering if the commit queue could correctly handle the change:
A renamed to B
another A created

> Source/WebKit/chromium/WebKit.gyp:-781
> -                        ['exclude', '/linux/'],

The rules have been in Chromium's build/filename_rules.gypi.
Comment 3 Xianzhu Wang 2012-06-11 16:31:32 PDT
Created attachment 146958 [details]
patch v2 (try to fix patch issue)
Comment 4 Xianzhu Wang 2012-06-11 16:49:18 PDT
Created attachment 146964 [details]
patch v3 (remove changes after move which break patch on bot)
Comment 5 WebKit Review Bot 2012-06-11 16:55:56 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 6 Xianzhu Wang 2012-06-12 16:38:25 PDT
Will make a smaller change.
Comment 7 Xianzhu Wang 2012-06-12 17:13:29 PDT
Created attachment 147197 [details]
a much smaller patch
Comment 8 Adam Barth 2012-06-12 19:46:59 PDT
Comment on attachment 147197 [details]
a much smaller patch

Ok.  I'm not 100% confident in all of the changes you're making to the GYP file, but the compiler should tell us if we've goofed up there.
Comment 9 Xianzhu Wang 2012-06-13 09:53:58 PDT
Comment on attachment 147197 [details]
a much smaller patch

Thanks Adam. I've verified the patch on Chromium's try bots.
Comment 10 WebKit Review Bot 2012-06-13 10:10:12 PDT
Comment on attachment 147197 [details]
a much smaller patch

Clearing flags on attachment: 147197

Committed r120220: <http://trac.webkit.org/changeset/120220>
Comment 11 WebKit Review Bot 2012-06-13 10:10:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2012-06-13 10:52:46 PDT
Re-opened since this is blocked by 89021
Comment 13 Xianzhu Wang 2012-06-13 11:19:09 PDT
Created attachment 147367 [details]
patch for re-applying with some changes in WebKit.gyp reverted
Comment 14 WebKit Review Bot 2012-06-13 20:28:30 PDT
Comment on attachment 147367 [details]
patch for re-applying with some changes in WebKit.gyp reverted

Clearing flags on attachment: 147367

Committed r120270: <http://trac.webkit.org/changeset/120270>
Comment 15 WebKit Review Bot 2012-06-13 20:28:35 PDT
All reviewed patches have been landed.  Closing bug.