[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