Bug 147426 - Don't use (Details) when exposing SPI
Summary: Don't use (Details) when exposing SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-29 16:32 PDT by Dean Jackson
Modified: 2015-07-30 14:23 PDT (History)
2 users (show)

See Also:


Attachments
Patch (61.49 KB, patch)
2015-07-29 16:42 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (61.42 KB, patch)
2015-07-29 17:13 PDT, Dean Jackson
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2015-07-29 16:32:48 PDT
If we are simply declaring the interface and not implementing, we should use class extensions rather than categories.
Comment 1 Radar WebKit Bug Importer 2015-07-29 16:34:34 PDT
<rdar://problem/22062407>
Comment 2 Dean Jackson 2015-07-29 16:42:22 PDT
Created attachment 257782 [details]
Patch
Comment 3 Dean Jackson 2015-07-29 17:13:47 PDT
Created attachment 257788 [details]
Patch
Comment 4 mitz 2015-07-29 17:53:44 PDT
Comment on attachment 257788 [details]
Patch

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

> Source/WebCore/platform/mac/ScrollViewMac.mm:43
> +@interface NSScrollView (Details)
>  @property BOOL automaticallyAdjustsContentInsets;
>  @end

Is this declared here for building on Mavericks? If so, can you guard this category with #if __MAC_OS_X_VERSION_MIN_REQUIRED < 101000 so that it’s easy to remove later? Maybe also give the category a name that begins with “Web”.

> Source/WebCore/platform/spi/cocoa/NSButtonCellSPI.h:30
> +#if PLATFORM(MAC) && USE(APPLE_INTERNAL_SDK)

We don’t need the PLATFORM(MAC) here. The fact that the #import <AppKit/NSButtonCell.h> above works proves that this header is used only on Mac, and the __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 is true only on Mac.
Comment 5 Dean Jackson 2015-07-30 14:17:39 PDT
Comment on attachment 257788 [details]
Patch

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

>> Source/WebCore/platform/mac/ScrollViewMac.mm:43
>>  @end
> 
> Is this declared here for building on Mavericks? If so, can you guard this category with #if __MAC_OS_X_VERSION_MIN_REQUIRED < 101000 so that it’s easy to remove later? Maybe also give the category a name that begins with “Web”.

You are right. I originally left it in because we can't redeclare it in an extension, but it is only needed for Mavericks.

>> Source/WebCore/platform/spi/cocoa/NSButtonCellSPI.h:30
>> +#if PLATFORM(MAC) && USE(APPLE_INTERNAL_SDK)
> 
> We don’t need the PLATFORM(MAC) here. The fact that the #import <AppKit/NSButtonCell.h> above works proves that this header is used only on Mac, and the __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 is true only on Mac.

Fixed.
Comment 6 Dean Jackson 2015-07-30 14:23:38 PDT
Committed r187609: <http://trac.webkit.org/changeset/187609>