Bug 73750 - [GTK] Move emissions of AtkDocument signals down to WebCore
: [GTK] Move emissions of AtkDocument signals down to WebCore
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2011-12-03 10:40 PST by
Modified: 2011-12-05 16:11 PST (History)


Attachments
Patch proposal (17.17 KB, patch)
2011-12-03 10:57 PST, Mario Sanchez Prada
webkit.review.bot: commit‑queue-
Review Patch | 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-
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: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 From 2011-12-03 10:57:18 PST -------
Created an attachment (id=117768) [details]
Patch proposal
------- Comment #2 From 2011-12-03 11:35:43 PST -------
(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
------- Comment #3 From 2011-12-03 11:39:40 PST -------
(In reply to comment #2)
> (From update of attachment 117768 [details] [details])
> Attachment 117768 [details] [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 From 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 From 2011-12-04 21:05:45 PST -------
(From update of attachment 117768 [details])
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 From 2011-12-05 07:25:48 PST -------
(From update of attachment 117768 [details])
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 From 2011-12-05 07:47:48 PST -------
Created an attachment (id=117879) [details]
Patch proposal
------- Comment #8 From 2011-12-05 09:58:13 PST -------
(From update of attachment 117879 [details])
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 From 2011-12-05 12:37:26 PST -------
(From update of attachment 117879 [details])
r=me
------- Comment #10 From 2011-12-05 16:11:17 PST -------
Committed r102062: <http://trac.webkit.org/changeset/102062>