Bug 214338 – Add some missing boilerplate to help fix build on macOS Big Sur

• Component: New Bugs
• Status: RESOLVED FIXED

[Saagar Jha]

Add some missing boilerplate to help fix build on macOS Big Sur

[Saagar Jha]

Created attachment 404316 [details]
Patch

[Darin Adler]

Comment on attachment 404316 [details]
Patch

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

This is OK, but not entirely correct.

> Source/WebCore/PAL/pal/spi/mac/NSAppearanceSPI.h:37
> +@property (nonatomic, setter=_setUsesMetricsAppearance:) BOOL _usesMetricsAppearance;

No way for you to know this, but this should be:

    - (BOOL)_usesMetricsAppearance;

Could declare it as a property, harmless but not quite right, and there is definitely no setter.

> Source/WebCore/PAL/pal/spi/mac/NSImageSPI.h:38
> ++ (instancetype)_imageWithSystemSymbolName:(NSString *)systemSymbolName;

No way for you to know this, but the return type should not be (instancetype). It should be like this instead:

    + (nullable NSImage *)_imageWithSystemSymbolName:(NSString *)name;

[Saagar Jha]

Yeah, I'm just guessing these from looking at AppKit's symbol table, so they might differ from the actual internal header. -[NSAppearance _setUsesMetricsAppearance:] does exist on my system and sets the same ivar that -[NSAppearance _usesMetricsAppearance] accesses, and it isn't packed into a bitfield like many IPI flags are, so I figured that its setter might be exposed as SPI. I'll fix that and the nullability annotation and put up another patch.

[Saagar Jha]

Created attachment 404356 [details]
Patch

[Alex Christensen]

Comment on attachment 404356 [details]
Patch

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

Needs a tweak, but otherwise looks good.

> Source/WebCore/PAL/pal/spi/mac/NSAppearanceSPI.h:37
> +- (BOOL)_usesMetricsAppearance;

This should only be declared in #if !USE(APPLE_INTERNAL_SDK), otherwise it will probably break an internal build because it's already declared.

[Darin Adler]

Comment on attachment 404356 [details]
Patch

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

>> Source/WebCore/PAL/pal/spi/mac/NSAppearanceSPI.h:37
>> +- (BOOL)_usesMetricsAppearance;
> 
> This should only be declared in #if !USE(APPLE_INTERNAL_SDK), otherwise it will probably break an internal build because it's already declared.

Oh, good point! How did I miss that?

[Saagar Jha]

I thought that this should work as long as it matches the previous instance, like other declarations? If not I'd be happy to wrap that in a #if !USE(APPLE_INTERNAL_SDK)–should I do that for the NSImage SPI as well?

(Also, is there a way to get webkit-patch upload to use staged changes for a patch? Until now I've been stashing all my work, reapplying it, deleting all of my additional changes that aren't in this patch, and then reapplying again after running the script to get back to how it was before…)

[Alex Christensen]

You've actually already done it for the NSImage SPI.

[Darin Adler]

(In reply to Saagar Jha from comment #7)
> I thought that this should work as long as it matches the previous instance,
> like other declarations?

Sadly, no, Objective-C class extensions don’t work that way.

> (Also, is there a way to get webkit-patch upload to use staged changes for a
> patch? Until now I've been stashing all my work, reapplying it, deleting all
> of my additional changes that aren't in this patch, and then reapplying
> again after running the script to get back to how it was before…)

I use separate branches for separate patches, not sure what this means in terms of the answer to your question.

[Saagar Jha]

Created attachment 404363 [details]
Patch

[Saagar Jha]

(In reply to Alex Christensen from comment #8)
> You've actually already done it for the NSImage SPI.

Ha, so I have. Added it to NSAppearance as well.

(In reply to Darin Adler from comment #9)
> Sadly, no, Objective-C class extensions don’t work that way.

Sad :(

 
> I use separate branches for separate patches, not sure what this means in
> terms of the answer to your question.

I have additional changes locally to that upstream probably doesn't want, like commenting out non-building code that I could't come up with fixes for. Usually I keep things like that permanently unstaged and upload only the changes that are relevant by staging and committing them. As webkit-patch doesn't seem to actually make commits I wasn't sure how to get it to understand the staging area–although if it just doesn't do this I can keep stashing as I was before. (I should note that I am using Git to work with the repository, not Subversion, so this might be a strange thing to try to do.)

[Alex Christensen]

Comment on attachment 404363 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/NSImageSPI.h:38
> ++ (nullable NSImage *)_imageWithSystemSymbolName:(NSString *)name;

It looks like you need to remove nullable here.  Adding a nullability specifier to an ObjC header makes it require nullability specifiers for all of them.

[Saagar Jha]

Created attachment 404372 [details]
Patch

[Alex Christensen]

Comment on attachment 404372 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/NSAppearanceSPI.h:36
>  - (void)_drawInRect:(NSRect)rect context:(CGContextRef)context options:(NSDictionary *)options;

This will break our internal build.  This needs to be here for APPLE_INTERNAL_SDK and not.  _usesMetricsAppearance only needs to be there for not APPLE_INTERNAL_SDK.

[Saagar Jha]

Created attachment 404374 [details]
Patch

[Saagar Jha]

Stopped trying to be clever; here's a new patch. Also I found out that webkit-upload has a -g argument and you can use that to just give it a commit to upload, so no need to stash anymore.

[EWS]

Committed r264428: <https://trac.webkit.org/changeset/264428>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404374 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65629661>

[Saagar Jha]

Thanks for your patience in reviewing this, Darin and Alex!