Bug 110584 – [EFL] Missing implementation of AccessibilityControllerEfl and AccessibilityUIElementEfl files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Krzysztof Czech]

Part of a task of adding support for Accessibility in DumpRenderTree.

[Krzysztof Czech]

Accessibility support in EFL's DumpRenderTree implementation.
> Part of a task of adding support for Accessibility in DumpRenderTree.

[Krzysztof Czech]

Created attachment 189742 [details]
Patch

[Mario Sanchez Prada]

The patch looks good to me in general, but I wonder whether this could be a good moment to try to unskip some tests that might be passing now thanks to the implementation of this missing functionality in EFL's DRT.

If that is possible, I would say it would be great to include that tests unskipping along with this patch.

[Gyuyoung Kim]

Comment on attachment 189742 [details]
Patch

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

Patch looks good to me overall.

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:814
> +CString DumpRenderTreeSupportEfl::accessibilityHelpText(const AtkObject* axObject)

BTW, is there any reason to use CString instead of String ? DumpRenderTreeSupportEfl only exports String. So, I don't prefer to export new WebKit object for outer application.

> Tools/DumpRenderTree/efl/AccessibilityUIElementEfl.cpp:4
> + * Copyright (C) 2013 Samsung Electronics

Missing All Rights Reserved ?

[Krzysztof Czech]

Created attachment 190023 [details]
Patch

[Krzysztof Czech]

(In reply to comment #3)
> The patch looks good to me in general, but I wonder whether this could be a good moment to try to unskip some tests that might be passing now thanks to the implementation of this missing functionality in EFL's DRT.
> 
> If that is possible, I would say it would be great to include that tests unskipping along with this patch.
Sounds good. Unskipped LayoutTests/accessibility folder and skipping failure tests.

[Krzysztof Czech]

(In reply to comment #4)
> (From update of attachment 189742 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189742&action=review
> 
> Patch looks good to me overall.
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:814
> > +CString DumpRenderTreeSupportEfl::accessibilityHelpText(const AtkObject* axObject)
> 
> BTW, is there any reason to use CString instead of String ? DumpRenderTreeSupportEfl only exports String. So, I don't prefer to export new WebKit object for outer application.
> 
I think, there is no any specific reason. I already updated it.
> > Tools/DumpRenderTree/efl/AccessibilityUIElementEfl.cpp:4
> > + * Copyright (C) 2013 Samsung Electronics
> 
> Missing All Rights Reserved ?
Done.

[Krzysztof Czech]

I'm unskipping whole LayoutTests/accessibility folder and skipping failure tests.
Modified also DumpRenderTreeChrome file so that accessibilityController may look as a js object.

[Gyuyoung Kim]

Comment on attachment 190023 [details]
Patch

LGTM. But, GTK folks might wanna have final look.

[Mario Sanchez Prada]

Comment on attachment 190023 [details]
Patch

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

The patch looks good to me overall. Just a comment on the change in the TestExpectations file, see it below

> LayoutTests/ChangeLog:8
> +        Unskipping LayoutTests/accessibility folder.

I think this is not 100% accurate, since you're unskipping the folder, but skipping some tests anyway in an individual basis.

Also, I wonder whether it would not be better for the EFL port to mark those tests as "Failures" instead of just skipping them, so you can easily realize when they start passing in the bots.

[Krzysztof Czech]

Created attachment 191492 [details]
Patch

[Krzysztof Czech]

(In reply to comment #10)
> (From update of attachment 190023 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190023&action=review
> 
> The patch looks good to me overall. Just a comment on the change in the TestExpectations file, see it below
> 
> > LayoutTests/ChangeLog:8
> > +        Unskipping LayoutTests/accessibility folder.
> 
> I think this is not 100% accurate, since you're unskipping the folder, but skipping some tests anyway in an individual basis.
Yes, it may sound inaccurate.
> 
> Also, I wonder whether it would not be better for the EFL port to mark those tests as "Failures" instead of just skipping them, so you can easily realize when they start passing in the bots.
Sounds good.
I added failure ones and skipped only those with missing expected results.

[Mario Sanchez Prada]

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

Thanks for considering my comments. I have no other objections

[Gyuyoung Kim]

(In reply to comment #13)
> (In reply to comment #11)
> > Created an attachment (id=191492) [details] [details]
> > Patch
> 
> Thanks for considering my comments. I have no other objections

If there is no objection by today, I'd like to land this patch.

[WebKit Review Bot]

Comment on attachment 191492 [details]
Patch

Clearing flags on attachment: 191492

Committed r145014: <http://trac.webkit.org/changeset/145014>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.