aria-haspopup indicates that the element has a popup context menu or sub-level menu.
Created attachment 205887 [details] Patch
Comment on attachment 205887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205887&action=review > Tools/ChangeLog:13 > + * WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp: > + (WTR::attributeSetValueById): > + (WTR::AccessibilityUIElement::stringAttributeValue): > + (WTR::AccessibilityUIElement::hasPopup): I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1 > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1098 > + if (attributeValue.isNull()) > + return false; > + > + return attributeValue == "true" ? true : false; I would probably write this way: if (attributeValue.isNull() || attributeValue == "false") return false; return true; ... but I'm fine with how it is now anyway. Your call.
(In reply to comment #2) > (From update of attachment 205887 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205887&action=review > > > Tools/ChangeLog:13 > > + * WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp: > > + (WTR::attributeSetValueById): > > + (WTR::AccessibilityUIElement::stringAttributeValue): > > + (WTR::AccessibilityUIElement::hasPopup): > > I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1 Ohh I forgot, you are right, I'll do it. > > > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1098 > > + if (attributeValue.isNull()) > > + return false; > > + > > + return attributeValue == "true" ? true : false; > > I would probably write this way: > > if (attributeValue.isNull() || attributeValue == "false") > return false; > > return true; > > ... but I'm fine with how it is now anyway. Your call. Nice hint, sounds good to me as well, I'll change it.
Created attachment 205976 [details] Patch
Attachment 205976 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 205978 [details] Patch
> I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1 WK1 support, done. > > I would probably write this way: > > if (attributeValue.isNull() || attributeValue == "false") > return false; > > return true; > > ... but I'm fine with how it is now anyway. Your call. How about just return return attributeValue == "true" ? true : false; I guess operator overloading for String will take care in case of nulls
(In reply to comment #7) > > I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1 > > WK1 support, done. Cool, thanks > > I would probably write this way: > > > > if (attributeValue.isNull() || attributeValue == "false") > > return false; > > > > return true; > > > > ... but I'm fine with how it is now anyway. Your call. > How about just return return attributeValue == "true" ? true : false; > I guess operator overloading for String will take care in case of nulls As I said, I'm fine with that too. Just sharing how I would probably do it, not necessarily meaning it's the best way to do it :). In any case, if you're not checking nulls explicitly anyway then you could reduce it even more to a single statement: return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true";
Created attachment 205982 [details] Patch
> In any case, if you're not checking nulls explicitly anyway then you could reduce it even more to a single statement: > > return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true"; This one look very nice, thanks.
Hello Chris, I already have an informal review of this patch. Could look at this, thanks.
Comment on attachment 205982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205982&action=review > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:41 > +static void attributesClear(AtkAttributeSet* attributesSet) this method should probably be named "clearAttributes()" > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:319 > +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id) this method seems like it should be named attributeValueForId after reading through what it does > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:326 > + attributeValue.set(g_strdup(attribute->value)); Do you need to copy this to a GOwnPtr, or can you just return directly from here with return String(g_strdup(attribute->value)) > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1038 > + return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element)), "aria-haspopup") == "true"; you probably want to do an equalIgnoringCase in case someone does aria-haspop=True > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:83 > +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id) ditto > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1093 > + return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true"; ditto
Created attachment 206358 [details] Patch
Rebased after r152397
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:319 > > +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id) > > this method seems like it should be named attributeValueForId after reading through what it does Yes, you are right, that sounds more accurate. > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:326 > > + attributeValue.set(g_strdup(attribute->value)); > > Do you need to copy this to a GOwnPtr, or can you just return directly from here > > with return String(g_strdup(attribute->value)) I cannot directly return, I must clear AtkAttributeSet first. > you probably want to do an equalIgnoringCase in case someone does aria-haspop=True Yes, you are right Thanks, applied all suggestions.
Comment on attachment 206358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206358&action=review > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:59 > + for (AtkAttributeSet* attributes = attributeSet; attributes; attributes = attributes->next) { attributes -> attribute ? > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:62 > + attributeValue.set(g_strdup(atkAttribute->value)); The strdup would not be needed if you freed the attributeSet list *after* constructing the return String. attributeValue would then be a const char*. > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:68 > + return String(attributeValue.get()); String::fromUTF8(attributeValue)? iirc the string string is UTF-8 encoded, right? > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:64 > + attributeValue.set(g_strdup(atkAttribute->value)); Same comments as above.
Created attachment 206517 [details] Patch
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:59 > > + for (AtkAttributeSet* attributes = attributeSet; attributes; attributes = attributes->next) { > > attributes -> attribute ? I named it attributes because, it describes range of ATK attributes. > > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:62 > > + attributeValue.set(g_strdup(atkAttribute->value)); > > The strdup would not be needed if you freed the attributeSet list *after* constructing the return String. attributeValue would then be a const char*. > I agree, done. > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:68 > > + return String(attributeValue.get()); > > String::fromUTF8(attributeValue)? iirc the string string is UTF-8 encoded, right? > I guess you are right, corrected > > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:64 > > + attributeValue.set(g_strdup(atkAttribute->value)); > > Same comments as above.
Thanks for review.
Comment on attachment 206517 [details] Patch Attachment 206517 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1063080 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Created attachment 206525 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Created attachment 206526 [details] Patch
Comment on attachment 206526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206526&action=review > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:54 > +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id) This could take a const char* in argument and return a CString. It does not look like we need a String at all (inside the function or the callers). Also, I find the name a bit confusing because it sounds like it 'sets' something. How about getAttributeSetValueForId() ? > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56 > +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id) Same comments as for DRT.
Created attachment 206741 [details] Patch
Created attachment 206742 [details] Patch
Attachment 206742 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/element-haspopup-expected.txt', u'LayoutTests/accessibility/element-haspopup.html', u'LayoutTests/accessibility/popup-button-title-expected.txt', u'LayoutTests/accessibility/popup-button-title.html', u'LayoutTests/platform/mac/accessibility/element-haspopup-expected.txt', u'LayoutTests/platform/mac/accessibility/element-haspopup.html', u'LayoutTests/platform/mac/accessibility/popup-button-title-expected.txt', u'LayoutTests/platform/mac/accessibility/popup-button-title.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:496: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 206744 [details] Patch
> > This could take a const char* in argument and return a CString. It does not look like we need a String at all (inside the function or the callers). There's one place when I need return String. It's an equalIgnoringCase function. It does not take two raw chars. I will have to convert one of those params to String. > > Also, I find the name a bit confusing because it sounds like it 'sets' something. How about getAttributeSetValueForId() ? Sounds good to me. > > > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56 > > +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id) > Done
Comment on attachment 206744 [details] Patch Looks fine. r=me.
Comment on attachment 206744 [details] Patch Clearing flags on attachment: 206744 Committed r152716: <http://trac.webkit.org/changeset/152716>
All reviewed patches have been landed. Closing bug.