Bug 25831

Created attachment 52585 [details] AtkDocument events implementation This patch implements the emission of the signals object:state-changed:busy, document:reload, document:load-complete, document:load-stopped and object:state-changed:defunct.

Attachment 52585 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 52590 [details] AtkDocument events implementation New version of the patch. Sorry for the error in the Changelog file of the previous one.

I just gave this a spin. Thanks for doing it! Unfortunately, here's what I'm seeing in the terminal window from which I launched GtkLauncher: (GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x968c768' (GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x96776a0' ** Message: console message: http://googlevideo.blogspot.com/ @813: ReferenceError: Can't find variable: OnLoad (GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x967ef40' (GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0xb4b14968' (GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x9674a00' ============================== Here's what I'm seeing in Accerciser's event monitor: [document frame | ] application: [application | GtkLauncher] [document frame | ] application: [application | GtkLauncher] document:load-complete(0, 0, ) source: [document frame | ] application: [application | GtkLauncher] [document frame | ] application: [application | GtkLauncher] [document frame | ] application: [application | GtkLauncher] ============================== Here's what I see in Accerciser's event monitor when using Firefox: document:reload(0, 0, Ubuntu Start Page) source: [document frame | Ubuntu Start Page] application: [application | Firefox] document:load-complete(0, 0, Ubuntu Start Page) source: [document frame | Ubuntu Start Page] application: [application | Firefox] document:load-complete(0, 0, foo - Google Search) source: [document frame | foo - Google Search] application: [application | Firefox] document:load-complete(0, 0, Foobar - Wikipedia, the free encyclopedia) source: [document frame | Foobar - Wikipedia, the free encyclopedia] application: [application | Firefox] The Accerciser output for WebKitGtk isn't just incomplete, something funky is taking place for events which are not document:load-complete. That's the biggest issue. It would be good to have the page title as the event.any_data, but if that's a pain it can be treated as a separate bug IMHO. (Sorry for the bad news!) BTW, José, if you're not familiar with Accerciser, I encourage you to get it. Best. Tool. EVER. Seriously. http://live.gnome.org/Accerciser If Alejandro (API) has the time, he should be able to give you a crash course in its use. Otherwise, I will do my best to be around on IRC. And if you don't see me in #webkit-gtk, I might be hiding but still online. My nick on freenode is joanie. Thanks again for your work!

Comment on attachment 52590 [details] AtkDocument events implementation Removing from the queue since the patch does not seem to work, need to work on it.

Created attachment 73716 [details] AtkDocument events implementation Attaching a new patch addressing the pending issues, based on Jose Millan's patch

Comment on attachment 73716 [details] AtkDocument events implementation View in context: https://bugs.webkit.org/attachment.cgi?id=73716&action=review > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:165 > + return returnString(coreObject->document()->title()); only returning the title here limits some future flexibility, so as using aria-label, name or something else that might also be available > WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:215 > + don't know how you would get into this method to turn on AX, since the only caller first checks if AX is on before calling this method... see notifyStatus() change

(In reply to comment #7) > (From update of attachment 73716 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73716&action=review > > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:165 > > + return returnString(coreObject->document()->title()); > > only returning the title here limits some future flexibility, so as using aria-label, name or something > else that might also be available My (probably poor) reasoning here was that no string was being returned for the object with the webArea role so that returning something like this would be at least an improvement (and very useful for emitting the signals along with this patch in the right way). But most likely you're right and I should instead re-write this in a way that all those other possible situations were taken into account as well (perhaps moving this lines to the end of the function, as some kind of fallback in case no other string was found, or by defining a dedicated static function). I'll come up with a new patch soon, just willing to share these thoughts now > > WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:215 > > + > > don't know how you would get into this method to turn on AX, since the only caller first checks if > AX is on before calling this method... see notifyStatus() change My bad, sure. Thanks for pointing it out.

Comment on attachment 73716 [details] AtkDocument events implementation Removing r? flag since there are still some changes that need to be done

Created attachment 73881 [details] AtkDocument events implementation New patch, addressing the issues pointed out by Chris.

Created attachment 74398 [details] AtkDocument events implementation + New unit test (In reply to comment #10) > Created an attachment (id=73881) [details] > AtkDocument events implementation > > New patch, addressing the issues pointed out by Chris. Following Martin's suggestion via IRC, now attaching a new version of the patch, featuring a new unit test to carefully check that the related events are being emitted, and in the right order. Asking for review over this one then...

Comment on attachment 74398 [details] AtkDocument events implementation + New unit test View in context: https://bugs.webkit.org/attachment.cgi?id=74398&action=review > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:196 > + Mmm, where is this used in the patch? > WebKit/gtk/tests/testatk.c:316 > + g_timeout_add(10000, (GSourceFunc)bailOutFromMainLoop, loop); Uh, 10000? In general I don't like this thing of having a timeout "in case the test hangs". It should either pass or ASSERT somewhere IMHO. That being said, 10s is surely way too long.

(In reply to comment #12) > (From update of attachment 74398 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74398&action=review > > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:196 > > + > > Mmm, where is this used in the patch? It's not explicitly used anywhere but this code allows that we get more complete information along with the signals issued by the webArea. In other words, without that code, we'd get things like this: document:load-complete(0, 0, ) source: [document frame | ] application: [application | GtkLauncher] ...but with the patch we'd get this: document:load-complete(0, 0, Ubuntu Start Page) source: [document frame | Ubuntu Start Page] application: [application | GtkLauncher] ...as that information comes right from the atk_object_get_name() method executed over the AtkObject for the webArea object. > > WebKit/gtk/tests/testatk.c:316 > > + g_timeout_add(10000, (GSourceFunc)bailOutFromMainLoop, loop); > > Uh, 10000? In general I don't like this thing of having a timeout "in case the test hangs". It should either pass or ASSERT somewhere IMHO. That being said, 10s is surely way too long. Well, I don't really see the problem because the test would normally (actually all the times I tried it) run as quick as the reload process lasts (that is, way less than 10 secs, not even 1sec I'd say). The timeout is just a way to ensure the test won't hang, and I couldn't find any other better option for that. Perhaps 10 secs is too much so we could lower that to 2-3 secs, that would look fine to me. But I do not see many other options here since we're dealing with asinchronous stuff... I'm open to suggestions, though :-)

(In reply to comment #13) > Well, I don't really see the problem because the test would normally (actually all the times I tried it) run as quick as the reload process lasts (that is, way less than 10 secs, not even 1sec I'd say). The timeout is just a way to ensure the test won't hang, and I couldn't find any other better option for that. > > Perhaps 10 secs is too much so we could lower that to 2-3 secs, that would look fine to me. But I do not see many other options here since we're dealing with asinchronous stuff... I'm open to suggestions, though :-) My point is, either you know the test just has to finish, probably very fast, or not. If you don't, then there's something wrong and we should fix it, or make the test ASSERT in case it hits the problem. Otherwise we should be adding 10s timeouts everywhere just in case some test hangs... (FWIW the layout tests have this I believe, but it's done generally and not in each test separately).

Created attachment 74920 [details] AtkDocument events implementation + New unit test (In reply to comment #14) > > My point is, either you know the test just has to finish, probably very fast, or not. > If you don't, then there's something wrong and we should fix it, or make the test > ASSERT in case it hits the problem. Otherwise we should be adding 10s timeouts > everywhere just in case some test hangs... (FWIW the layout tests have this I > believe, but it's done generally and not in each test separately). After the discussion about this in the IRC, I just rewrote the unit test code in testatk.c not to use a new main loop nor a call to g_add_timeout(), using the g_test_trap_* API instead, where I set a 2secs timeout as guard for the unlikely case where the unit test would fail. Attaching a new patch then

Comment on attachment 74920 [details] AtkDocument events implementation + New unit test Great stuff!

Committed r72764: <http://trac.webkit.org/changeset/72764>

Reopening as there seems to be a problem with the current patch, which was causing API crashes in the bots. The patch has already been rolled out through bug 50215. My wild guessing up to this point is that could be something related to the fork done trhough the g_test_trap_fork() function, so perhaps it would be better to avoid that usage and replace it with some good-enough equivalent code to test this case (something like the previous version of th epatch). But won't be able to tell for sure until I find a way to reproduce the problem locally

Looks like the problem is related to another (still unresolved) bug, which is currently being worked around through an "ASSERT(m_currentItem); if (!m_current_item) return;" combo: void HistoryController::restoreScrollPositionAndViewState() { [...] ASSERT(m_currentItem); // FIXME: As the ASSERT attests, it seems we should always have a currentItem here. // One counterexample is <rdar://problem/4917290> // For now, to cover this issue in release builds, there is no technical harm to returning // early and from a user standpoint - as in the above radar - the previous page load failed // so there *is* no scroll or view state to restore! if (!m_currentItem) return; [...] } Not sure how to proceed here then... I'll try to investigate the root issue to see if I can help on that FIXME, or just find another way to implement the unit test avoiding this code (whenever it's possible), if I can't find a better fix for the whole problem. Btw, is there any way I can check rdar://problem/4917290 ?

Depending on bug 50331, reported to track the issue about the failing ASSERT

Attachment 74920 [details] was posted by a committer and has review+, assigning to Mario Sanchez Prada for commit.

(In reply to comment #21) > Attachment 74920 [details] was posted by a committer and has review+, assigning to Mario Sanchez Prada for commit. Yes, I'm just waiting for bug 50331 to be resolved before committing this one, since it's depending on that.

Removing dependency from bug 50331 since it's very likely that, as soon as bug 54198 gets fixed, we could write a Layout test for this patch instead of an unit test (which was what was actually being blocked by bug 50331)

Created attachment 82118 [details] Patch proposal + Layout Tests Attaching a new patch that replaces the old unit test with a layout test, now that we have bug 54198 fixed and no longer depend on bug 50331 Asking for review again then. It should be pretty straightforward, as the fix itself hasn't changed a single line.

Comment on attachment 82118 [details] Patch proposal + Layout Tests Looks pretty good to me, great that we can use layout tests for this now.

(In reply to comment #25) > (From update of attachment 82118 [details]) > Looks pretty good to me, great that we can use layout tests for this now. Yeah, I definitely agree with this as well. Thanks for the quick review

Committed r78331: <http://trac.webkit.org/changeset/78331>

http://trac.webkit.org/changeset/78331 might have broken GTK Linux 32-bit Release The following tests are not passing: fast/dynamic/view-overflow.html fast/dynamic/window-scrollbars-test.html fast/frames/flattening/frameset-flattening-simple.html fast/overflow/clip-rects-fixed-ancestor.html fast/repaint/fixed-child-move-after-scroll.html fast/repaint/fixed-child-of-fixed-move-after-scroll.html fast/repaint/fixed-move-after-scroll.html fast/repaint/fixed-tranformed.html fast/repaint/fixed.html scrollbars/custom-scrollbar-with-incomplete-style.html transforms/2d/transform-fixed-container.html

Reopening as per rollout done through bug 54295

Comment on attachment 82118 [details] Patch proposal + Layout Tests Clearing up flags as this patch caused some problems and needed to be rolled out (see bug 54295)

Created attachment 82645 [details] Patch proposal + Layout test (In reply to comment #29) > Reopening as per rollout done through bug 54295 Attaching new patch, which does not interfere with other tests as it was pointed out in comment 28. Seems like we can't call to gtk_widget_get_accessible() from notifyAccessibilityStatus() in FrameLoaderClientGtk.cpp, as that will call AXObjectCache::rootObject() which will call getOrCreate() over m_document->view, which by that time might be not ready. I worked around this by directly calling to getOrCreate over the contentRenderer for the frame currently in use, which returned the valid AtkObject as well (and proof for this is that the returned object implements the AtkDocument interface which only happens for that single object of interest). Hence, eagerly asking for review :-)

Comment on attachment 82645 [details] Patch proposal + Layout test Let's give this another shot!

Committed r78715: <http://trac.webkit.org/changeset/78715>