Bug 73750

Summary: [GTK] Move emissions of AtkDocument signals down to WebCore
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cfleizach, dglazkov, japhet, jdiggs, mrobinson, webkit.review.bot
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch proposal
webkit.review.bot: commit-queue-
Patch proposal cfleizach: review+, webkit.review.bot: commit-queue-

Description Mario Sanchez Prada 2011-12-03 10:40:29 PST
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.
Comment 1 Mario Sanchez Prada 2011-12-03 10:57:18 PST
Created attachment 117768 [details]
Patch proposal
Comment 2 WebKit Review Bot 2011-12-03 11:35:43 PST
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
Comment 3 Mario Sanchez Prada 2011-12-03 11:39:40 PST
(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?
Comment 4 Mario Sanchez Prada 2011-12-04 04:07:09 PST
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.
Comment 5 chris fleizach 2011-12-04 21:05:45 PST
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
Comment 6 Mario Sanchez Prada 2011-12-05 07:25:48 PST
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.
Comment 7 Mario Sanchez Prada 2011-12-05 07:47:48 PST
Created attachment 117879 [details]
Patch proposal
Comment 8 WebKit Review Bot 2011-12-05 09:58:13 PST
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
Comment 9 chris fleizach 2011-12-05 12:37:26 PST
Comment on attachment 117879 [details]
Patch proposal

r=me
Comment 10 Mario Sanchez Prada 2011-12-05 16:11:17 PST
Committed r102062: <http://trac.webkit.org/changeset/102062>