Bug 58321

Summary: Allow CG Font support in Chrome Darwin build using Skia
Product: WebKit Reporter: Cary Clark <caryclark>
Component: New BugsAssignee: Cary Clark <caryclark>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, commit-queue, dglazkov, eric, mark
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch none

Description Cary Clark 2011-04-12 07:47:51 PDT
Allow CG Font support in Chrome Darwin build using Skia
Comment 1 Cary Clark 2011-04-12 08:09:14 PDT
Created attachment 89204 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-12 10:51:38 PDT
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.
Comment 3 Eric Seidel (no email) 2011-04-12 10:53:28 PDT
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.
Comment 4 Cary Clark 2011-04-12 11:46:50 PDT
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.
Comment 5 Eric Seidel (no email) 2011-04-12 11:52:24 PDT
(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?
Comment 6 Eric Seidel (no email) 2011-04-12 11:53:56 PDT
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.
Comment 7 Cary Clark 2011-04-12 12:01:15 PDT
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.
Comment 8 Eric Seidel (no email) 2011-04-12 12:14:42 PDT
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?
Comment 9 Eric Seidel (no email) 2011-04-12 12:15:38 PDT
(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?
Comment 10 Eric Seidel (no email) 2011-04-12 13:05:48 PDT
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.
Comment 11 Cary Clark 2011-04-13 10:58:55 PDT
Created attachment 89403 [details]
Patch
Comment 12 Eric Seidel (no email) 2011-04-13 11:22:40 PDT
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.
Comment 13 WebKit Commit Bot 2011-04-13 19:13:49 PDT
Comment on attachment 89403 [details]
Patch

Clearing flags on attachment: 89403

Committed r83805: <http://trac.webkit.org/changeset/83805>
Comment 14 WebKit Commit Bot 2011-04-13 19:13:56 PDT
All reviewed patches have been landed.  Closing bug.