If an AccessibilityObject is detached, any WebAccessibilityObject methods should return an empty or default value.
Created attachment 144622 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 144622 [details] Patch looks ok to me.
Comment on attachment 144622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144622&action=review One minor comment, otherwise looks good. > Source/WebKit/chromium/public/WebAccessibilityObject.h:72 > + bool isDetached() const; This needs to be marked WEBKIT_EXPORT
Comment on attachment 144622 [details] Patch Attachment 144622 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12839818 New failing tests: accessibility/loading-iframe-sends-notification.html platform/chromium/accessibility/add-to-menu-list-crashes.html accessibility/image-map1.html accessibility/selection-states.html accessibility/multiselect-list-reports-active-option.html accessibility/loading-iframe-updates-axtree.html
Created attachment 144670 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 144720 [details] Patch
Created attachment 144723 [details] Patch
Please take another look. It ended up being slightly more complicated, I didn't realize this would break existing tests, but this was a change that was needed anyway. The original patch worked for AccessibilityRenderObject but not other subclasses of AccessibilityObject. Explanation: for all other ports, there's one wrapper for every AccessibilityObject. When the AccessibilityObject is detached, it detaches the wrapper. The wrapper is typically a native refcounted object, so if the operating system is still holding onto a reference, the wrapper will persist, but its getters won't do anything because it's detached. For the Chromium port, the native accessibility objects are in a different process, so we don't need native wrappers. The AccessibilityObjectWrapper code was never used, so I deleted it. Instead, AccessibilityObject just keeps track or whether or not it's detached - for all objects, not just AccessibilityRenderObject. That way the Chromium port can figure out if it has a detached object without needing a wrapper.
Comment on attachment 144723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144723&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:102 > + return true; should the default isDetached return whether the wrapper == nil? > Source/WebKit/chromium/src/WebAccessibilityObject.cpp:107 > m_private->updateBackingStore(); is it worth combining isDetached() and m_private->updateBackingStore()? i see that would save you from duplicating that same code in every method. it would also help catch causes where updateBackingStore may not be called (like performDefaultAction)
(In reply to comment #10) > (From update of attachment 144723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144723&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:102 > > + return true; > > should the default isDetached return whether the wrapper == nil? I can't find any place where isDetached is used on Mac or any other platform. On any other platform, detaching directly disconnects the native wrapper so that it no longer has a pointer to the AccessibilityObject. Only in Chromium do we keep around a reference to an AccessibilityObject for a little while longer (and FYI, only while a notification is queued). So I'm happy to add it, if it makes more sense, for future use? > > Source/WebKit/chromium/src/WebAccessibilityObject.cpp:107 > > m_private->updateBackingStore(); > > is it worth combining isDetached() and m_private->updateBackingStore()? > > i see that would save you from duplicating that same code in every method. it would also help catch causes where updateBackingStore may not be called (like performDefaultAction) Agreed! Let's do this in a separate change, though - I think the best solution would be to directly expose updateBackingStore in WebKit so that accessibility calls can be batched. Sound good?
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 144723 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=144723&action=review > > > > > Source/WebCore/accessibility/AccessibilityObject.cpp:102 > > > + return true; > > > > should the default isDetached return whether the wrapper == nil? > > I can't find any place where isDetached is used on Mac or any other platform. On any other platform, detaching directly disconnects the native wrapper so that it no longer has a pointer to the AccessibilityObject. Only in Chromium do we keep around a reference to an AccessibilityObject for a little while longer (and FYI, only while a notification is queued). > > So I'm happy to add it, if it makes more sense, for future use? I think we should change it now, because as it stands, it is returning false information. > > > > Source/WebKit/chromium/src/WebAccessibilityObject.cpp:107 > > > m_private->updateBackingStore(); > > > > is it worth combining isDetached() and m_private->updateBackingStore()? > > > > i see that would save you from duplicating that same code in every method. it would also help catch causes where updateBackingStore may not be called (like performDefaultAction) > > Agreed! Let's do this in a separate change, though - I think the best solution would be to directly expose updateBackingStore in WebKit so that accessibility calls can be batched. Sound good? Sounds good
Created attachment 144832 [details] Patch
Comment on attachment 144832 [details] Patch Attachment 144832 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12847918
Created attachment 144854 [details] Patch for landing
Comment on attachment 144854 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=144854&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:92 > -#if HAVE(ACCESSIBILITY) > +#if PLATFORM(CHROMIUM) > + m_detached = true; > +#elif HAVE(ACCESSIBILITY) These ifdefs don't look quite right. PLATFORM(CHROMIUM) can build with HAVE(ACCESSIBILITY) and !HAVE(ACCESSIBILITY). It seems strange to couple these two ifdefs.
Created attachment 144887 [details] Patch
(In reply to comment #16) > (From update of attachment 144854 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144854&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:92 > > -#if HAVE(ACCESSIBILITY) > > +#if PLATFORM(CHROMIUM) > > + m_detached = true; > > +#elif HAVE(ACCESSIBILITY) > > These ifdefs don't look quite right. PLATFORM(CHROMIUM) can build with HAVE(ACCESSIBILITY) and !HAVE(ACCESSIBILITY). It seems strange to couple these two ifdefs. You're right. I fixed it, now the ifdefs are consistent with whether setWrapper() and wrapper() are defined in AccessibilityObject.h.
Comment on attachment 144887 [details] Patch Thanks. Do you want me to forward Chris' R+, or should we wait for him to R+ this patch himself?
(In reply to comment #19) > (From update of attachment 144887 [details]) > Thanks. Do you want me to forward Chris' R+, or should we wait for him to R+ this patch himself? I think it'd be okay to forward the R+ if you don't mind, since Chris was happy with it before and we only made this one small change. Thanks.
Comment on attachment 144887 [details] Patch Done.
Comment on attachment 144887 [details] Patch Clearing flags on attachment: 144887 Committed r119012: <http://trac.webkit.org/changeset/119012>
All reviewed patches have been landed. Closing bug.