Bug 40275 – [BREWMP] Port graphics backend

• Component: Platform
• Status: RESOLVED WONTFIX

[Kwang Yul Seo]

Use skia as Brew MP's graphics backend.

[Kwang Yul Seo]

Created attachment 65777 [details]
FontCustomPlatformData

Reuse chromium's FontCustomPlatformData.

[Kwang Yul Seo]

CC'ing David Levin as it touches chromium.

[Kwang Yul Seo]

Comment on attachment 65777 [details]
FontCustomPlatformData

I will submit patches for WebKit Brew MP after bug 39672 is resolved.

[Kwang Yul Seo]

Created attachment 67622 [details]
FontCustomPlatformData

Add PLATFORM(BREWMP) guard to reuse the OS(LINUX) code.

[Kwang Yul Seo]

CC'ing James here.

[James Robinson]

Since Brew MP has no builders on build.webkit.org, what's the plan if changes to this file break that port?

Also, what OS setting does Brew MP set?  I don't know anything about it as a platform.  Maybe it's simpler for Brew to just set OS(LINUX)?  It's kind of weird to mix up OS() and PLATFORM() #ifdefs in a single expression.

[Kwang Yul Seo]

(In reply to comment #6)
> Since Brew MP has no builders on build.webkit.org, what's the plan if changes to this file break that port?

Brew MP uses waf. I am upstreaming the build system in bug 44645. Until we have a buildbot, I will fix the build manually :(.

> Also, what OS setting does Brew MP set?  I don't know anything about it as a platform.  Maybe it's simpler for Brew to just set OS(LINUX)?  It's kind of weird to mix up OS() and PLATFORM() #ifdefs in a single expression.

A Brew MP application is written only with Brew MP API and C standard functions. Underlying OS is not exposed, so we don't know the operating system where Brew MP runs. Sadly, there is no way but to add PLATFORM(BREWMP) guard.

[Kwang Yul Seo]

Created attachment 68144 [details]
Font

Port Font to Brew MP.

[Kwang Yul Seo]

Created attachment 68149 [details]
Font

Fix COMPILE_ASSERT.

[Kwang Yul Seo]

(In reply to comment #6)
> Since Brew MP has no builders on build.webkit.org, what's the plan if changes to this file break that port?
> 
> Also, what OS setting does Brew MP set?  I don't know anything about it as a platform.  Maybe it's simpler for Brew to just set OS(LINUX)?  It's kind of weird to mix up OS() and PLATFORM() #ifdefs in a single expression.

Another way to clean up the code is to split OS(WINDOWS) and OS(LINUX) into separate files and move OS(WINDOWS) part to platform/graphics/chromium and leave OS(LINUX) part in platform/graphics/skia without guards. 

I think OS(WINDOWS) part of FontCustomPlatformData.cpp/h is still Chromium-specific while OS(LINUX) part depends only on skia.

After this cleanup, Brew MP can use FontCustomPlatformData in platform/graphics/skia without modification.

[Kwang Yul Seo]

Created attachment 71186 [details]
FontCustomPlatformData

Update the patch because OS(FREEBSD) guard was added.

[Kwang Yul Seo]

Created attachment 71219 [details]
FontPlatformData

[Kwang Yul Seo]

Created attachment 71232 [details]
FontCache

[Kwang Yul Seo]

(In reply to comment #13)
> Created an attachment (id=71232) [details]
> FontCache

Except for FontCache::getFontDataForCharacters, all other methods are still the same as Chromium Linux.  If you think it is better to share the code with PLATFORM(CHROMIUM) guards in FontCache::getFontDataForCharacters, I will submit an alternative patch in bug 39672.

[James Robinson]

Comment on attachment 71186 [details]
FontCustomPlatformData

It seems like these files only handle two cases - windows (in which case they use GDI) and everything else (originally just linux, but later freebsd and now brewmp).  Would it be useful to define another macro for the using-Skia-but-not-GDI case so we can stick the OS(LINUX) || OS(FREEBSD) || PLATFORM(BREWMP) logic all in one place?

Kwang, do you think such a macro would be useful in other files in addition to this pair?  If so we should consider adding it, but if it's just for these files then I think this way is fine.

R=me, request commit-queue again if you want to land with these macros.

[Kwang Yul Seo]

(In reply to comment #15)
> (From update of attachment 71186 [details])
> It seems like these files only handle two cases - windows (in which case they use GDI) and everything else (originally just linux, but later freebsd and now brewmp).  Would it be useful to define another macro for the using-Skia-but-not-GDI case so we can stick the OS(LINUX) || OS(FREEBSD) || PLATFORM(BREWMP) logic all in one place?
> 
> Kwang, do you think such a macro would be useful in other files in addition to this pair?  If so we should consider adding it, but if it's just for these files then I think this way is fine.
> 
> R=me, request commit-queue again if you want to land with these macros.

Currently, only there files have OS(LINUX) || OS(FREEBSD) || PLATFORM(BREWMP) check.

[James Robinson]

Comment on attachment 71186 [details]
FontCustomPlatformData

OK!

[WebKit Commit Bot]

Comment on attachment 71186 [details]
FontCustomPlatformData

Clearing flags on attachment: 71186

Committed r70675: <http://trac.webkit.org/changeset/70675>

[Holger Freyther]

Comment on attachment 68149 [details]
Font

Is there anything that is specific to BREW? If not you should really share the implementation.

[Holger Freyther]

Comment on attachment 68149 [details]
Font

Is there anything that is specific to BREW? If not you should really share the implementation.

[Holger Freyther]

Comment on attachment 68149 [details]
Font

Is there anything that is specific to BREW? If not you should really share the implementation.

[Dirk Schulze]

Comment on attachment 68149 [details]
Font

I agree to Holger. Looks like you should share the code than copying it.

[Dirk Schulze]

Comment on attachment 71219 [details]
FontPlatformData

ditto.

[Dirk Schulze]

Comment on attachment 71232 [details]
FontCache

ditto.

[Kwang Yul Seo]

Brew MP port is no longer maintained.