Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1806001:
Add browser test to verify that MSAA clients can correctly retrieve accessibi... (Closed)
|
Created:
10 years, 8 months ago by Chris Guillory Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, Paweł Hajdan Jr., dtseng, tfarina (gmail-do not use) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd browser test to verify that MSAA clients can correctly retrieve accessibility tree from renderer.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46238
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 51
Patch Set 6 : '' #Patch Set 7 : '' #Messages
Total messages: 6 (0 generated)
This is ready for an initial look. I do want to make the test more extensive and evolve them over time.
This is great. I don't want you to add anything more to the test except the one suggestion at the end (to navigate away and test for E_FAIL). I just have some suggestions for naming classes and methods. Please check that in particular variable names match the Chrome style and not the Windows style. http://codereview.chromium.org/1806001/diff/13001/14002 File chrome/browser/accessibility_win_browsertest.cc (right): http://codereview.chromium.org/1806001/diff/13001/14002#newcode26 chrome/browser/accessibility_win_browsertest.cc:26: void GetRenderWidgetHostViewClientAccessible(IAccessible** ppAccessible); How about having the method return IAccessible*? http://codereview.chromium.org/1806001/diff/13001/14002#newcode33 chrome/browser/accessibility_win_browsertest.cc:33: class AccessibleInfo { Call this AccessibleChecker http://codereview.chromium.org/1806001/diff/13001/14002#newcode35 chrome/browser/accessibility_win_browsertest.cc:35: AccessibleInfo(std::wstring name_, int32 role_); Call these arguments expected_name and expected_role. They shouldn't end in an underscore. Instead of a second constructor, how about an AddExpectedChild method? I think it will be simpler. http://codereview.chromium.org/1806001/diff/13001/14002#newcode39 chrome/browser/accessibility_win_browsertest.cc:39: void TestAccessible(IAccessible* pAccessible); Call this CheckAccessible. Comment this method: Check that the name and role of the given IAccessible instance and its descendants match the expected names and roles that this object was initialized with. http://codereview.chromium.org/1806001/diff/13001/14002#newcode46 chrome/browser/accessibility_win_browsertest.cc:46: void GetAccessibleFromAccessibleChildrenResultArray( Maybe have this return IAccessible* (whenever there's only one return arg) http://codereview.chromium.org/1806001/diff/13001/14002#newcode49 chrome/browser/accessibility_win_browsertest.cc:49: VARIANT Variant_I4(LONG lVal) { Maybe call this CreateI4Variant. http://codereview.chromium.org/1806001/diff/13001/14002#newcode70 chrome/browser/accessibility_win_browsertest.cc:70: HWND hwndRenderWidgetHostView = nit: lowercase and underscores instead of camelCase for variable names http://codereview.chromium.org/1806001/diff/13001/14002#newcode71 chrome/browser/accessibility_win_browsertest.cc:71: browser()-> Nit: indent 4 spaces for line continuation of any kind http://codereview.chromium.org/1806001/diff/13001/14002#newcode75 chrome/browser/accessibility_win_browsertest.cc:75: AccessibleObjectFromWindow( Nit: put on previous line, then indent the next line 4 spaces http://codereview.chromium.org/1806001/diff/13001/14002#newcode79 chrome/browser/accessibility_win_browsertest.cc:79: ASSERT_NE(*ppAccessible, reinterpret_cast<IAccessible*>(NULL)); reinterpret_cast isn't necessary for NULL, I think. http://codereview.chromium.org/1806001/diff/13001/14002#newcode83 chrome/browser/accessibility_win_browsertest.cc:83: std::wstring name, int32 role) : Nit: this will fit on the previous line, but put : on the next line, indented 4 spaces http://codereview.chromium.org/1806001/diff/13001/14002#newcode185 chrome/browser/accessibility_win_browsertest.cc:185: // AccessibleInfo textboxInfo(L"", ROLE_SYSTEM_TEXT); Did this one not work? http://codereview.chromium.org/1806001/diff/13001/14002#newcode201 chrome/browser/accessibility_win_browsertest.cc:201: rootAccessibleInfo.TestAccessible(pRootAccessible); How about one more quick test at the end: navigate to a different url (you can use "about:" and test that the IAccessible returns E_FAIL.
http://codereview.chromium.org/1806001/diff/13001/14002 File chrome/browser/accessibility_win_browsertest.cc (right): http://codereview.chromium.org/1806001/diff/13001/14002#newcode20 chrome/browser/accessibility_win_browsertest.cc:20: public: Drive-by review: how about adding a comment here with a pointer to the bug which (when resolved) will enable us to remove this flag? http://codereview.chromium.org/1806001/diff/13001/14002#newcode168 chrome/browser/accessibility_win_browsertest.cc:168: FAIL(); Drive-by review (nit): no need to break in the default case. http://codereview.chromium.org/1806001/diff/13001/14002#newcode202 chrome/browser/accessibility_win_browsertest.cc:202: } Drive-by review: how about also checking parent?
http://codereview.chromium.org/1806001/diff/13001/14002 File chrome/browser/accessibility_win_browsertest.cc (right): http://codereview.chromium.org/1806001/diff/13001/14002#newcode31 chrome/browser/accessibility_win_browsertest.cc:31: typedef std::vector<AccessibleInfo*> AccessibleInfoVector; nit: I would prefer to move this into the class. http://codereview.chromium.org/1806001/diff/13001/14002#newcode61 chrome/browser/accessibility_win_browsertest.cc:61: std::wstring name_; nit: could you add a comment about of what this name is? http://codereview.chromium.org/1806001/diff/13001/14002#newcode70 chrome/browser/accessibility_win_browsertest.cc:70: HWND hwndRenderWidgetHostView = On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > nit: lowercase and underscores instead of camelCase for variable names > Also there are other cases below too. http://codereview.chromium.org/1806001/diff/13001/14002#newcode88 chrome/browser/accessibility_win_browsertest.cc:88: std::wstring name, int32 role, AccessibleInfoVector& children) : nit: please, put : on the next line. Also could you initialize each variable per line? http://codereview.chromium.org/1806001/diff/13001/14002#newcode102 chrome/browser/accessibility_win_browsertest.cc:102: if (name_.size() == 0) { nit: !name_.empty() ? http://codereview.chromium.org/1806001/diff/13001/14002#newcode125 chrome/browser/accessibility_win_browsertest.cc:125: get_accChildCount(reinterpret_cast<long*>(&childCount)); nit: indent just 4 spaces. http://codereview.chromium.org/1806001/diff/13001/14002#newcode139 chrome/browser/accessibility_win_browsertest.cc:139: itChildAccessibleInfo++, pChild++) { nit: ++it..., ++pChild? http://codereview.chromium.org/1806001/diff/13001/14002#newcode141 chrome/browser/accessibility_win_browsertest.cc:141: nit: remove this blank line. http://codereview.chromium.org/1806001/diff/13001/14002#newcode194 chrome/browser/accessibility_win_browsertest.cc:194: AccessibleInfo* rootChildren[] = {&groupingAccessibleInfo}; nit: this is odd; :) http://codereview.chromium.org/1806001/diff/13001/14002#newcode203 chrome/browser/accessibility_win_browsertest.cc:203: } // Namespace. nit: just // namespace
Thanks for the reviews. http://codereview.chromium.org/1806001/diff/13001/14002 File chrome/browser/accessibility_win_browsertest.cc (right): http://codereview.chromium.org/1806001/diff/13001/14002#newcode20 chrome/browser/accessibility_win_browsertest.cc:20: public: On 2010/04/29 18:06:38, Jonas Klink (Google) wrote: > Drive-by review: how about adding a comment here with a pointer to the bug which > (when resolved) will enable us to remove this flag? Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode26 chrome/browser/accessibility_win_browsertest.cc:26: void GetRenderWidgetHostViewClientAccessible(IAccessible** ppAccessible); On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > How about having the method return IAccessible*? > I originally has this function doing that but it didn't compile using the gtest ASSERT_EQ macro since it wants to return on failure. I can use EXPECT_EQ which doesn't return on failure and return the IAccessible*. http://codereview.chromium.org/1806001/diff/13001/14002#newcode31 chrome/browser/accessibility_win_browsertest.cc:31: typedef std::vector<AccessibleInfo*> AccessibleInfoVector; On 2010/04/29 20:59:56, tfarina wrote: > nit: I would prefer to move this into the class. Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode33 chrome/browser/accessibility_win_browsertest.cc:33: class AccessibleInfo { On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Call this AccessibleChecker Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode35 chrome/browser/accessibility_win_browsertest.cc:35: AccessibleInfo(std::wstring name_, int32 role_); On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Call these arguments expected_name and expected_role. They shouldn't end in an > underscore. > > Instead of a second constructor, how about an AddExpectedChild method? I think > it will be simpler. > Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode39 chrome/browser/accessibility_win_browsertest.cc:39: void TestAccessible(IAccessible* pAccessible); On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Call this CheckAccessible. Comment this method: Check that the name and role of > the given IAccessible instance and its descendants match the expected names and > roles that this object was initialized with. Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode46 chrome/browser/accessibility_win_browsertest.cc:46: void GetAccessibleFromAccessibleChildrenResultArray( On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Maybe have this return IAccessible* (whenever there's only one return arg) Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode49 chrome/browser/accessibility_win_browsertest.cc:49: VARIANT Variant_I4(LONG lVal) { On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Maybe call this CreateI4Variant. Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode61 chrome/browser/accessibility_win_browsertest.cc:61: std::wstring name_; On 2010/04/29 20:59:56, tfarina wrote: > nit: could you add a comment about of what this name is? Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode70 chrome/browser/accessibility_win_browsertest.cc:70: HWND hwndRenderWidgetHostView = On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > nit: lowercase and underscores instead of camelCase for variable names > Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode70 chrome/browser/accessibility_win_browsertest.cc:70: HWND hwndRenderWidgetHostView = On 2010/04/29 20:59:56, tfarina wrote: > On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > > nit: lowercase and underscores instead of camelCase for variable names > > > Also there are other cases below too. > Tried to tackle all these. http://codereview.chromium.org/1806001/diff/13001/14002#newcode71 chrome/browser/accessibility_win_browsertest.cc:71: browser()-> On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Nit: indent 4 spaces for line continuation of any kind Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode75 chrome/browser/accessibility_win_browsertest.cc:75: AccessibleObjectFromWindow( On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Nit: put on previous line, then indent the next line 4 spaces Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode79 chrome/browser/accessibility_win_browsertest.cc:79: ASSERT_NE(*ppAccessible, reinterpret_cast<IAccessible*>(NULL)); On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > reinterpret_cast isn't necessary for NULL, I think. It gives me a compile error if I don't do it. http://codereview.chromium.org/1806001/diff/13001/14002#newcode83 chrome/browser/accessibility_win_browsertest.cc:83: std::wstring name, int32 role) : On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Nit: this will fit on the previous line, but put : on the next line, indented 4 > spaces Doesn't fit anymore. Indented 4. http://codereview.chromium.org/1806001/diff/13001/14002#newcode88 chrome/browser/accessibility_win_browsertest.cc:88: std::wstring name, int32 role, AccessibleInfoVector& children) : On 2010/04/29 20:59:56, tfarina wrote: > nit: please, put : on the next line. Also could you initialize each variable per > line? Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode102 chrome/browser/accessibility_win_browsertest.cc:102: if (name_.size() == 0) { On 2010/04/29 20:59:56, tfarina wrote: > nit: !name_.empty() ? I think you mean name_.empty(). Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode125 chrome/browser/accessibility_win_browsertest.cc:125: get_accChildCount(reinterpret_cast<long*>(&childCount)); On 2010/04/29 20:59:56, tfarina wrote: > nit: indent just 4 spaces. Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode139 chrome/browser/accessibility_win_browsertest.cc:139: itChildAccessibleInfo++, pChild++) { On 2010/04/29 20:59:56, tfarina wrote: > nit: ++it..., ++pChild? Done. But why? Convention? http://codereview.chromium.org/1806001/diff/13001/14002#newcode141 chrome/browser/accessibility_win_browsertest.cc:141: On 2010/04/29 20:59:56, tfarina wrote: > nit: remove this blank line. Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode168 chrome/browser/accessibility_win_browsertest.cc:168: FAIL(); On 2010/04/29 18:06:38, Jonas Klink (Google) wrote: > Drive-by review (nit): no need to break in the default case. This default case is removed now that I am returning an IAccessible*. This is a habit for me. http://codereview.chromium.org/1806001/diff/13001/14002#newcode185 chrome/browser/accessibility_win_browsertest.cc:185: // AccessibleInfo textboxInfo(L"", ROLE_SYSTEM_TEXT); On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > Did this one not work? Correct. The input text control has nested controls. Through debugging I see an AccessibleRenderObject tree with this WebCore node tree: HTMLInputElement TextControlInnerTextElement WebCore::Text Perhaps I can just verify button and checkbox for the initial check-in. http://codereview.chromium.org/1806001/diff/13001/14002#newcode194 chrome/browser/accessibility_win_browsertest.cc:194: AccessibleInfo* rootChildren[] = {&groupingAccessibleInfo}; On 2010/04/29 20:59:56, tfarina wrote: > nit: this is odd; :) Removed. Using new function AddpendExpectedChild. http://codereview.chromium.org/1806001/diff/13001/14002#newcode201 chrome/browser/accessibility_win_browsertest.cc:201: rootAccessibleInfo.TestAccessible(pRootAccessible); On 2010/04/29 13:15:47, Dominic Mazzoni wrote: > How about one more quick test at the end: navigate to a different url (you can > use "about:" and test that the IAccessible returns E_FAIL. > Done. http://codereview.chromium.org/1806001/diff/13001/14002#newcode203 chrome/browser/accessibility_win_browsertest.cc:203: } // Namespace. On 2010/04/29 20:59:56, tfarina wrote: > nit: just // namespace Done.
LGTM. |
