Bug 73746 – [GTK] Don't log ATK document events in DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Mario Sanchez Prada]

Some months ago, we introduced a new feature in the WKGTK's DRT so it could log accessibility related events just by telling it so (accessibilityController.logAccessibilityEvents()), which basically meant that the following type of events would be logged:

   1. Events related to the accessible objects: 'state-change', 'focus-event', 'visible-data-changed'...
   2. Events related to the 'document' representing the web, that is, the following three events: 'load-complete', 'load-stopped' and 'reload'.

It turned out that, while the first kind of loggintg (1) proved itself to be really useful and convenient, the second one presented several problems, specially in those cases where the layout test was performing reloads, contained different frames... behaving with certain flakyness some times, which caused some tests (sometimes completely unrelated to this loading events) to be skipped, since the actual output was different in those document-related lines.

So, and based in some other experiences, I'm proposing now to remove the second kinf of logging (2) from the WKGTK's DRT, and so replace the layout test 'document-reload-events.html' with a new unit test in testatk.c

Patch coming in some minutes...

[Mario Sanchez Prada]

*** Bug 67398 has been marked as a duplicate of this bug. ***

[Mario Sanchez Prada]

Created attachment 117766 [details]
Patch proposal

Patch implementing this idea.

[Martin Robinson]

Comment on attachment 117766 [details]
Patch proposal

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

Seems good to me, but I have included a few minor style issue below.

> Source/WebKit/gtk/ChangeLog:21
> +        (updateLoadingEventsResult): New, updates a global variable to
> +        allow checking later on that the right signals were emmitted.
> +        (documentLoadingEventCb): New, global listener for load-complete
> +        and reload signals over the document object (the web area).
> +        (testWebkitAtkDocumentLoadingEvents): New unit test, globally
> +        connects to document-related signals and check they are properly
> +        emitted when reloading the web view.
> +        (main): Added new test.

This is nice.

> Source/WebKit/gtk/tests/testatk.c:495
> +    if (!ATK_IS_OBJECT(axObject) || !signalName)

Is this an error if it doesn't happen. If so, would it be better to assert here instead?

> Source/WebKit/gtk/tests/testatk.c:506
> +
> +static gboolean documentLoadingEventCb(GSignalInvocationHint *signalHint,
> +                                       guint numParamValues,
> +                                       const GValue *paramValues,
> +                                       gpointer data)

Please put these on one line to match WebKit style. The function should also be named documentLoadingEventCallback.

> Source/WebKit/gtk/tests/testatk.c:516
> +    GSignalQuery signal_query;

signal_query -> signalQuery.

[Mario Sanchez Prada]

Comment on attachment 117766 [details]
Patch proposal

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

Thanks for the fast review. I'll address those issues before pushing.

>> Source/WebKit/gtk/tests/testatk.c:495
>> +    if (!ATK_IS_OBJECT(axObject) || !signalName)
> 
> Is this an error if it doesn't happen. If so, would it be better to assert here instead?

Actually the whole line should be removed since that should not ever happen. Also, the axObject in the params list is not used in the function, so it should be removed too.

Will do those small changes before committing.

>> Source/WebKit/gtk/tests/testatk.c:506
>> +                                       gpointer data)
> 
> Please put these on one line to match WebKit style. The function should also be named documentLoadingEventCallback.

Ok.

[Martin Robinson]

Comment on attachment 117766 [details]
Patch proposal

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

>>> Source/WebKit/gtk/tests/testatk.c:495
>>> +    if (!ATK_IS_OBJECT(axObject) || !signalName)
>> 
>> Is this an error if it doesn't happen. If so, would it be better to assert here instead?
> 
> Actually the whole line should be removed since that should not ever happen. Also, the axObject in the params list is not used in the function, so it should be removed too.
> 
> Will do those small changes before committing.

That's a good situation to do an assertion.

[Mario Sanchez Prada]

(In reply to comment #5)
> (From update of attachment 117766 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117766&action=review
> 
> >>> Source/WebKit/gtk/tests/testatk.c:495
> >>> +    if (!ATK_IS_OBJECT(axObject) || !signalName)
> >> 
> >> Is this an error if it doesn't happen. If so, would it be better to assert here instead?
> > 
> > Actually the whole line should be removed since that should not ever happen. Also, the axObject in the params list is not used in the function, so it should be removed too.
> > 
> > Will do those small changes before committing.
> 
> That's a good situation to do an assertion.

Ok, will do then, but won't push until tomorrow since I'm leaving my place in 5 min

[Mario Sanchez Prada]

Committed r101951: <http://trac.webkit.org/changeset/101951>