Bug 58321 – Allow CG Font support in Chrome Darwin build using Skia

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Cary Clark]

Allow CG Font support in Chrome Darwin build using Skia

[Cary Clark]

Created attachment 89204 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 89204 [details]
Patch

This is the wrong way to do this.  The right way is to make CG into a USE macro instead and define it for the SKIA/Darwin case.

[Eric Seidel (no email)]

You'd need to split this into two bugs.  One which just renames all the places where we have PLATFORM(CG) into USE(CG).  And then a second, which turns on CG in whatever case you want it on.

You should also talk with some of the Apple folks, since they're they main CG users.

I assume that the Windows build in WebKIt already does somethign similar with CAIRO.

[Cary Clark]

Eric: The desire is not to define USE(CG) in the Skia/Darwin case. The desire is to make a handful of convenience routines (really only two: operator CGRect() in IntRect and FloatRect) available in a port which does not use CG as the rendering engine.

I don't see how changing PLATFORM(CG) to USE(CG) will enable this, although I have no problem if this change is desirable for other reasons.

[Eric Seidel (no email)]

(In reply to comment #4)
> Eric: The desire is not to define USE(CG) in the Skia/Darwin case. The desire is to make a handful of convenience routines (really only two: operator CGRect() in IntRect and FloatRect) available in a port which does not use CG as the rendering engine.

Ok, then I"m clearly misunderstanding your patch.  I got scared by the obsenely long #if conditions. :)

I guess I'm still confused as to what your patch is doing.  It's affecting synthetic bold behavior as well, no?

[Eric Seidel (no email)]

Why would you want CGRect conversions if the port doesn't use CG?  I assume that Skia uses these types when on Darwin?  Seems we should consider splitting these things out into a separate define?

USE(CG_TYPES)?  I'm not sure I understand well enough to really comment yet.

[Cary Clark]

This patch is moving Chrome on Mac to use Skia for the WebKit rendering engine. It continues to use CoreGraphics to draw UI elements like buttons and scrollbars. So while WebCore rendering uses Skia data types, some UI platform implementation classes use CG types.

[Eric Seidel (no email)]

Comment on attachment 89204 [details]
Patch

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

So does this compile?  Since you're not a committer yet, the cr-mac-ews is ignoring your patch (hence why its bubble is purple).

> Source/WebCore/platform/graphics/GlyphBuffer.h:37
> +#if PLATFORM(CG) || (PLATFORM(WX) && OS(DARWIN)) || (USE(SKIA) && PLATFORM(CHROMIUM) && OS(DARWIN))

This same #if block is repeated 7 times.  It seems it should be its own #define somehow.  It looks like it's enabling synthetic bold, but maybe it's doing other things? Can we give it a good name?

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 89204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89204&action=review
> 
> So does this compile?  Since you're not a committer yet, the cr-mac-ews is ignoring your patch (hence why its bubble is purple).

My confusion stems from the fact that this change doesn't include any change to what feature defines the cr-mac build uses.  Should it?

[Eric Seidel (no email)]

Comment on attachment 89204 [details]
Patch

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

> Source/WebCore/loader/cache/CachedFont.cpp:30
> +#if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (OS(WINDOWS) || OS(LINUX) || OS(FREEBSD) || (USE(SKIA) && OS(DARWIN)))) || PLATFORM(HAIKU) || OS(WINCE) || PLATFORM(ANDROID) || PLATFORM(BREWMP)

This should really be a blacklist instead of a whitelist.  You don't have to fix that, but adding a FIXME saying so would be nice. :)

> Source/WebCore/platform/graphics/FontPlatformData.h:87
> +#if PLATFORM(CG) || (USE(SKIA) && PLATFORM(CHROMIUM) && OS(DARWIN))

I think we can come up with a nice name for your define here.  Something like:
USE(SKIA_MAC) maybe?

You'd then define WTF_USE_SKIA_MAC 1 in platform.h

There is precedent for such things.  Basically you're making some exceptions to the general "exclude all CG code on non-CG builds" rule.

> Source/WebCore/platform/graphics/FontPlatformData.h:182
> +#if PLATFORM(CG) || (USE(SKIA) && PLATFORM(CHROMIUM))

Would use your fancy new USE.  CG should not be a PLATFORM.  You could pay the debt of turning it to a USE, but that's not necessarily related to your bug here.

[Cary Clark]

Created attachment 89403 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 89403 [details]
Patch

LGTM.  I think technically we should define a default value for WTF_USE_SKIA_ON_MAC_CHROME in platform.h, but I also think it doesn't matter.

[WebKit Commit Bot]

Comment on attachment 89403 [details]
Patch

Clearing flags on attachment: 89403

Committed r83805: <http://trac.webkit.org/changeset/83805>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.