Create a Language log channel
Created attachment 434880 [details] Patch
<rdar://problem/81495451>
Created attachment 434882 [details] Patch
Comment on attachment 434882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434882&action=review > Source/WTF/wtf/Language.cpp:42 > +DEFINE_LOG_CHANNEL_WITH_DETAILS(Language, WTFLogChannelState::On, WTFLogLevel::Error, LOG_CHANNEL_WEBKIT_SUBSYSTEM); On by default? > Source/WTF/wtf/Language.cpp:105 > + LOG_WITH_STREAM(Language, > + stream << "defaultLanguage() is returning " << languages[0]; > + ); Don't wrap > Source/WTF/wtf/Language.cpp:124 > + for (const auto& language : override) > + stream << " " << language; LOG_WITH_STREAM/TextStream knows how to output vectors so you shouldn't need the loop. > Source/WTF/wtf/Language.cpp:151 > + for (const auto& language : override) > + stream << " " << language; Ditto > Source/WTF/wtf/Language.h:38 > +extern WTFLogChannel LogLanguage; This is a bit of a hack/layering violation. This might be the first log channel used in WTF and you've arbitrarily picked the WebCore prefix? Maybe logging should be a PAL thing eventually. > Source/WTF/wtf/cf/LanguageCF.cpp:97 > + for (CFIndex i = 0; i < CFArrayGetCount(platformLanguages.get()); ++i) > + stream << " " << String(static_cast<CFStringRef>(CFArrayGetValueAtIndex(platformLanguages.get(), i))); Maybe we should teach TextStream how to log CFArrays. Or maybe we can invoke %@ formatting? > Source/WTF/wtf/cf/LanguageCF.cpp:122 > + for (const auto& language : languages) > + stream << " " << language; No need to loop. > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:58 > + LOG_WITH_STREAM(Language, A bit weird here that the WebCore log channel is being used in WebKit. > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:155 > + LOG_WITH_STREAM(Language, > + stream << "Process Launcher is copying OverrideLanguages into initialization message: " << languagesIterator->value; Why the wrapping? > Source/WebKit/UIProcess/WebProcessPool.cpp:372 > + for (const auto& language : languages) > + stream << " " << language; No need to loop > Source/WebKit/UIProcess/WebProcessPool.cpp:763 > + for (const auto& language : parameters.overrideLanguages) > + stream << " " << language; No need to loop > Source/WebKit/UIProcess/WebProcessProxy.cpp:380 > + LOG_WITH_STREAM(Language, > + stream << "Setting WebProcess's launch OverrideLanguages to " << languageString; > + ); Don't wrap > Source/WebKit/WebProcess/WebProcess.cpp:463 > + for (const auto& language : parameters.overrideLanguages) > + stream << " " << language; No need to loop > Source/WebKit/WebProcess/WebProcess.cpp:740 > + for (const auto& language : languages) > + stream << " " << language; No need to loop
Created attachment 434883 [details] Addressing review comments
Comment on attachment 434882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434882&action=review >> Source/WTF/wtf/Language.cpp:42 >> +DEFINE_LOG_CHANNEL_WITH_DETAILS(Language, WTFLogChannelState::On, WTFLogLevel::Error, LOG_CHANNEL_WEBKIT_SUBSYSTEM); > > On by default? There is no defaults write for WTF logging. All the logging in this patch is non-release, so I didn't figure it was a big deal. >> Source/WTF/wtf/Language.h:38 >> +extern WTFLogChannel LogLanguage; > > This is a bit of a hack/layering violation. This might be the first log channel used in WTF and you've arbitrarily picked the WebCore prefix? Maybe logging should be a PAL thing eventually. It's the second logging channel in WTF. I'm following the pattern of RefCountedLeaks. >> Source/WTF/wtf/cf/LanguageCF.cpp:97 >> + stream << " " << String(static_cast<CFStringRef>(CFArrayGetValueAtIndex(platformLanguages.get(), i))); > > Maybe we should teach TextStream how to log CFArrays. Or maybe we can invoke %@ formatting? This is a good idea, but I think it's for another patch. Do you think this one should be blocked on that one? I don't like the %@ formatting because it spreads the array across multiple lines, which TextStream doesn't do for Vectors. >> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:58 >> + LOG_WITH_STREAM(Language, > > A bit weird here that the WebCore log channel is being used in WebKit. It's not a WebCore log channel. There are 2 log channels - one for WebKit and one for WebCore. They both have the name "Language"
Comment on attachment 434882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434882&action=review >>> Source/WTF/wtf/Language.cpp:42 >>> +DEFINE_LOG_CHANNEL_WITH_DETAILS(Language, WTFLogChannelState::On, WTFLogLevel::Error, LOG_CHANNEL_WEBKIT_SUBSYSTEM); >> >> On by default? > > There is no defaults write for WTF logging. > > All the logging in this patch is non-release, so I didn't figure it was a big deal. I think it's probably a good idea for me to add a default write for WTF logging. We should have one. Line breaking is in WTF, which is a good indication that bidi will probably end up there too. So, if I don't add one now, I'll probably need to add one when I do more bidi stuff. So I might as well just do it now. I'll add all the infrastructure to make it a proper sibling to WebCoreLogging and WebKit2Logging.
Created attachment 434889 [details] WIP
Comment on attachment 434882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434882&action=review >>>> Source/WTF/wtf/Language.cpp:42 >>>> +DEFINE_LOG_CHANNEL_WITH_DETAILS(Language, WTFLogChannelState::On, WTFLogLevel::Error, LOG_CHANNEL_WEBKIT_SUBSYSTEM); >>> >>> On by default? >> >> There is no defaults write for WTF logging. >> >> All the logging in this patch is non-release, so I didn't figure it was a big deal. > > I think it's probably a good idea for me to add a default write for WTF logging. We should have one. > > Line breaking is in WTF, which is a good indication that bidi will probably end up there too. So, if I don't add one now, I'll probably need to add one when I do more bidi stuff. So I might as well just do it now. > > I'll add all the infrastructure to make it a proper sibling to WebCoreLogging and WebKit2Logging. https://bugs.webkit.org/show_bug.cgi?id=228768 >>> Source/WTF/wtf/Language.h:38 >>> +extern WTFLogChannel LogLanguage; >> >> This is a bit of a hack/layering violation. This might be the first log channel used in WTF and you've arbitrarily picked the WebCore prefix? Maybe logging should be a PAL thing eventually. > > It's the second logging channel in WTF. I'm following the pattern of RefCountedLeaks. Turns out it's the 5th! Even more reason to do https://bugs.webkit.org/show_bug.cgi?id=228768.
Created attachment 435222 [details] Patch for committing
Created attachment 435223 [details] Patch for committing
Committed r280811 (240381@main): <https://commits.webkit.org/240381@main>