Add some missing boilerplate to help fix build on macOS Big Sur
Created attachment 404316 [details] Patch
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;
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.
Created attachment 404356 [details] Patch
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.
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?
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…)
You've actually already done it for the NSImage SPI.
(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.
Created attachment 404363 [details] Patch
(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.)
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.
Created attachment 404372 [details] Patch
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.
Created attachment 404374 [details] Patch
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.
Committed r264428: <https://trac.webkit.org/changeset/264428> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404374 [details].
<rdar://problem/65629661>
Thanks for your patience in reviewing this, Darin and Alex!