Bug 113754 - Remove code for 10.5 and earlier from Source/WebCore
Summary: Remove code for 10.5 and earlier from Source/WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-02 00:04 PDT by Ryosuke Niwa
Modified: 2013-04-04 23:03 PDT (History)
6 users (show)

See Also:


Attachments
Removed the code (48.59 KB, patch)
2013-04-02 00:05 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed builds (48.33 KB, patch)
2013-04-02 01:16 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Cleanup (56.25 KB, patch)
2013-04-02 21:25 PDT, Ryosuke Niwa
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-04-02 00:04:35 PDT
Remove code for 10.5 and earlier from Source/WebCore
Comment 1 Ryosuke Niwa 2013-04-02 00:05:40 PDT
Created attachment 196083 [details]
Removed the code
Comment 2 Early Warning System Bot 2013-04-02 00:14:55 PDT
Comment on attachment 196083 [details]
Removed the code

Attachment 196083 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17314680
Comment 3 Early Warning System Bot 2013-04-02 00:17:40 PDT
Comment on attachment 196083 [details]
Removed the code

Attachment 196083 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17392179
Comment 4 WebKit Review Bot 2013-04-02 00:41:39 PDT
Comment on attachment 196083 [details]
Removed the code

Attachment 196083 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17392187
Comment 5 EFL EWS Bot 2013-04-02 00:52:55 PDT
Comment on attachment 196083 [details]
Removed the code

Attachment 196083 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17250987
Comment 6 Ryosuke Niwa 2013-04-02 01:16:37 PDT
Created attachment 196095 [details]
Fixed builds
Comment 7 Darin Adler 2013-04-02 09:41:37 PDT
Comment on attachment 196095 [details]
Fixed builds

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(adoptCF(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.
Comment 8 Ryosuke Niwa 2013-04-02 11:27:21 PDT
I think I messed up this patch for Windows and iOS. Let me fix that and reupload it.
Comment 9 Ryosuke Niwa 2013-04-02 21:25:05 PDT
Created attachment 196275 [details]
Cleanup
Comment 10 Benjamin Poulain 2013-04-04 20:35:55 PDT
Comment on attachment 196275 [details]
Cleanup

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

Looks great!

> Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:119
> -    cgFontRef.adoptCF(CGFontCreateWithDataProvider(dataProvider.get()));
> +    RetainPtr<CGFontRef> cgFontRef = adoptCF(CGFontCreateWithDataProvider(dataProvider.get()));

RetainPtr<CGFontRef> cgFontRef(AdoptCF, CGFontCreateWithDataProvider(dataProvider.get())); ?

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1009
>          initialSize.width = naturalSize.width;
>          initialSize.height = naturalSize.height;
>      }

Not sure why this is not
    initialSize = naturalSize;

But that's unrelated :)

> Source/WebCore/platform/mac/WebVideoFullscreenHUDWindowController.mm:372
> +

I would remove that blank line. The object is initialized in the following lines.
Comment 11 Ryosuke Niwa 2013-04-04 23:03:31 PDT
Committed r147710: <http://trac.webkit.org/changeset/147710>