Bug 195644 - Add AggregateLogger, a Logger specialization for singleton classes.
Summary: Add AggregateLogger, a Logger specialization for singleton classes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-12 15:13 PDT by Jer Noble
Modified: 2019-03-13 13:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (28.97 KB, patch)
2019-03-12 15:22 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (29.03 KB, patch)
2019-03-12 15:26 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (28.80 KB, patch)
2019-03-13 10:24 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (29.20 KB, patch)
2019-03-13 10:33 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.97 MB, application/zip)
2019-03-13 12:50 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2019-03-12 15:13:48 PDT
Add AggregateLogger, a Logger specialization for singleton classes.
Comment 1 Jer Noble 2019-03-12 15:22:40 PDT
Created attachment 364458 [details]
Patch
Comment 2 Jer Noble 2019-03-12 15:26:52 PDT
Created attachment 364459 [details]
Patch
Comment 3 Justin Fan 2019-03-12 17:20:23 PDT
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'?
Comment 4 Eric Carlson 2019-03-12 17:40:21 PDT
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.
Comment 5 Jer Noble 2019-03-12 17:51:46 PDT
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.
Comment 6 Jer Noble 2019-03-12 17:56:30 PDT
(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]
Comment 7 Jer Noble 2019-03-13 10:24:50 PDT
Created attachment 364544 [details]
Patch for landing
Comment 8 Jer Noble 2019-03-13 10:33:24 PDT
Created attachment 364546 [details]
Patch for landing
Comment 9 Build Bot 2019-03-13 12:50:15 PDT
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
Comment 10 Build Bot 2019-03-13 12:50:26 PDT
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
Comment 11 WebKit Commit Bot 2019-03-13 13:18:14 PDT
Comment on attachment 364546 [details]
Patch for landing

Clearing flags on attachment: 364546

Committed r242901: <https://trac.webkit.org/changeset/242901>
Comment 12 WebKit Commit Bot 2019-03-13 13:18:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-03-13 13:19:23 PDT
<rdar://problem/48860165>