[webkit-reviews] review granted: [Bug 176106] Add Logger observer and helper class : [Attachment 319370] Proposed patch.

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 176106: Add Logger observer and helper class
https://bugs.webkit.org/show_bug.cgi?id=176106

Attachment 319370: Proposed patch.

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




--- Comment #4 from Andy Estes <aestes at apple.com> ---
Comment on attachment 319370
  --> https://bugs.webkit.org/attachment.cgi?id=319370
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?