Bug 214707

Summary: Remove definition of NSImageSymbolScale, which is part of the beta 3 SDK
Product: WebKit Reporter: Saagar Jha <saagar>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, jbedard, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=214844
Attachments:
Description Flags
Patch none

Description Saagar Jha 2020-07-23 15:45:05 PDT
Remove definition of NSImageSymbolScale, which is part of the beta 3 SDK
Comment 1 Jonathan Bedard 2020-07-23 16:28:57 PDT
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.
Comment 2 Saagar Jha 2020-07-23 16:30:33 PDT
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 :(
Comment 3 Saagar Jha 2020-07-23 18:03:27 PDT
Created attachment 405109 [details]
Patch
Comment 4 Jonathan Bedard 2020-07-24 07:27:21 PDT
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.
Comment 5 Alexey Proskuryakov 2020-07-24 09:30:38 PDT
We do not care about building against old seed SDKs.
Comment 6 Alexey Proskuryakov 2020-07-24 09:36:33 PDT
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.
Comment 7 Saagar Jha 2020-07-24 09:41:16 PDT
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)?
Comment 8 Alexey Proskuryakov 2020-07-24 09:48:18 PDT
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.
Comment 9 Jonathan Bedard 2020-07-27 12:57:14 PDT
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.
Comment 10 Jonathan Bedard 2020-07-27 13:03:50 PDT
(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.
Comment 11 EWS 2020-07-27 13:07:02 PDT
Committed r264939: <https://trac.webkit.org/changeset/264939>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405109 [details].
Comment 12 Radar WebKit Bug Importer 2020-07-27 13:07:26 PDT
<rdar://problem/66178015>
Comment 13 Alexey Proskuryakov 2020-07-27 13:11:38 PDT
> 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.