[webkit-reviews] review granted: [Bug 210564] Add logging to core accessibility. : [Attachment 397220] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Andres Gonzalez
<andresg_22 at apple.com>'s request for review:
Bug 210564: Add logging to core accessibility.
https://bugs.webkit.org/show_bug.cgi?id=210564

Attachment 397220: Patch

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




--- Comment #21 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 397220
  --> https://bugs.webkit.org/attachment.cgi?id=397220
Patch

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

> Source/WebCore/accessibility/AXLogger.cpp:53
> +void AXLogger::log(const String& message)

Why do you need this at all? Why not use LOG(Accessibility...) at all the call
sites?

> Source/WebCore/accessibility/AXLogger.cpp:69
> +TextStream& operator<<(TextStream& stream, const AccessibilityRole& role)

AccessibilityRole is just an enum. No need to pass by reference.

> Source/WebCore/accessibility/AXLogger.cpp:533
> +    stream << "objectID " << object.objectID() << " {";
> +    stream.increaseIndent();

Look at GroupScope which will do parens and stream.increaseIndent() for you.

> Source/WebCore/accessibility/AXLogger.cpp:541
> +    stream.dumpProperty("parentObject", parent ? parent->objectID() : 0);
> +#if PLATFORM(COCOA)
> +    stream.dumpProperty("remoteParentObject", object.remoteParentObject());
> +#endif

Seems a bit weird to dump parents. Normally you dump trees from the top down.

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:1300
> +WTF::TextStream& operator<<(WTF::TextStream&, const AccessibilityRole&);

AccessibilityRole by value