This is a continuation of 99011 bug. We started there sharing Accessibility bits related to WebKitTestRunner.
I think we can do the same in DumpRenderTree.
Attachment 184757[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityCallbacks.h', u'Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp', u'Tools/DumpRenderTree/efl/CMakeLists.txt', u'Tools/DumpRenderTree/gtk/AccessibilityCallbacks.cpp', u'Tools/DumpRenderTree/gtk/AccessibilityCallbacks.h', u'Tools/GNUmakefile.am']" exit_code: 1
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:75: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:79: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:81: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:91: signal_query is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:108: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 6 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 184755[details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.
View in context: https://bugs.webkit.org/attachment.cgi?id=184755&action=review
Looks good to me.
> Tools/ChangeLog:8
>
> Reviewed by NOBODY (OOPS!).
>
> + Shares specific ATK's accessibility implementation.
Your ChangeLog seems corrupt here. You probably won't be able to use the commit queue because of this.
(In reply to comment #9)
> (From update of attachment 184755[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184755&action=review
>
> Looks good to me.
>
> > Tools/ChangeLog:8
> >
> > Reviewed by NOBODY (OOPS!).
> >
> > + Shares specific ATK's accessibility implementation.
>
> Your ChangeLog seems corrupt here. You probably won't be able to use the commit queue because of this.
Corrected.
Attachment 185433[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityCallbacks.h', u'Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp', u'Tools/DumpRenderTree/efl/CMakeLists.txt', u'Tools/DumpRenderTree/gtk/AccessibilityCallbacks.cpp', u'Tools/DumpRenderTree/gtk/AccessibilityCallbacks.h', u'Tools/GNUmakefile.am']" exit_code: 1
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:75: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:79: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:81: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:91: signal_query is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:108: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 6 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
I think you don't need to divide this patch into two patches. This patch is just to move gtk files to atk. As far as I know, WebKit prefers to land one patch per bug.
BTW, How about fixing style error together ?
(In reply to comment #14)
> I think you don't need to divide this patch into two patches. This patch is just to move gtk files to atk. As far as I know, WebKit prefers to land one patch per bug.
>
> BTW, How about fixing style error together ?
Done.
I merged both previous patches into one and fixed style errors.
Comment on attachment 186345[details]
Updated patch
View in context: https://bugs.webkit.org/attachment.cgi?id=186345&action=review
Informal GTK review here: lgtm too. Just a couple of comments below (probably won't mean a single change anyway)
> Tools/ChangeLog:145
> + * DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:
> + (AccessibilityController::focusedElement):
> + (AccessibilityController::rootElement):
> + (AccessibilityController::accessibleElementById):
Just a comment: I guess you're leaving these three out of the merge because they are implemented in terms of DumpRenderTreeSupportGtk and so you plan to add EFL equivalent functions in the future in a DumpRenderTree/efl/AccessibilityControllerEfl.cpp file, right?
With regard to this, this makes me think that DumpRenderTreeSupportGtk might be yet another good candidate to be split in different files, in order to share implementation with other ports like EFL. At least the implementation of getRootElement() and getFocusedElement() should be the same, I think. But this is just a rant for another moment :)
> Tools/DumpRenderTree/efl/CMakeLists.txt:2
> + ${TOOLS_DIR}/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp
Stupid CMake newbie question: Don't you need to add here AccessibilityControllerAtk.cpp and AccessibilityUIElementAtk.cpp too?
(In reply to comment #20)
> (From update of attachment 186345[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186345&action=review
>
> Informal GTK review here: lgtm too. Just a couple of comments below (probably won't mean a single change anyway)
>
> > Tools/ChangeLog:145
> > + * DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:
> > + (AccessibilityController::focusedElement):
> > + (AccessibilityController::rootElement):
> > + (AccessibilityController::accessibleElementById):
>
Just a comment: I guess you're leaving these three out of the merge because > they are implemented in terms of DumpRenderTreeSupportGtk and so you plan to add EFL equivalent functions in the future in a > > > > > DumpRenderTree/efl/AccessibilityControllerEfl.cpp file, right?
> Yes, exactly, that's my intention.> With regard to this, this makes me think that DumpRenderTreeSupportGtk might be yet another good candidate to be split in different files, in order to share implementation with other ports like EFL. At least the implementation of getRootElement() and getFocusedElement() should be the same, I think. But this is just a rant for another moment :)> I already provided implementation to EFL of those two methods. Indeed, sharing specific bits from DumpRenderTreeSupportGtk regarding ATK sounds great to me.>
> > Tools/DumpRenderTree/efl/CMakeLists.txt:2
> > + ${TOOLS_DIR}/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp
>
> Stupid CMake newbie question: Don't you need to add here AccessibilityControllerAtk.cpp and AccessibilityUIElementAtk.cpp too?> Yes, You are right, but I thought about to add them along with respective implementation of AccessibilityControllerEfl.cpp and AccessibilityUIElementEfl.cpp, which is almost finished.
(In reply to comment #21)
>
> (In reply to comment #20)
> > (From update of attachment 186345[details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186345&action=review
> >
> > Informal GTK review here: lgtm too. Just a couple of comments below (probably won't mean a single change anyway)
> >
> > > Tools/ChangeLog:145
> > > + * DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:
> > > + (AccessibilityController::focusedElement):
> > > + (AccessibilityController::rootElement):
> > > + (AccessibilityController::accessibleElementById):
> >
> Just a comment: I guess you're leaving these three out of the merge because > > they are implemented in terms of DumpRenderTreeSupportGtk and so you plan to > add EFL equivalent functions in the future in a > > > > > > DumpRenderTree/efl/AccessibilityControllerEfl.cpp file, right?
Yes, exactly, that's my intention.
>
> > With regard to this, this makes me think that DumpRenderTreeSupportGtk > > > might be yet another good candidate to be split in different files, in order > to share implementation with other ports like EFL. At least the > implementation of > getRootElement() and getFocusedElement() should be the > > same, I think. But this > is just a rant for another moment :)
I already provided implementation to EFL of those two methods. Indeed, sharing specific bits from DumpRenderTreeSupportGtk regarding ATK sounds great to me.
>
> >
> > > Tools/DumpRenderTree/efl/CMakeLists.txt:2
> > > + ${TOOLS_DIR}/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp
> >
> > Stupid CMake newbie question: Don't you need to add here AccessibilityControllerAtk.cpp and AccessibilityUIElementAtk.cpp too?
>
Yes, You are right, but I thought about to add them along with respective implementation of AccessibilityControllerEfl.cpp and AccessibilityUIElementEfl.cpp, which is almost finished.
Thanks for answering my questions. Unless I'm missing something not obvious, the patch lgtm 100% now.
(In reply to comment #22)
> [...]
> > > > Tools/DumpRenderTree/efl/CMakeLists.txt:2
> > > > + ${TOOLS_DIR}/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp
> > >
> > > Stupid CMake newbie question: Don't you need to add here
> > > AccessibilityControllerAtk.cpp and AccessibilityUIElementAtk.cpp too?
> >
> Yes, You are right, but I thought about to add them along with respective
> implementation of AccessibilityControllerEfl.cpp and
> AccessibilityUIElementEfl.cpp, which is almost finished.
Sounds good to me too
2012-12-14 03:20 PST, Krzysztof Czech
2013-01-16 05:15 PST, Krzysztof Czech
2013-01-22 04:21 PST, Krzysztof Czech
2013-01-25 08:21 PST, Krzysztof Czech
2013-01-25 08:23 PST, Krzysztof Czech
2013-01-30 01:21 PST, Krzysztof Czech
2013-01-30 01:30 PST, Krzysztof Czech
2013-02-04 03:53 PST, Krzysztof Czech
2013-02-04 04:15 PST, Krzysztof Czech
2013-02-11 02:54 PST, Krzysztof Czech