[webkit-reviews] review granted: [Bug 113754] Remove code for 10.5 and earlier from Source/WebCore : [Attachment 196095] Fixed builds

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 113754: Remove code for 10.5 and earlier from Source/WebCore
https://bugs.webkit.org/show_bug.cgi?id=113754

Attachment 196095: Fixed builds
https://bugs.webkit.org/attachment.cgi?id=196095&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=196095&action=review


I’m reviewing the mechanics of this patch, not the concept of whether it’s OK
to do it. I assume you already know it’s OK.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1472
>      UNUSED_PARAM(allowsFontSmoothing);

Please remove this line. It was only there to quiet warnings in older versions
of OS X.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:197
>      CGRect bound = CGRectZero;
> -#if !PLATFORM(MAC) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
>      bound = CGPathGetPathBoundingBox(m_path);

Please merge these into a single line. There is no reason to set CGRect to
CGRectZero and then overwrite it.

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:249
>  static bool canSetCascadeListForCustomFont()
>  {
> -#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
>      return true;
> -#else
> -    return false;
> -#endif
>  }

Please remove this function entirely, in a followup patch if not in this first
cut.

> Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:121
>      cgFontRef.adoptCF(CGFontCreateWithDataProvider(dataProvider.get()));

Please move the definition of cgFontRef down here now that there is no #if.

Also, modern style is to use the free function adoptCF, not the member
function, which we should deprecate. So this should say:

    RetainPtr<CGFontRef> cgFontRef =
adoptCF(CGFontCreateWithDataProvider(dataProvider.get()));

One advantage of using the free function is that you can cut down on local
variables. For example, we could write something like this:

    RetainPtr<CGFontRef> cgFontRef =
adoptCF(CGFontCreateWithDataProvider(adoptCF(CGDataProviderCreateWithCFData(ado
ptCF(buffer->createCFData()).get())).get());

That’s probably a little too long to be readable, but I like having that
option.

> Source/WebCore/platform/mac/CursorMac.mm:-259
> -	   m_platformCursor = createNamedCursor("contextMenuCursor", 3, 2);

We should also remove these cursor image files from the repository and the
project file. Either in this patch or in a followup.

> Source/WebCore/platform/mac/CursorMac.mm:-283
> -	   m_platformCursor = createNamedCursor("noDropCursor", 3, 1);

We should also remove these cursor image files from the repository and the
project file.

> Source/WebCore/platform/mac/CursorMac.mm:-291
> -	   m_platformCursor = createNamedCursor("copyCursor", 3, 2);

We should also remove these cursor image files from the repository and the
project file.

> Source/WebCore/platform/mac/CursorMac.mm:-303
> -	   m_platformCursor = createNamedCursor("notAllowedCursor", 11, 11);

We should also remove these cursor image files from the repository and the
project file.

> Source/WebCore/platform/mac/WebVideoFullscreenController.mm:-58
>  @interface WebVideoFullscreenWindow : NSWindow
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
>  <NSAnimationDelegate>
> -#endif

Please merge this into a single line.

> Source/WebCore/platform/mac/WebVideoFullscreenHUDWindowController.mm:49
>  @interface WebVideoFullscreenHUDWindowController (Private)
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
>  <NSWindowDelegate>

Please merge this into a single line.