Bug 105007 – [GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.

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

[Krzysztof Czech]

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.

[Krzysztof Czech]

Created attachment 179461 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.

[Mario Sanchez Prada]

Looks good to me too, providing the patch is just about renaming files after applying patch for bug 105009 (and it seems so).

[Krzysztof Czech]

Created attachment 182962 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.

[Krzysztof Czech]

Created attachment 183963 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.

[Krzysztof Czech]

*** Bug 105009 has been marked as a duplicate of this bug. ***

[Krzysztof Czech]

Created attachment 184755 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.

[Krzysztof Czech]

Created attachment 184757 [details]
[GTK][EFL] Isolates GTK's DumpRenderTree code.

[WebKit Review Bot]

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.

[Martin Robinson]

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.

[Krzysztof Czech]

Created attachment 185432 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.

[Krzysztof Czech]

(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.

[Krzysztof Czech]

Created attachment 185433 [details]
[GTK][EFL] Isolates GTK's DumpRenderTree code.

[WebKit Review Bot]

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.

[Gyuyoung Kim]

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 ?

[Krzysztof Czech]

Created attachment 186341 [details]
Updated patch

[Krzysztof Czech]

(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.

[EFL EWS Bot]

Comment on attachment 186341 [details]
Updated patch

Attachment 186341 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16369177

[Krzysztof Czech]

Created attachment 186345 [details]
Updated patch

[Gyuyoung Kim]

Comment on attachment 186345 [details]
Updated patch

LGTM. I think GTK folks should sign off on this patch. Martin ?

[Mario Sanchez Prada]

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?

[Krzysztof Czech]

(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.

[Krzysztof Czech]

(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.

[Mario Sanchez Prada]

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

[Krzysztof Czech]

Created attachment 187535 [details]
Rebased patch

[WebKit Review Bot]

Comment on attachment 187535 [details]
Rebased patch

Clearing flags on attachment: 187535

Committed r142451: <http://trac.webkit.org/changeset/142451>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.