Bug 73750 – [GTK] Move emissions of AtkDocument signals down to WebCore

• Component: Accessibility
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

At the moment, the emission of the AtkDocument related signals ('load-complete', 'load-stopped' and 'reload') is being done from the FrameLoaderClientGtk class, in Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp.

This has not been a (big) problem so far, but now that we want to add a11y support to WK2 such a thing doesn't seem to be a good idea, because you would need to implement something similar in WebKit2 in case you wanted to see those signals in WebKit2GTK based browsers (and you will want that, be sure of it). Such implementation, also, should happen in the WebProcess, since that's were we have access to the AtkObject wrapping the WebCore's accessibility object for the webArea.

However, a better alternative seems to be to move the emission of those signals down to WebCore, so we would just emit them in one place, and both WebKitGTK and WebKit2GTK based browsers would see them without any extra work.

So, this bug is for that.

[Mario Sanchez Prada]

Created attachment 117768 [details]
Patch proposal

[WebKit Review Bot]

Comment on attachment 117768 [details]
Patch proposal

Attachment 117768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10725811

New failing tests:
svg/custom/linking-uri-01-b.svg

[Mario Sanchez Prada]

(In reply to comment #2)
> (From update of attachment 117768 [details])
> Attachment 117768 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10725811
> 
> New failing tests:
> svg/custom/linking-uri-01-b.svg

Seems unrelated to me. Flaky test perhaps?

[Mario Sanchez Prada]

Even if this patch shouldn't modify the behaviour in other ports different than GTK (it provides empty implementations for those ports using their own platform-specific versions of AXObjectCache), I guess Chris might be interested on taking a look to it, so adding him to CC.

[chris fleizach]

Comment on attachment 117768 [details]
Patch proposal

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

> Source/WebCore/accessibility/AXObjectCache.h:153
> +        AXLoadingReload,

this should be Reloaded

> Source/WebCore/accessibility/AXObjectCache.h:158
> +    void loadingEventNotification(Frame*, AXLoadingEvent);

i would name this frameLoadingEventNotification

> Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:195
> +    // Sanity check

comment is unnecessary

> Source/WebCore/loader/FrameLoader.cpp:1123
> +            : AXObjectCache::AXLoadingStarted;

probably not necessary to separate onto three lines

[Mario Sanchez Prada]

Comment on attachment 117768 [details]
Patch proposal

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

Thanks a lot for the quick review, Chris. I assume you're reviewing- because of the things you pointed out but that overall you don't see it as a wrong approach. So, I will address your comments and upload a new version soon.

Now see my comments below...

>> Source/WebCore/accessibility/AXObjectCache.h:153
>> +        AXLoadingReload,
> 
> this should be Reloaded

Ok

>> Source/WebCore/accessibility/AXObjectCache.h:158
>> +    void loadingEventNotification(Frame*, AXLoadingEvent);
> 
> i would name this frameLoadingEventNotification

Sounds better indeed. I'll change it.

>> Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:195
>> +    // Sanity check
> 
> comment is unnecessary

Agreed :)

>> Source/WebCore/loader/FrameLoader.cpp:1123
>> +            : AXObjectCache::AXLoadingStarted;
> 
> probably not necessary to separate onto three lines

Well, it just looked better to my eyes but yeah... I agree it's not necessary. Will change that too.

[Mario Sanchez Prada]

Created attachment 117879 [details]
Patch proposal

[WebKit Review Bot]

Comment on attachment 117879 [details]
Patch proposal

Attachment 117879 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10688644

New failing tests:
svg/custom/linking-uri-01-b.svg

[chris fleizach]

Comment on attachment 117879 [details]
Patch proposal

r=me

[Mario Sanchez Prada]

Committed r102062: <http://trac.webkit.org/changeset/102062>