Summary: | [EFL] Missing implementation of AccessibilityControllerEfl and AccessibilityUIElementEfl files | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Krzysztof Czech <k.czech> | ||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | gyuyoung.kim, lucas.de.marchi, mario, mrobinson, rakuco, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Krzysztof Czech
2013-02-22 03:41:20 PST
Accessibility support in EFL's DumpRenderTree implementation.
> Part of a task of adding support for Accessibility in DumpRenderTree.
Created attachment 189742 [details]
Patch
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. 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 ? Created attachment 190023 [details]
Patch
(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. (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. I'm unskipping whole LayoutTests/accessibility folder and skipping failure tests. Modified also DumpRenderTreeChrome file so that accessibilityController may look as a js object. Comment on attachment 190023 [details]
Patch
LGTM. But, GTK folks might wanna have final look.
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. Created attachment 191492 [details]
Patch
(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. (In reply to comment #11) > Created an attachment (id=191492) [details] > Patch Thanks for considering my comments. I have no other objections (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. Comment on attachment 191492 [details] Patch Clearing flags on attachment: 191492 Committed r145014: <http://trac.webkit.org/changeset/145014> All reviewed patches have been landed. Closing bug. |