Bug 210564 – Add logging to core accessibility.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

Add logging to core accessibility.

[Andres Gonzalez]

Created attachment 396561 [details]
Patch

[EWS]

Committed r260168: <https://trac.webkit.org/changeset/260168>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396561 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61863477>

[Simon Fraser (smfr)]

Was there a reason not to use the usual LOG and LOG_WITH_STREAM macros?

[Andres Gonzalez]

(In reply to Simon Fraser (smfr) from comment #4)
> Was there a reason not to use the usual LOG and LOG_WITH_STREAM macros?

Thanks Simon. Part of the reason is my ignorance, didn't know that that was the usual way. So I went ahead and made the change to use those macros, to find out that they log to the standard output and not to the system log stream. We want to be able moving forward to log to the system log, not just to the std output. If there is a way to use the LOG macros to log to the system log, please let me know. Thanks again.

[Simon Fraser (smfr)]

If you're logging to system log, in release builds, you need to use the RELEASE_LOG macros.

This is important because the logged data needs to undergo privacy review.

[Andres Gonzalez]

Reopening to attach new patch.

[Andres Gonzalez]

Created attachment 396681 [details]
Patch

[Andres Gonzalez]

(In reply to Simon Fraser (smfr) from comment #6)
> If you're logging to system log, in release builds, you need to use the
> RELEASE_LOG macros.
> 
> This is important because the logged data needs to undergo privacy review.

Got it, using LOG macros in latest review. Thanks again.

[Simon Fraser (smfr)]

Comment on attachment 396681 [details]
Patch

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

> Source/WebCore/accessibility/AXLogger.cpp:41
> +        channel->state = WTFLogChannelState::On;

Does this turn on accessibility logging for everyone? We certainly don't want that.

[Andres Gonzalez]

Created attachment 397016 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 397016 [details]
Patch

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

> Source/WebCore/accessibility/AXLogger.cpp:64
> +    stream << "objectID: " << object.objectID() << " {\n";
> +    stream << "role: " << static_cast<unsigned>(object.roleValue()) << "\n";
> +    stream << "}";

There are "dumpProperty" helpers that output something like this format.

Also instead of static_cast<unsigned>(object.roleValue()) you should implement WTF::TextStream& operator<<(WTF::TextStream&, AccessibilityRole);

Better yet, implement  WTF::TextStream& operator<<(WTF::TextStream&, const AXCoreObject&); and you don't need this function at all.

[Andres Gonzalez]

Created attachment 397080 [details]
Patch

[Andres Gonzalez]

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 397016 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397016&action=review
> 
> > Source/WebCore/accessibility/AXLogger.cpp:64
> > +    stream << "objectID: " << object.objectID() << " {\n";
> > +    stream << "role: " << static_cast<unsigned>(object.roleValue()) << "\n";
> > +    stream << "}";
> 
> There are "dumpProperty" helpers that output something like this format.

Using dumpProperty.
> 
> Also instead of static_cast<unsigned>(object.roleValue()) you should
> implement WTF::TextStream& operator<<(WTF::TextStream&, AccessibilityRole);

Done.
> 
> Better yet, implement  WTF::TextStream& operator<<(WTF::TextStream&, const
> AXCoreObject&); and you don't need this function at all.

Done. But still need this function to be able to write AXLOG(axCoreObject). Thanks.

[Simon Fraser (smfr)]

Too much red to review.

[chris fleizach]

./accessibility/AXLogger.cpp:53:34: error: unused parameter 'message' [-Werror,-Wunused-parameter]

[Andres Gonzalez]

Created attachment 397193 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #16)
> ./accessibility/AXLogger.cpp:53:34: error: unused parameter 'message'
> [-Werror,-Wunused-parameter]

Fixed release build.

The long function is just one repetitive long switch to provide a logging string for each AccessibilityRole, 151 of them.

[Andres Gonzalez]

Created attachment 397220 [details]
Patch

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #19)
> Created attachment 397220 [details]
> Patch

Fix for non-COCOA builds.

[Simon Fraser (smfr)]

Comment on attachment 397220 [details]
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

[Andres Gonzalez]

Created attachment 397258 [details]
Patch

[Andres Gonzalez]

(In reply to Simon Fraser (smfr) from comment #21)
> Comment on attachment 397220 [details]
> 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?

It is convenient to always type just AXLOG(x) where x can be a simple string or some other accessibility object.
> 
> > Source/WebCore/accessibility/AXLogger.cpp:69
> > +TextStream& operator<<(TextStream& stream, const AccessibilityRole& role)
> 
> AccessibilityRole is just an enum. No need to pass by reference.

Fixed.
> 
> > Source/WebCore/accessibility/AXLogger.cpp:533
> > +    stream << "objectID " << object.objectID() << " {";
> > +    stream.increaseIndent();
> 
> Look at GroupScope which will do parens and stream.increaseIndent() for you.

Great! Done.
> 
> > 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.

This is the dump for a single object. It just happened that I needed the parent and the remote parent for another bug I was debugging, and then add them to the logger. We'll add more properties to this method as needed. Will also add another method to log the accessibility object hierarchy.
> 
> > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1300
> > +WTF::TextStream& operator<<(WTF::TextStream&, const AccessibilityRole&);
> 
> AccessibilityRole by value

Fixed. Thanks!

[EWS]

Committed r260548: <https://trac.webkit.org/changeset/260548>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397258 [details].