Bug 176106 – Add Logger observer and helper class

• Status: ASSIGNED
• Component: WebCore Misc.

[Eric Carlson]

Make it possible to observe Logger output. Add a helper class to make logging code less verbose.

[Eric Carlson]

Created attachment 319367 [details]
Proposed patch.

[Build Bot]

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.

[Eric Carlson]

Created attachment 319370 [details]
Proposed patch.

[Andy Estes]

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?

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 319382 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 319382 [details]
Patch for landing

Clearing flags on attachment: 319382

Committed r221388: <http://trac.webkit.org/changeset/221388>

[Radar WebKit Bug Importer]

<rdar://problem/34168545>