Bug 73746 - [GTK] Don't log ATK document events in DRT
: [GTK] Don't log ATK document events in DRT
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2011-12-03 10:16 PST by
Modified: 2011-12-04 01:39 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-12-03 10:18:07 PST -------
*** Bug 67398 has been marked as a duplicate of this bug. ***
------- Comment #2 From 2011-12-03 10:33:31 PST -------
Created an attachment (id=117766) [details]
Patch proposal

Patch implementing this idea.
------- Comment #3 From 2011-12-03 10:42:19 PST -------
(From update of attachment 117766 [details])
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 From 2011-12-03 11:22:11 PST -------
(From update of attachment 117766 [details])
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 From 2011-12-03 11:32:07 PST -------
(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.
------- Comment #6 From 2011-12-03 12:14:28 PST -------
(In reply to comment #5)
> (From update of attachment 117766 [details] [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 From 2011-12-04 01:39:43 PST -------
Committed r101951: <http://trac.webkit.org/changeset/101951>