Bug 73750 - [GTK] Move emissions of AtkDocument signals down to WebCore
: [GTK] Move emissions of AtkDocument signals down to WebCore
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:40 PST by Mario Sanchez Prada
Modified: 2011-12-05 16:11 PST (History)
7 users (show)

See Also:


Attachments
Patch proposal (17.17 KB, patch)
2011-12-03 10:57 PST, Mario Sanchez Prada
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Patch proposal (17.58 KB, patch)
2011-12-05 07:47 PST, Mario Sanchez Prada
cfleizach: review+
webkit.review.bot: commit‑queue-
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: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>