[webkit-reviews] review granted: [Bug 228764] Create a Language log channel : [Attachment 434882] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 3 20:37:51 PDT 2021
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 228764: Create a Language log channel
https://bugs.webkit.org/show_bug.cgi?id=228764
Attachment 434882: Patch
https://bugs.webkit.org/attachment.cgi?id=434882&action=review
--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 434882
--> https://bugs.webkit.org/attachment.cgi?id=434882
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
More information about the webkit-reviews
mailing list