Bug 192427 – Stop defining CGFloat in WebKit

• Status: ASSIGNED
• Component: WebKit Misc.

[Alexey Proskuryakov]

The definitions are in somewhat strange places. But hopefully, they are not needed at all any more.

Added in r22681, which claimed that it was removing CG header includes from open source, but was in fact adding those.

Let's see what EWS says.

[Alexey Proskuryakov]

Created attachment 356656 [details]
proposed patch

[Alex Christensen]

Comment on attachment 356656 [details]
proposed patch

r=me if this doesn't break anything.

[Alexey Proskuryakov]

Comment on attachment 356656 [details]
proposed patch

Well, it does break the build.

[Alexey Proskuryakov]

Created attachment 356747 [details]
...and include CoreGraphics.h

Let's include CoreGraphics.h instead. Having it in the prefix file may even make the build faster (and if it gets slower, we'll roll back).

[Tim Horton]

Comment on attachment 356747 [details]
...and include CoreGraphics.h

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

> Source/WebCore/WebCorePrefix.h:90
> +#include <CoreGraphics/CoreGraphics.h>

Why did you remove all the CG headers?

"The project should be able to build without this header, although we rarely test that."

> Source/WebKitLegacy/mac/ChangeLog:18
> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        Stop defining CGFloat in WebKit
> +        https://bugs.webkit.org/show_bug.cgi?id=192427
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebKitPrefix.h:

Double changelog

[Simon Fraser (smfr)]

Comment on attachment 356747 [details]
...and include CoreGraphics.h

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

>> Source/WebCore/WebCorePrefix.h:90
>> +#include <CoreGraphics/CoreGraphics.h>
> 
> Why did you remove all the CG headers?
> 
> "The project should be able to build without this header, although we rarely test that."

Should we just include CGGeometry.h instead?

[Alexey Proskuryakov]

> "The project should be able to build without this header, although we rarely test that."

I didn't notice this comment, but it looks like a lie to me. There are lots of things that seem like they are required for Mac build in this header. And I don't know why that would be undesirable.

Windows did fail to build, so maybe it's actually building without the prefix. I'm thinking that I should fix WebCoreTestSupport to use the prefix.

> Should we just include CGGeometry.h instead?

I'm hoping that having all of CoreGraphics in precompiled header will make our builds faster.

[Darin Adler]

Comment on attachment 356747 [details]
...and include CoreGraphics.h

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

>>> Source/WebCore/WebCorePrefix.h:90
>>> +#include <CoreGraphics/CoreGraphics.h>
>> 
>> Why did you remove all the CG headers?
>> 
>> "The project should be able to build without this header, although we rarely test that."
> 
> Should we just include CGGeometry.h instead?

As discussed in webkit-dev, if we add CoreGraphics.h to both this and to "config.h" then I think it’s legit to remove all those includes elsewhere. And I think the decision should probably be based on compile speed.

[Alexey Proskuryakov]

Agreed that it would be useful to test the build speed theory before moving forward. Posted a patch that just adds the include to WebCorePrefix in bug 192557.

[Alexey Proskuryakov]

Adding CoreGraphics to WebCorePrefix didn't change build speed on Mac at all.