[GTK] accessibility/aria-invalid.html times out

[Zan Dobersek]

The test times out on all GTK builders:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=accessibility%2Faria-invalid.html

[Anton Obzhirov]

Created attachment 208804 [details]
WIP patch

It depends on addNotificationListener implementation.

[Mario Sanchez Prada]

Comment on attachment 208804 [details]
WIP patch

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

> LayoutTests/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * platform/gtk/TestExpectations:

You need a small description here

> Source/WebCore/ChangeLog:13
> +        * accessibility/atk/AXObjectCacheAtk.cpp:
> +        (WebCore::AXObjectCache::postPlatformNotification):
> +        * accessibility/atk/WebKitAccessibleWrapperAtk.cpp:
> +        (webkitAccessibleGetAttributes):

I know not everyone does it (even though it's in the guidelines), but I normally like to have shorts descriptions for every function touched in the ChangeLog.

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:169
> +    } else if (notification == AXInvalidStatusChanged) {
> +        g_signal_emit_by_name(axObject, "state-change", "invalid-entry", coreObject->invalidStatus() != "false");
>      }

You do not need { } if it's a one line branch

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:898
> +
> +    // In case of 'aria-invalid' when the attribute empty or has "false" for ATK
> +    // according to http://www.w3.org/WAI/PF/aria-implementation/#mapping attribute
> +    // is not mapped but layout tests will expect 'false'.
> +    if (attributeValue.isEmpty() && atkAttributeName == "aria-invalid")
> +        return JSStringCreateWithUTF8CString("false");
> +

You probably need to add this aria-invalid specific mapping to WKTR too

[Anton Obzhirov]

Comment on attachment 208804 [details]
WIP patch

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

>> LayoutTests/ChangeLog:7
>> +        * platform/gtk/TestExpectations:
> 
> You need a small description here

ok

>> Source/WebCore/ChangeLog:13
>> +        (webkitAccessibleGetAttributes):
> 
> I know not everyone does it (even though it's in the guidelines), but I normally like to have shorts descriptions for every function touched in the ChangeLog.

ok

>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:169
>>      }
> 
> You do not need { } if it's a one line branch

ok

>> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:898
>> +
> 
> You probably need to add this aria-invalid specific mapping to WKTR too

will do

[Anton Obzhirov]

Created attachment 209982 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 209982 [details]
Patch

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

(In reply to comment #4)
> Created an attachment (id=209982) [details]
> Patch

Looks good to me now (and after SVN revision r154697 the addNotificationListener implementation is no longer an issue in DRT. However, I believe you will need to keep the aria-invalid test skipped for WK2 only since we still don't have addNotificationListener for AccessibilityUIElement in WKTR, hence it will still timeout there.

> LayoutTests/platform/gtk/TestExpectations:-783
> -webkit.org/b/98350 accessibility/aria-invalid.html [ Timeout ]

I would suggest to move this line to the TestExpectations file in gtk-wk2 and reference it against bug 120421 (which tracks the missing implementation of addNotificationListner in WKTR

[Anton Obzhirov]

Created attachment 210286 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 210286 [details]
Patch

Thanks for the quick response. LGTM now.

[WebKit Commit Bot]

Comment on attachment 210286 [details]
Patch

Clearing flags on attachment: 210286

Committed r154960: <http://trac.webkit.org/changeset/154960>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.