Bug 105007 - [GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.
Summary: [GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation wit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Krzysztof Czech
URL:
Keywords:
: 105009 (view as bug list)
Depends on: 105009
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-14 03:04 PST by Krzysztof Czech
Modified: 2013-02-11 03:53 PST (History)
10 users (show)

See Also:


Attachments
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports. (88.53 KB, patch)
2012-12-14 03:20 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports. (90.13 KB, patch)
2013-01-16 05:15 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports. (79.76 KB, patch)
2013-01-22 04:21 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports. (70.16 KB, patch)
2013-01-25 08:21 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
[GTK][EFL] Isolates GTK's DumpRenderTree code. (21.92 KB, patch)
2013-01-25 08:23 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports. (69.83 KB, patch)
2013-01-30 01:21 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
[GTK][EFL] Isolates GTK's DumpRenderTree code. (21.92 KB, patch)
2013-01-30 01:30 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
Updated patch (90.55 KB, patch)
2013-02-04 03:53 PST, Krzysztof Czech
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (90.55 KB, patch)
2013-02-04 04:15 PST, Krzysztof Czech
mrobinson: review+
Details | Formatted Diff | Diff
Rebased patch (90.55 KB, patch)
2013-02-11 02:54 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2012-12-14 03:04:30 PST
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.
Comment 1 Krzysztof Czech 2012-12-14 03:20:32 PST
Created attachment 179461 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.
Comment 2 Mario Sanchez Prada 2012-12-14 04:30:03 PST
Looks good to me too, providing the patch is just about renaming files after applying patch for bug 105009 (and it seems so).
Comment 3 Krzysztof Czech 2013-01-16 05:15:08 PST
Created attachment 182962 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.
Comment 4 Krzysztof Czech 2013-01-22 04:21:40 PST
Created attachment 183963 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.
Comment 5 Krzysztof Czech 2013-01-25 07:41:33 PST
*** Bug 105009 has been marked as a duplicate of this bug. ***
Comment 6 Krzysztof Czech 2013-01-25 08:21:16 PST
Created attachment 184755 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.
Comment 7 Krzysztof Czech 2013-01-25 08:23:33 PST
Created attachment 184757 [details]
[GTK][EFL] Isolates GTK's DumpRenderTree code.
Comment 8 WebKit Review Bot 2013-01-25 08:27:19 PST
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 9 Martin Robinson 2013-01-25 08:31:35 PST
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.
Comment 10 Krzysztof Czech 2013-01-30 01:21:39 PST
Created attachment 185432 [details]
[GTK][EFL] Share WebKit-GTK's DumpRenderTree accessibility implementation with other Webkit ports.
Comment 11 Krzysztof Czech 2013-01-30 01:28:06 PST
(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.
Comment 12 Krzysztof Czech 2013-01-30 01:30:39 PST
Created attachment 185433 [details]
[GTK][EFL] Isolates GTK's DumpRenderTree code.
Comment 13 WebKit Review Bot 2013-01-30 01:32:49 PST
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.
Comment 14 Gyuyoung Kim 2013-01-31 01:43:01 PST
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 ?
Comment 15 Krzysztof Czech 2013-02-04 03:53:11 PST
Created attachment 186341 [details]
Updated patch
Comment 16 Krzysztof Czech 2013-02-04 03:54:19 PST
(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 17 EFL EWS Bot 2013-02-04 04:00:13 PST
Comment on attachment 186341 [details]
Updated patch

Attachment 186341 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16369177
Comment 18 Krzysztof Czech 2013-02-04 04:15:20 PST
Created attachment 186345 [details]
Updated patch
Comment 19 Gyuyoung Kim 2013-02-07 01:33:16 PST
Comment on attachment 186345 [details]
Updated patch

LGTM. I think GTK folks should sign off on this patch. Martin ?
Comment 20 Mario Sanchez Prada 2013-02-07 03:09:47 PST
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?
Comment 21 Krzysztof Czech 2013-02-08 02:20:46 PST

(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.
Comment 22 Krzysztof Czech 2013-02-08 02:25:55 PST
(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.
Comment 23 Mario Sanchez Prada 2013-02-08 02:28:42 PST
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
Comment 24 Krzysztof Czech 2013-02-11 02:54:40 PST
Created attachment 187535 [details]
Rebased patch
Comment 25 WebKit Review Bot 2013-02-11 03:53:21 PST
Comment on attachment 187535 [details]
Rebased patch

Clearing flags on attachment: 187535

Committed r142451: <http://trac.webkit.org/changeset/142451>
Comment 26 WebKit Review Bot 2013-02-11 03:53:28 PST
All reviewed patches have been landed.  Closing bug.