Bug 94611 – Chromium: WebAccessibilityObject should expose updateBackingStore

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

Right now most WebAccessibilityObject methods call updateBackingStore under the hood, but this is inefficient when several methods are used in a row, and also has the potential to cause bugs if updateBackingStore deletes the underlying object.

Instead, WebAccessibilityObject should expose 

This will be a three-parter: first we'll expose updateBackingStore without a LayoutTest, then Chromium will be modified to use it, then finally WebKit will be modified to not call updateBackingStore automatically, with a new LayoutTest to cover the final change.

[Dominic Mazzoni]

Created attachment 159709 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[chris fleizach]

Comment on attachment 159709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159709&action=review

> Source/WebKit/chromium/public/WebAccessibilityObject.h:85
> +    WEBKIT_EXPORT void updateBackingStore();

seems like you might be able to make this one method that returns a bool

bool updateBackingStoreIfPossible() or something like that, so that you can check the result of this immediately

then presumably inside you would call updateBackingStore() and then return isDetached

[Dominic Mazzoni]

(In reply to comment #3)
> (From update of attachment 159709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159709&action=review
> 
> > Source/WebKit/chromium/public/WebAccessibilityObject.h:85
> > +    WEBKIT_EXPORT void updateBackingStore();
> 
> seems like you might be able to make this one method that returns a bool
> 
> bool updateBackingStoreIfPossible() or something like that, so that you can check the result of this immediately
> 
> then presumably inside you would call updateBackingStore() and then return isDetached

That's a good idea, except do you think that would imply that only this object's underlying node could be detached? Actually any AccessibilityObject could be detached...

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 159709 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159709&action=review
> > 
> > > Source/WebKit/chromium/public/WebAccessibilityObject.h:85
> > > +    WEBKIT_EXPORT void updateBackingStore();
> > 
> > seems like you might be able to make this one method that returns a bool
> > 
> > bool updateBackingStoreIfPossible() or something like that, so that you can check the result of this immediately
> > 
> > then presumably inside you would call updateBackingStore() and then return isDetached
> 
> That's a good idea, except do you think that would imply that only this object's underlying node could be detached? Actually any AccessibilityObject could be detached...

My thinking was that if updateBackingStore() returned false, you would process anymore code and return a default value, because that element was no longer valid.

Are you not using isDetached() for that purpose

[Dominic Mazzoni]

Created attachment 160094 [details]
Patch

[Dominic Mazzoni]

(In reply to comment #5)
> > That's a good idea, except do you think that would imply that only this object's underlying node could be detached? Actually any AccessibilityObject could be detached...
> 
> My thinking was that if updateBackingStore() returned false, you would process anymore code and return a default value, because that element was no longer valid.
> 
> Are you not using isDetached() for that purpose

You're correct, isDetached is definitely used for that purpose. I made the change as suggested, I agree it will simplify things some of the time. I modified the comment to warn that calling updateBackingStore on one object may invalidate a separate object.

[chris fleizach]

Comment on attachment 160094 [details]
Patch

should we change the name to something like "verifyBackingStoreIsValid()" or "ensureBackingStoreIsValid()"

[Dominic Mazzoni]

(In reply to comment #8)
> (From update of attachment 160094 [details])
> should we change the name to something like "verifyBackingStoreIsValid()" or "ensureBackingStoreIsValid()"

Do you want to change it in AccessibilityObject too? It doesn't make sense to call them something different.

How about updateBackingStoreAndCheckValidity? I'll make the change in all of the ports that use it.

[Dominic Mazzoni]

Any thoughts?

[chris fleizach]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 160094 [details] [details])
> > should we change the name to something like "verifyBackingStoreIsValid()" or "ensureBackingStoreIsValid()"
> 
> Do you want to change it in AccessibilityObject too? It doesn't make sense to call them something different.
> 
> How about updateBackingStoreAndCheckValidity? I'll make the change in all of the ports that use it.

We can change it in AXObject. I assume that would become a bool return as well. 

That name is a bit verbose, but I can't think of anything else good right now

[Dominic Mazzoni]

Hmmm, it looks like it wouldn't be so easy for AccessibilityObject::updateBackingStore to return a bool, because the object itself might be deleted at the time we'd want to check.

We can't retain a pointer to the wrapper, because there is no wrapper on Chromium.

It's probably not a good idea to retain the axID and check with AXObjectCache, because the hash lookup would be slower than the current code path.

I guess I'll just leave AccessibilityObject for now and add the new method to chromium. We can revisit later.

[Dominic Mazzoni]

Created attachment 160515 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 160515 [details]
Patch for landing

Clearing flags on attachment: 160515

Committed r126662: <http://trac.webkit.org/changeset/126662>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.