Bug 228764 – Create a Language log channel

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Create a Language log channel

[Myles C. Maxfield]

Created attachment 434880 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81495451>

[Myles C. Maxfield]

Created attachment 434882 [details]
Patch

[Simon Fraser (smfr)]

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

[Myles C. Maxfield]

Created attachment 434883 [details]
Addressing review comments

[Myles C. Maxfield]

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"

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 434889 [details]
WIP

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 435222 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 435223 [details]
Patch for committing

[Myles C. Maxfield]

Committed r280811 (240381@main): <https://commits.webkit.org/240381@main>