Bug 69676 – [Chromium] Improve AccessibilityController and AccessibilityUIElement

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

[Dominic Mazzoni]

Specifically, I have a couple of WebCore/accessibility patches that need layout tests, and we should have tests that assert that we get exactly the notifications we need for Chromium, since the same notifications aren't necessarily needed on Mac OS X or other platforms.

The initial goal is to implement enough functionality to make those tests work and get as many other accessibility layout tests to pass at the same time.

[Dominic Mazzoni]

Created attachment 110253 [details]
Patch

[chris fleizach]

Comment on attachment 110253 [details]
Patch

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

seems ok overall. a few minor things interspersed.

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:374
> +        CppVariant notificationNameArgument;

you should declare  m_notificationCallbacks.size(); outside the loop so it's not called on every iteration

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:454
> +    CppVariant* result)

this seems like it should be on the same line as the method declaration

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:725
> +        result->setNull();

this if statement seems like it could lead to crashes in DRT
ie) if arguments.size == 0, then the first half of the argument evaluates to true, then the second half ( arguments[0] ) is evaluated, which will crash since size == 0

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:791
> +        if (m_elements[i]->isEqual(object))

You should move m_elements.size() outside the food loop

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.h:50
> +    virtual bool isEqual(const WebKit::WebAccessibilityObject& other);

"other" is unnecessary here

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.h:126
> +        const CppArgumentList&, CppVariant*);

does not seem necessary to put this on a separate line

[Dominic Mazzoni]

Comment on attachment 110253 [details]
Patch

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

>> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:374
>> +        CppVariant notificationNameArgument;
> 
> you should declare  m_notificationCallbacks.size(); outside the loop so it's not called on every iteration

Sure, done. I know older compilers couldn't do this automatically, but maybe clang can? Either way I want to match the webkit style.

>> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:454
>> +    CppVariant* result)
> 
> this seems like it should be on the same line as the method declaration

Done

>> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:725
>> +        result->setNull();
> 
> this if statement seems like it could lead to crashes in DRT
> ie) if arguments.size == 0, then the first half of the argument evaluates to true, then the second half ( arguments[0] ) is evaluated, which will crash since size == 0

Good catch. This should have been an || not an &&. Same with the function below.

>> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:791
>> +        if (m_elements[i]->isEqual(object))
> 
> You should move m_elements.size() outside the food loop

I wonder if you were hungry when you wrote this. :) Done.

>> Tools/DumpRenderTree/chromium/AccessibilityUIElement.h:50
>> +    virtual bool isEqual(const WebKit::WebAccessibilityObject& other);
> 
> "other" is unnecessary here

Done. If I understand correctly, I do want an argument name when the argument is nonobvious, like in notificationReceived, below - but not when there's a single argument and the meaning is obvious from the name of the method, is that right?

[chris fleizach]

(In reply to comment #3)

> Done. If I understand correctly, I do want an argument name when the argument is nonobvious, like in notificationReceived, below - but not when there's a single argument and the meaning is obvious from the name of the method, is that right?

Yes, I believe that is the standard. Thanks

[Dominic Mazzoni]

Created attachment 110418 [details]
Patch

[chris fleizach]

Comment on attachment 110418 [details]
Patch

r=me

[Dominic Mazzoni]

Created attachment 110420 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 110420 [details]
Patch for landing

Rejecting attachment 110420 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 110420 [details]
Patch for landing

Clearing flags on attachment: 110420

Committed r97128: <http://trac.webkit.org/changeset/97128>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.