Bug 176106

Summary: Add Logger observer and helper class
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebCore Misc.Assignee: Eric Carlson <eric.carlson>
Status: ASSIGNED ---    
Severity: Normal CC: aestes, buildbot, commit-queue, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch.
none
Proposed patch.
none
Patch for landing none

Description Eric Carlson 2017-08-30 08:33:36 PDT
Make it possible to observe Logger output. Add a helper class to make logging code less verbose.
Comment 1 Eric Carlson 2017-08-30 08:57:09 PDT
Created attachment 319367 [details]
Proposed patch.
Comment 2 Build Bot 2017-08-30 08:58:28 PDT
Attachment 319367 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/Logger.h:59:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Eric Carlson 2017-08-30 09:23:44 PDT
Created attachment 319370 [details]
Proposed patch.
Comment 4 Andy Estes 2017-08-30 09:44:42 PDT
Comment on attachment 319370 [details]
Proposed patch.

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

> Source/WebCore/PAL/pal/Logger.h:40
> +    template<typename U = T> static typename std::enable_if<std::is_same<U, bool>::value, String>::type toString(bool argument) { return argument ? String("true") : String("false"); }

You can use ASCIILiteral for "true" and "false".

> Source/WebCore/PAL/pal/Logger.h:45
> +    template<typename U = T> static typename std::enable_if<std::is_same<U, AtomicString>::value, String>::type toString(AtomicString argument) { return argument.string(); }
> +    template<typename U = T> static typename std::enable_if<std::is_same<U, const AtomicString&>::value, String>::type toString(const AtomicString& argument) { return argument.string(); }

I wonder if you can use typename std::remove_reference<U>::type in the std::is_same<> check so that you don't have to treat AtomicString and const AtomicString& separately.

> Source/WebCore/PAL/pal/Logger.h:47
> +    template<typename U = T> static typename std::enable_if<std::is_same<U, const String&>::value, String>::type toString(const String& argument) { return argument; }

Ditto.

> Source/WebCore/PAL/pal/Logger.h:169
> +    static inline void removeObserver(Observer& observer)

Looks like it's up to the caller to ensure that removeObserver() is called before an Observer passed to addObserver() is destroyed. Is that the design?
Comment 5 Eric Carlson 2017-08-30 11:20:11 PDT
Comment on attachment 319370 [details]
Proposed patch.

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

Thanks!

>> Source/WebCore/PAL/pal/Logger.h:40
>> +    template<typename U = T> static typename std::enable_if<std::is_same<U, bool>::value, String>::type toString(bool argument) { return argument ? String("true") : String("false"); }
> 
> You can use ASCIILiteral for "true" and "false".

Fixed.

>> Source/WebCore/PAL/pal/Logger.h:45
>> +    template<typename U = T> static typename std::enable_if<std::is_same<U, const AtomicString&>::value, String>::type toString(const AtomicString& argument) { return argument.string(); }
> 
> I wonder if you can use typename std::remove_reference<U>::type in the std::is_same<> check so that you don't have to treat AtomicString and const AtomicString& separately.

Great idea, fixed.

>> Source/WebCore/PAL/pal/Logger.h:47
>> +    template<typename U = T> static typename std::enable_if<std::is_same<U, const String&>::value, String>::type toString(const String& argument) { return argument; }
> 
> Ditto.

Ditto.

>> Source/WebCore/PAL/pal/Logger.h:169
>> +    static inline void removeObserver(Observer& observer)
> 
> Looks like it's up to the caller to ensure that removeObserver() is called before an Observer passed to addObserver() is destroyed. Is that the design?

Yes, I wanted to keep this as simple as possible for now. I will reevaluate later when it is used outside of testing.
Comment 6 Eric Carlson 2017-08-30 11:26:12 PDT
Created attachment 319382 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-08-30 12:06:53 PDT
Comment on attachment 319382 [details]
Patch for landing

Clearing flags on attachment: 319382

Committed r221388: <http://trac.webkit.org/changeset/221388>
Comment 8 Radar WebKit Bug Importer 2017-08-30 12:11:08 PDT
<rdar://problem/34168545>