Skip to content

[GTK][WPE] Follow-up to HyperlinkAuditingEnabled deprecation in 295103@main #45604

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
Merged
merged 1 commit into from
May 20, 2025
Merged

[GTK][WPE] Follow-up to HyperlinkAuditingEnabled deprecation in 295103@main #45604

merged 1 commit into from
May 20, 2025

Conversation

aperezdc
Copy link
Contributor

@aperezdc aperezdc commented May 19, 2025 •
edited by webkit-early-warning-system
Loading

8886943

[GTK][WPE] Follow-up to HyperlinkAuditingEnabled deprecation in 295103@main
https://bugs.webkit.org/show_bug.cgi?id=293245

Reviewed by Michael Catanzaro.

* Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:
(webkit_settings_get_enable_hyperlink_auditing): Make the returned value
consistent with the default property value (TRUE), and do not warn on
getter usage.
* Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:
(testWebKitSettings): Bring back the API test, to ensure that the setter
does not change the property after all.

Canonical link: https://commits.webkit.org/295155@main

4b355ab

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
✅ 🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aperezdc aperezdc requested a review from a team as a code owner May 19, 2025 20:17
@aperezdc aperezdc self-assigned this May 19, 2025
@aperezdc aperezdc added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label May 19, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 19, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 3ded44c)

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
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aperezdc aperezdc requested review from mcatanzaro and annevk May 19, 2025 20:17
annevk
annevk reviewed May 20, 2025
View reviewed changes
Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp
@@ -2154,11 +2154,9 @@ void webkit_settings_set_javascript_can_open_windows_automatically(WebKitSetting
*/
gboolean webkit_settings_get_enable_hyperlink_auditing(WebKitSettings* settings)
{
g_return_val_if_fail(WEBKIT_IS_SETTINGS(settings), FALSE);

g_warning("webkit_settings_get_enable_hyperlink_auditing is deprecated and always returns FALSE.");
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is not desired you should also remove it from webkit_settings_get_enable_dns_prefetching. Or is it still desired if it returns false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that we do not have a strongly agreed policy on this regard for the GTK/WPE ports, and that the idea has been to warn when trying to set the option to its non-default value. Not super important, because this is unlikely to spam the logs—typically applications are not all the time changing settings after all—but still nice to try and be consistent.

I would have commented on the patch that removed the DNS prefetching option, but I managed to miss it when it landed 😇. Maybe I'll send a follow up when I have some spare cycles for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about just deleting all of the warnings, because they're not very useful.

But I think they are OK as long as we don't warn when setting to default value, which happens during WebKitSettings construction as every one of these setters gets called.

Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp
g_assert_true(webkit_settings_get_enable_hyperlink_auditing(settings));
webkit_settings_set_enable_hyperlink_auditing(settings, FALSE);
g_assert_true(webkit_settings_get_enable_hyperlink_auditing(settings));
ALLOW_DEPRECATED_DECLARATIONS_END
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is desired you should probably also restore the test for webkit_settings_get_enable_dns_prefetching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #47405 🙃

@annevk
Copy link
Contributor

annevk commented May 20, 2025

Thanks!

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 20, 2025
@aperezdc aperezdc removed the merging-blocked Applied to prevent a change from being merged label May 20, 2025
@aperezdc aperezdc force-pushed the followup-to-295103 branch from 3ded44c to 4b355ab Compare May 20, 2025 11:00
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 20, 2025 •
edited
Loading

EWS run on current version of this PR (hash 4b355ab)

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
✅ 🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aperezdc aperezdc added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 20, 2025
@aperezdc
[GTK][WPE] Follow-up to HyperlinkAuditingEnabled deprecation in 29510…
8886943 
…3@main

https://bugs.webkit.org/show_bug.cgi?id=293245

Reviewed by Michael Catanzaro.

* Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:
(webkit_settings_get_enable_hyperlink_auditing): Make the returned value
consistent with the default property value (TRUE), and do not warn on
getter usage.
* Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:
(testWebKitSettings): Bring back the API test, to ensure that the setter
does not change the property after all.

Canonical link: https://commits.webkit.org/295155@main
@webkit-commit-queue
Copy link
Collaborator

Committed 295155@main (8886943): https://commits.webkit.org/295155@main

Reviewed commits have been landed. Closing PR #45604 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 8886943 into WebKit:main May 20, 2025
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 20, 2025
@aperezdc aperezdc deleted the followup-to-295103 branch May 20, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcatanzaro mcatanzaro mcatanzaro approved these changes

@annevk annevk annevk left review comments

Assignees

@aperezdc aperezdc

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

Successfully merging this pull request may close these issues.
6 participants
@aperezdc @webkit-early-warning-system @annevk @webkit-commit-queue @mcatanzaro @webkit-ews-buildbot