Skip to content

[WTF][WPE] Read the log level string from an Android system property #47352

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

[WTF][WPE] Read the log level string from an Android system property #47352

wants to merge 1 commit into from
+24 −1

Conversation

aperezdc
Copy link
Contributor

@aperezdc aperezdc commented Jun 29, 2025 •
edited by webkit-early-warning-system
Loading

6adbad7

[WTF][WPE] Read the log level string from an Android system property
https://bugs.webkit.org/show_bug.cgi?id=295175

Reviewed by NOBODY (OOPS!).

On Android, if the WEBKIT_DEBUG environment variable is undefined, try
the debug.WPEWebKit.log system property as well to determine the value
returned by WTF::logLevelString(), which is used to configure logging
channels.

This allows using the "setprop" command line tool to configure logging:

  adb shell setprop debug.WPEWebKit.log 'Scrolling,Loading'

6adbad7

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 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

@aperezdc aperezdc requested a review from a team as a code owner June 29, 2025 12:37
@aperezdc aperezdc self-assigned this Jun 29, 2025
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash f292bc1)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe loading 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 loading 🧪 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

@aperezdc aperezdc added the WPE WebKit WebKit WPE component label Jun 29, 2025
@aperezdc aperezdc requested a review from spenap June 29, 2025 12:38
@aperezdc
Copy link
Contributor Author

This is most useful in combination with #47339 in order to have the logging messages sent to the Android logging service.

@aperezdc aperezdc mentioned this pull request Jun 29, 2025
Open
philn
philn reviewed Jun 29, 2025
View reviewed changes
Source/WTF/wtf/unix/LoggingUnix.cpp Outdated

static const char* androidLogLevelString()
{
const auto* propertyInfo = __system_property_find("debug.wpewebkit.log");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove wpe here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with both approaches.

  • debug.webkit.log is consistent with WEBKIT_DEBUG as we normally use it,
  • debug.wpewebkit.log is possibly fairer considering we're only having active Android efforts for the WPEWebKit port

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 reason to use debug.wpewebkit.log here was that people have been exploring how to run WebKitGTK on Android (no success yet), so at some point we may want to have also debug.webkitgtk.log as well—we may want to allow configuring logging separately for each port, in case the GTK port is eventually made to work on Android.

Thinking about that, probably the best approach could be using something like this:

Suggested change
const auto* propertyInfo = __system_property_find("debug.wpewebkit.log");
const auto* propertyInfo =
__system_property_find("debug." LOG_CHANNEL_WEBKIT_SUBSYSTEM ".log");

This would result in debug.WPEWebKit.log for the WPE port, and debug.WebKitGTK.log for the GTK one. Also, this makes the name match the “log tag” passed to __android_log_{print,vprint,write} in #47339, making everything a bit more consistent.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the patch in this way. PTAL.

spenap
spenap reviewed Jun 30, 2025
View reviewed changes
Copy link
Contributor

@spenap spenap left a comment

Choose a reason for hiding this comment

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

Philippe has a valid question, but I'm OK with both your approach and his suggestion. The changes look good!

@aperezdc aperezdc force-pushed the android-logging-sysprop branch from f292bc1 to e22246b Compare June 30, 2025 11:25
@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 e22246b)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win
✅ 🛠 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

@aperezdc
[WTF][WPE] Read the log level string from an Android system property
6adbad7 
https://bugs.webkit.org/show_bug.cgi?id=295175

Reviewed by NOBODY (OOPS!).

On Android, if the WEBKIT_DEBUG environment variable is undefined, try
the debug.WPEWebKit.log system property as well to determine the value
returned by WTF::logLevelString(), which is used to configure logging
channels.

This allows using the "setprop" command line tool to configure logging:

  adb shell setprop debug.WPEWebKit.log 'Scrolling,Loading'
@aperezdc aperezdc force-pushed the android-logging-sysprop branch from e22246b to 6adbad7 Compare June 30, 2025 11:29
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on current version of this PR (hash 6adbad7)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 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

@aperezdc aperezdc requested review from philn and spenap June 30, 2025 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philn philn Awaiting requested review from philn

@spenap spenap Awaiting requested review from spenap

Assignees

@aperezdc aperezdc

Labels
WPE WebKit WebKit WPE component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aperezdc @webkit-early-warning-system @philn @spenap