[webkit-reviews] review granted: [Bug 69676] [Chromium] Improve AccessibilityController and AccessibilityUIElement : [Attachment 110253] Patch

[bugzilla-daemon at webkit.org]

Dominic Mazzoni <dmazzoni at google.com> has granted Dominic Mazzoni
<dmazzoni at google.com>'s request for review:
Bug 69676: [Chromium] Improve AccessibilityController and
AccessibilityUIElement
https://bugs.webkit.org/show_bug.cgi?id=69676

Attachment 110253: Patch
https://bugs.webkit.org/attachment.cgi?id=110253&action=review

------- Additional Comments from Dominic Mazzoni <dmazzoni at google.com>
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?