Bug 192427 - Stop defining CGFloat in WebKit
Summary: Stop defining CGFloat in WebKit
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-05 14:14 PST by Alexey Proskuryakov
Modified: 2018-12-12 15:12 PST (History)
4 users (show)

See Also:


Attachments
proposed patch (2.15 KB, patch)
2018-12-05 14:15 PST, Alexey Proskuryakov
ap: review-
Details | Formatted Diff | Diff
...and include CoreGraphics.h (19.75 KB, patch)
2018-12-06 13:30 PST, Alexey Proskuryakov
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2018-12-05 14:14:41 PST
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.
Comment 1 Alexey Proskuryakov 2018-12-05 14:15:53 PST
Created attachment 356656 [details]
proposed patch
Comment 2 Alex Christensen 2018-12-05 14:16:55 PST
Comment on attachment 356656 [details]
proposed patch

r=me if this doesn't break anything.
Comment 3 Alexey Proskuryakov 2018-12-05 14:50:23 PST
Comment on attachment 356656 [details]
proposed patch

Well, it does break the build.
Comment 4 Alexey Proskuryakov 2018-12-06 13:30:06 PST
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).
Comment 5 Tim Horton 2018-12-06 13:52:41 PST
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
> +2018-12-05  Alexey Proskuryakov  <ap@apple.com>
> +
> +        Stop defining CGFloat in WebKit
> +        https://bugs.webkit.org/show_bug.cgi?id=192427
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebKitPrefix.h:

Double changelog
Comment 6 Simon Fraser (smfr) 2018-12-06 16:26:11 PST
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?
Comment 7 Alexey Proskuryakov 2018-12-06 21:11:40 PST
> "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.
Comment 8 Darin Adler 2018-12-08 19:51:30 PST
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.
Comment 9 Alexey Proskuryakov 2018-12-10 10:02:20 PST
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.
Comment 10 Alexey Proskuryakov 2018-12-12 15:12:50 PST
Adding CoreGraphics to WebCorePrefix didn't change build speed on Mac at all.