Bug 195644 – Add AggregateLogger, a Logger specialization for singleton classes.

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

[Jer Noble]

Add AggregateLogger, a Logger specialization for singleton classes.

[Jer Noble]

Created attachment 364458 [details]
Patch

[Jer Noble]

Created attachment 364459 [details]
Patch

[Justin Fan]

Comment on attachment 364459 [details]
Patch

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

> Source/WebCore/platform/audio/PlatformMediaSessionManager.h:1
> + /*

Oops

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.h:62
> +    const char* logClassName() const final { return "MediaSessionManageriOS"; }

should the return type be  'const char* const'?

[Eric Carlson]

Comment on attachment 364459 [details]
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.

[Jer Noble]

Comment on attachment 364459 [details]
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

Changed.

>> 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.

Agreed.

>> Source/WebCore/platform/audio/PlatformMediaSessionManager.h:1
>> + /*
> 
> Oops

Oops

>> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:69
>> +        , ")");
> 
> Nit: this formatting is quite odd, it would be easier to read if the opening and closing parentheses were on the same line

Updated.

>> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:-152
>> -    LOG(Media, "MediaSessionManagerCocoa::removeSession");
> 
> Why not replace this with "ALWAYS_LOG(LOGIDENTIFIER, session.logIdentifier())"

Because it's already logged in PlatformMediaSessionManager::removeSession().

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

Ditto.

>> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.h:62
>> +    const char* logClassName() const final { return "MediaSessionManageriOS"; }
> 
> should the return type be  'const char* const'?

Sure.

[Jer Noble]

(In reply to Jer Noble from comment #5)
> Comment on attachment 364459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364459&action=review
> 
> >> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.h:62
> >> +    const char* logClassName() const final { return "MediaSessionManageriOS"; }
> > 
> > should the return type be  'const char* const'?
> 
> Sure.

Whoops, nope:

> error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers]

[Jer Noble]

Created attachment 364544 [details]
Patch for landing

[Jer Noble]

Created attachment 364546 [details]
Patch for landing

[EWS Watchlist]

Comment on attachment 364546 [details]
Patch for landing

Attachment 364546 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11492053

New failing tests:
http/tests/cache/memory-cache-pruning.html

[EWS Watchlist]

Created attachment 364563 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 364546 [details]
Patch for landing

Clearing flags on attachment: 364546

Committed r242901: <https://trac.webkit.org/changeset/242901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48860165>