Bug 138633 – Move FontMac and FontCacheMac off of WKSI

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Move FontMac and FontCacheMac off of WKSI

[Myles C. Maxfield]

Created attachment 241386 [details]
Patch

[Myles C. Maxfield]

Created attachment 241392 [details]
Patch

[Myles C. Maxfield]

Created attachment 241396 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 241396 [details]
Patch

Please fix windows.

[Myles C. Maxfield]

Created attachment 241406 [details]
Patch

[Myles C. Maxfield]

Created attachment 241432 [details]
Patch

[Myles C. Maxfield]

Created attachment 241472 [details]
Patch

[Myles C. Maxfield]

Created attachment 241473 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 241473 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17544
>  				9348428119F1A9190009D5AE /* NSViewSPI.h */,
> +				1C6466271A12C3F90094603C /* NSFontSPI.h */,

This doesn't look sorted. Please run sort-Xcode-project-file

> Source/WebCore/platform/graphics/mac/FontMac.mm:183
> +static inline bool isIntegral(float value)
> +{
> +    return (int)value == value;
> +}

We should move this to MathUtils.h

[Daniel Bates]

Comment on attachment 241473 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        * WebCore.vcxproj/WebCoreCG.props:

We should also consider updating Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters with the SPI directory and SPI files. See the changes I made in file Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters in the patch for bug #138709 (attachment #241514 [details]).

[Daniel Bates]

(In reply to comment #9)
> > Source/WebCore/platform/graphics/mac/FontMac.mm:183
> > +static inline bool isIntegral(float value)
> > +{
> > +    return (int)value == value;
> > +}
> 
> We should move this to MathUtils.h

We should also look to make use of this function in GraphicsLayerCA.cpp instead of defining it again: <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp?rev=175794#L85> (*).

Also, I suggest we use a C++ style cast instead of a C-style cast as seen in (*) to be more consistent with the cast style we use in C++/Objective-C++ files.

[Myles C. Maxfield]

Comment on attachment 241473 [details]
Patch

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17544
>> +				1C6466271A12C3F90094603C /* NSFontSPI.h */,
> 
> This doesn't look sorted. Please run sort-Xcode-project-file

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:183
>> +}
> 
> We should move this to MathUtils.h

Done.

[Myles C. Maxfield]

(In reply to comment #10)
> Comment on attachment 241473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241473&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        * WebCore.vcxproj/WebCoreCG.props:
> 
> We should also consider updating
> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters with the SPI
> directory and SPI files. See the changes I made in file
> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters in the patch for bug
> #138709 (attachment #241514 [details]).

Done.

[Myles C. Maxfield]

(In reply to comment #11)
> (In reply to comment #9)
> > > Source/WebCore/platform/graphics/mac/FontMac.mm:183
> > > +static inline bool isIntegral(float value)
> > > +{
> > > +    return (int)value == value;
> > > +}
> > 
> > We should move this to MathUtils.h
> 
> We should also look to make use of this function in GraphicsLayerCA.cpp
> instead of defining it again:
> <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ca/
> GraphicsLayerCA.cpp?rev=175794#L85> (*).
> 
> Also, I suggest we use a C++ style cast instead of a C-style cast as seen in
> (*) to be more consistent with the cast style we use in C++/Objective-C++
> files.

Done.

[Daniel Bates]

Comment on attachment 241473 [details]
Patch

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

> Source/WebCore/platform/spi/cg/CoreGraphicsSPI.h:35
> +#include <CoreGraphics/CGContextGState.h>
> +#include <CoreGraphics/CGFontCache.h>
> +#include <CoreGraphics/CGFontPrivate.h>
> +#include <CoreGraphics/CGFontUnicodeSupport.h>

Nit: These includes are unnecessary as CoreGraphicsPrivate.h ultimately includes them.

[Daniel Bates]

(In reply to comment #15)
> Comment on attachment 241473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241473&action=review
> 
> > Source/WebCore/platform/spi/cg/CoreGraphicsSPI.h:35
> > +#include <CoreGraphics/CGContextGState.h>
> > +#include <CoreGraphics/CGFontCache.h>
> > +#include <CoreGraphics/CGFontPrivate.h>
> > +#include <CoreGraphics/CGFontUnicodeSupport.h>
> 
> Nit: These includes are unnecessary as CoreGraphicsPrivate.h ultimately
> includes them.

Err, I meant that the includes <CoreGraphics/CGContextGState.h>, <CoreGraphics/CGFontPrivate.h> and <CoreGraphics/CGFontUnicodeSupport.h> are not necessary. That is, it should be sufficient to include headers <CoreGraphics/CGFontCache.h> and <CoreGraphics/CoreGraphicsPrivate.h>.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/176112

[Myles C. Maxfield]

(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 241473 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=241473&action=review
> > 
> > > Source/WebCore/platform/spi/cg/CoreGraphicsSPI.h:35
> > > +#include <CoreGraphics/CGContextGState.h>
> > > +#include <CoreGraphics/CGFontCache.h>
> > > +#include <CoreGraphics/CGFontPrivate.h>
> > > +#include <CoreGraphics/CGFontUnicodeSupport.h>
> > 
> > Nit: These includes are unnecessary as CoreGraphicsPrivate.h ultimately
> > includes them.
> 
> Err, I meant that the includes <CoreGraphics/CGContextGState.h>,
> <CoreGraphics/CGFontPrivate.h> and <CoreGraphics/CGFontUnicodeSupport.h> are
> not necessary. That is, it should be sufficient to include headers
> <CoreGraphics/CGFontCache.h> and <CoreGraphics/CoreGraphicsPrivate.h>.

Addressed in https://trac.webkit.org/r176114