Bug 214707 – Remove definition of NSImageSymbolScale, which is part of the beta 3 SDK

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Saagar Jha]

Remove definition of NSImageSymbolScale, which is part of the beta 3 SDK

[Jonathan Bedard]

I'll take a look at this tomorrow. Hadn't tried building with the Seed 3 SDK yet, so there may be more issues like this.

[Saagar Jha]

Sure. I have a patch ready (as the bug title might suggest) but webkit-patch is failing halfway through on my computer, so I got stuck with just the bug report :(

[Saagar Jha]

Created attachment 405109 [details]
Patch

[Jonathan Bedard]

My only question is do we care about keeping the Seed 1 build working. I think the answer is "no", but I also know we're about to bring up Seed 3 EWS builders, which isn't something we usually do.

[Alexey Proskuryakov]

We do not care about building against old seed SDKs.

[Alexey Proskuryakov]

Comment on attachment 405109 [details]
Patch

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

But also, we should probably add preprocessor checks in this file, as nothing here says that this is new with Big Sur.

Looks like using these is guarded by HAVE(ALTERNATE_ICONS).

> Source/WebCore/PAL/pal/spi/mac/NSImageSPI.h:39
>  extern const NSImageHintKey NSImageHintSymbolFont;
>  extern const NSImageHintKey NSImageHintSymbolScale;

Are these still not part of SDK, even though NSImageSymbolScale is?

> Source/WebCore/PAL/pal/spi/mac/NSImageSPI.h:42
>  - (void)lockFocusWithRect:(NSRect)rect context:(nullable NSGraphicsContext *)context hints:(nullable NSDictionary *)hints flipped:(BOOL)flipped;

Not seeing an underscore makes me wonder if this is SDK too.

[Saagar Jha]

I'm not seeing either in my SDK, it's just NSImageSymbolScale that seems to have been added. Did you want me to wrap the new stuff from Big Sur in HAVE(ALTERNATE_ICONS)?

[Alexey Proskuryakov]

I think that it does need to be wrapped, but also we need a better name for the macro. Not sure if it's best for you to just wrap declarations in this header, or for someone from Apple to make the bigger change.

[Jonathan Bedard]

Comment on attachment 405109 [details]
Patch

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

>> Source/WebCore/PAL/pal/spi/mac/NSImageSPI.h:39
>>  extern const NSImageHintKey NSImageHintSymbolScale;
> 
> Are these still not part of SDK, even though NSImageSymbolScale is?

Verified that these are not part of the SDK.

>> Source/WebCore/PAL/pal/spi/mac/NSImageSPI.h:42
>>  - (void)lockFocusWithRect:(NSRect)rect context:(nullable NSGraphicsContext *)context hints:(nullable NSDictionary *)hints flipped:(BOOL)flipped;
> 
> Not seeing an underscore makes me wonder if this is SDK too.

That actually pre-dates Big Sur. And yes, it's still required.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #8)
> I think that it does need to be wrapped, but also we need a better name for
> the macro. Not sure if it's best for you to just wrap declarations in this
> header, or for someone from Apple to make the bigger change.

We can do the wrapping, will open a different bug for it. But it doesn't seem like we actually need to.

[EWS]

Committed r264939: <https://trac.webkit.org/changeset/264939>

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

[Radar WebKit Bug Importer]

<rdar://problem/66178015>

[Alexey Proskuryakov]

> We can do the wrapping, will open a different bug for it. But it doesn't
> seem like we actually need to.

This is necessary for EWS to produce a compile time error if one attempts to use SPIs on an OS version that didn't have them.