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.
Created attachment 110253 [details] Patch
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
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?
(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
Created attachment 110418 [details] Patch
Comment on attachment 110418 [details] Patch r=me
Created attachment 110420 [details] Patch for landing
Comment on attachment 110420 [details] Patch for landing Rejecting attachment 110420 [details] from commit-queue. dmazzoni@google.com 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.
Comment on attachment 110420 [details] Patch for landing Clearing flags on attachment: 110420 Committed r97128: <http://trac.webkit.org/changeset/97128>
All reviewed patches have been landed. Closing bug.