Bug 73746 - [GTK] Don't log ATK document events in DRT
: [GTK] Don't log ATK document events in DRT
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-03 10:16 PST by Mario Sanchez Prada
Modified: 2011-12-04 01:39 PST (History)
2 users (show)

See Also:


Attachments
Patch proposal (17.32 KB, patch)
2011-12-03 10:33 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2011-12-03 10:16:11 PST
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...
Comment 1 Mario Sanchez Prada 2011-12-03 10:18:07 PST
*** Bug 67398 has been marked as a duplicate of this bug. ***
Comment 2 Mario Sanchez Prada 2011-12-03 10:33:31 PST
Created attachment 117766 [details]
Patch proposal

Patch implementing this idea.
Comment 3 Martin Robinson 2011-12-03 10:42:19 PST
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.
Comment 4 Mario Sanchez Prada 2011-12-03 11:22:11 PST
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.
Comment 5 Martin Robinson 2011-12-03 11:32:07 PST
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.
Comment 6 Mario Sanchez Prada 2011-12-03 12:14:28 PST
(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
Comment 7 Mario Sanchez Prada 2011-12-04 01:39:43 PST
Committed r101951: <http://trac.webkit.org/changeset/101951>