[webkit-reviews] review granted: [Bug 195644] Add AggregateLogger, a Logger specialization for singleton classes. : [Attachment 364459] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 195644: Add AggregateLogger, a Logger specialization for singleton classes.
https://bugs.webkit.org/show_bug.cgi?id=195644

Attachment 364459: Patch

https://bugs.webkit.org/attachment.cgi?id=364459&action=review




--- Comment #4 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 364459
  --> https://bugs.webkit.org/attachment.cgi?id=364459
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364459&action=review

> Source/WTF/wtf/AggregateLogger.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2019

> Source/WTF/wtf/AggregateLogger.h:90
> +	   return anyOf(m_loggers, [channel, level] (auto& logger) { return
logger->willLog(channel, level); });

As we discussed, I think this should be allOf(...) so nothing will be logged if
any logger is suppressing output.

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:69
> +    ALWAYS_LOG(LOGIDENTIFIER, "types: Video(",
count(PlatformMediaSession::Video)
> +	   , "), Audio(", count(PlatformMediaSession::Audio)
> +	   , "), VideoAudio(", count(PlatformMediaSession::VideoAudio)
> +	   , "), WebAudio(", count(PlatformMediaSession::WebAudio)
> +	   , ")");

Nit: this formatting is quite odd, it would be easier to read if the opening
and closing parentheses were on the same line

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:-152
> -    LOG(Media, "MediaSessionManagerCocoa::removeSession");

Why not replace this with "ALWAYS_LOG(LOGIDENTIFIER, session.logIdentifier())"

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:-159
> -    LOG(Media, "MediaSessionManagerCocoa::sessionWillEndPlayback");

Ditto.