|Summary:||[GTK] Move emissions of AtkDocument signals down to WebCore|
|Product:||WebKit||Reporter:||Mario Sanchez Prada <mario>|
|Severity:||Normal||CC:||abarth, cfleizach, dglazkov, japhet, jdiggs, mrobinson, webkit.review.bot|
|Version:||528+ (Nightly build)|
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