Skip to content

More Cocoa HAVE directives cleanup #47367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

More Cocoa HAVE directives cleanup #47367

wants to merge 1 commit into from
+14 −119

Conversation

annevk
Copy link
Contributor

@annevk annevk commented Jun 30, 2025 •
edited by webkit-early-warning-system
Loading

f7ed280

More Cocoa HAVE directives cleanup
https://bugs.webkit.org/show_bug.cgi?id=295192

Reviewed by NOBODY (OOPS!).

These all appear to be leftovers from a time when they were targeting
specific OS versions.

* Source/WebCore/page/PageGroup.cpp:

PLATFORM(MAC) is redundant with HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK).

* Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.mm:

defined(CL_HAS_RADAR_88834301) has been around for long enough now that
we don't have to mention it explicitly anymore.

f7ed280

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk self-assigned this Jun 30, 2025
@annevk annevk added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jun 30, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 0f5fe81)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ✅ 🛠 wpe   🛠 win
✅ 🧪 bindings ❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug   🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ✅ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ❌ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ❌ 🛠 vision ❌ 🧪 mac-AS-debug-wk2   🧪 gtk-wk2
❌ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
⏳ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ✅ 🛠 playstation
❌ 🛠 tv ❌ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
❌ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 30, 2025
@annevk annevk removed the merging-blocked Applied to prevent a change from being merged label Jun 30, 2025
@annevk annevk force-pushed the eng/More-Cocoa-HAVE-directives-cleanup branch from 0f5fe81 to 9bb5443 Compare June 30, 2025 08:26
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 9bb5443)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk marked this pull request as ready for review June 30, 2025 09:06
@annevk annevk requested review from cdumez and rniwa as code owners June 30, 2025 09:06
pvollan
pvollan reviewed Jun 30, 2025
View reviewed changes
Source/WebCore/html/MediaElementSession.cpp Outdated
@@ -330,10 +330,6 @@ void MediaElementSession::isVisibleInViewportChanged()
RefPtr element = m_element.get();
if (!element || element->isFullscreen() || element->isVisibleInViewport())
m_elementIsHiddenUntilVisibleInViewport = false;

#if PLATFORM(COCOA) && !HAVE(CGS_FIX_FOR_RADAR_97530095)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be removed when HAVE(CGS_FIX_FOR_RADAR_97530095) is 0 on Cocoa platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops, thank you! I just went by the radar being long since fixed, but I did not mean to introduce a functional change. Let me take that out and perhaps follow-up on it separately at some point.

@annevk
More Cocoa HAVE directives cleanup
f7ed280 
https://bugs.webkit.org/show_bug.cgi?id=295192

Reviewed by NOBODY (OOPS!).

These all appear to be leftovers from a time when they were targeting
specific OS versions.

* Source/WebCore/page/PageGroup.cpp:

PLATFORM(MAC) is redundant with HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK).

* Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.mm:

defined(CL_HAS_RADAR_88834301) has been around for long enough now that
we don't have to mention it explicitly anymore.
@annevk annevk force-pushed the eng/More-Cocoa-HAVE-directives-cleanup branch from 9bb5443 to f7ed280 Compare July 1, 2025 07:31
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 1, 2025 •
edited
Loading

EWS run on current version of this PR (hash f7ed280)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk requested a review from pvollan July 1, 2025 07:31
pvollan
pvollan reviewed Jul 1, 2025
View reviewed changes
Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.mm
@@ -70,7 +70,7 @@ - (instancetype)initWithWebsiteIdentifier:(const String&)websiteIdentifier clien
_isWaitingForAuthorization = YES;
_mode = mode;

#if USE(APPLE_INTERNAL_SDK) && HAVE(CORE_LOCATION_WEBSITE_IDENTIFIERS) && defined(CL_HAS_RADAR_88834301)
Copy link
Contributor

@pvollan pvollan Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like CL_HAS_RADAR_88834301 is not defined. Can the code inside #If be removed, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought initially, but It's defined in the Cocoa framework we use that introduces this API. Maybe @cdumez should double check this though since he introduced this code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point! I did not realize it was defined there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvollan pvollan pvollan left review comments

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa

Assignees

@annevk annevk

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@annevk @webkit-early-warning-system @pvollan @webkit-ews-buildbot