Allow CG Font support in Chrome Darwin build using Skia
Created attachment 89204 [details] Patch
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.
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.
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.
(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?
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.
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 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?
(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 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.
Created attachment 89403 [details] Patch
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 on attachment 89403 [details] Patch Clearing flags on attachment: 89403 Committed r83805: <http://trac.webkit.org/changeset/83805>
All reviewed patches have been landed. Closing bug.