Bug 101748 – [WK2][GTK][EFL] Share WebKit2-GTK's WebProcess Accessibility implementation with other WebKit ports.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

Remove and rename files related to Accessibility from WebPage/gtk to WebPage/atk.

[Krzysztof Czech]

Created attachment 173293 [details]
[EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

[Krzysztof Czech]

Sharing the WebCore's Accessibility, I thought that, some bits from WebProcess part can also be shared.

[Gyuyoung Kim]

Comment on attachment 173293 [details]
[EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

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

> Source/WebKit2/ChangeLog:3
> +        [EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

I think this is not related to [EFL] port patch. I think [WK2] is proper for this patch.

[WebKit Review Bot]

Attachment 173293 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.h:26:  #ifndef header guard has wrong style, please use: WebPageAccessibilityObjectAtk_h  [build/header_guard] [5]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.h:56:  web_page_accessibility_object_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:107:  web_page_accessibility_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:111:  web_page_accessibility_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Created attachment 173889 [details]
[WK2] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

[WebKit Review Bot]

Attachment 173889 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.h:26:  #ifndef header guard has wrong style, please use: WebPageAccessibilityObjectAtk_h  [build/header_guard] [5]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.h:56:  web_page_accessibility_object_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:107:  web_page_accessibility_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:111:  web_page_accessibility_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

This patch touches WK2-GTK implementation. How about changing the title to: [WK2][GTK][EFL] ..

There is style error which is connected witch the proposed changes:
#ifndef header guard has wrong style, please use: WebPageAccessibilityObjectAtk_h

IMHO, other style errors could be fixed in this bug according to WebKit coding style (only 3 left). It will encourage the reviewers to take a look at the patch.

[Krzysztof Czech]

(In reply to comment #7)
> This patch touches WK2-GTK implementation. How about changing the title to: [WK2][GTK][EFL] ..
I think is sounds reasonable. Done
> 
> There is style error which is connected witch the proposed changes:
> #ifndef header guard has wrong style, please use: WebPageAccessibilityObjectAtk_h
Done.
> 
> IMHO, other style errors could be fixed in this bug according to WebKit coding style (only 3 left). It will encourage the reviewers to take a look at the patch.

I think it is glib's ecosystem naming convention so probably it should be left as it is.

[Krzysztof Czech]

Created attachment 174168 [details]
[WK2][GTK][EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

[Martin Robinson]

(In reply to comment #8)

> I think it is glib's ecosystem naming convention so probably it should be left as it is.

At some point the GTK+ port switched to only using GLib naming conventions in public headers.

[WebKit Review Bot]

Attachment 174168 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.h:56:  web_page_accessibility_object_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:107:  web_page_accessibility_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:111:  web_page_accessibility_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

(In reply to comment #10)
> (In reply to comment #8)
> 
> > I think it is glib's ecosystem naming convention so probably it should be left as it is.
> 
> At some point the GTK+ port switched to only using GLib naming conventions in public headers.

Thanks for the clarifications. LGTM.

[Krzysztof Czech]

This patch eliminates some code duplication in terms of Accessibility (see 99578)

Ping reviewers ?.

[Martin Robinson]

Comment on attachment 174168 [details]
[WK2][GTK][EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

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

> Source/WebKit2/GNUmakefile.list.am:1148
> +    Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.h \
> +    Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp \

It seems to maintain the alphabetical ordering, these should go above Source/WebKit2/WebProcess/WebPage/gtk/WebInspectorGtk.cpp.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:33
> +#include "WebPageAccessibilityObjectAtk.h"

It makes sense to keep the header name as WebPageAccessibilityObject.h since that's the name of GObject.

[Krzysztof Czech]

Created attachment 174637 [details]
[WK2][GTK][EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

[WK2][GTK][EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

[Krzysztof Czech]

Created attachment 174638 [details]
[WK2][GTK][EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

[Krzysztof Czech]

(In reply to comment #14)
> (From update of attachment 174168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174168&action=review
> 
> > Source/WebKit2/GNUmakefile.list.am:1148
> > +    Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.h \
> > +    Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp \
> 
> It seems to maintain the alphabetical ordering, these should go above Source/WebKit2/WebProcess/WebPage/gtk/WebInspectorGtk.cpp.
Done.
> 
> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:33
> > +#include "WebPageAccessibilityObjectAtk.h"
> 
> It makes sense to keep the header name as WebPageAccessibilityObject.h since that's the name of GObject.
Done.

[WebKit Review Bot]

Attachment 174638 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:107:  web_page_accessibility_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:111:  web_page_accessibility_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObject.h:56:  web_page_accessibility_object_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

Comment on attachment 174638 [details]
[WK2][GTK][EFL] Share WebKit2-GTK's Accessibility implementation with other WebKit ports.

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

The changes look good to me. Actually the only comment I can make about them is about fixing an issue with naming I'm guilty of :/

Additionally, besides the code in this patch (which is about the AtkPlug part in the Web process only), I wonder if it could be possible to share the bits in WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.[cpp|h] as well, as they implement the other side of this thing (the AtkSocket part in the UI process).

Or maybe you were already thinking of doing that in a separate bug?

> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:60
> +    Page* page = accessible->m_page->corePage();
> +    if (!page)
> +        return 0;
> +
> +    Frame* core = page->mainFrame();
> +    if (!core || !core->document())
> +        return 0;
> +
> +    AccessibilityObject* root = core->document()->axObjectCache()->rootObject();
> +    if (!root)
> +        return 0;
> +
> +    AtkObject* axRoot = root->wrapper();

I know I'm the one to blame for this in the first place, but perhaps this patch is a good moment to use more meaningful names here :)

  page -> corePage
  core -> coreFrame
  root -> coreRootObject
  axRoot -> rootObject (to be consistent with the rest of this file)

... or something like that.

[Krzysztof Czech]

(In reply to comment #19)
> (From update of attachment 174638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174638&action=review
> 
> The changes look good to me. Actually the only comment I can make about them is about fixing an issue with naming I'm guilty of :/
> 
> Additionally, besides the code in this patch (which is about the AtkPlug part in the Web process only), I wonder if it could be possible to share the bits in WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.[cpp|h] as well, as they implement the other side of this thing (the AtkSocket part in the UI process).
> 
> Or maybe you were already thinking of doing that in a separate bug?
> 
> > Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:60
> > +    Page* page = accessible->m_page->corePage();
> > +    if (!page)
> > +        return 0;
> > +
> > +    Frame* core = page->mainFrame();
> > +    if (!core || !core->document())
> > +        return 0;
> > +
> > +    AccessibilityObject* root = core->document()->axObjectCache()->rootObject();
> > +    if (!root)
> > +        return 0;
> > +
> > +    AtkObject* axRoot = root->wrapper();
> 
> I know I'm the one to blame for this in the first place, but perhaps this patch is a good moment to use more meaningful names here :)
> 
>   page -> corePage
>   core -> coreFrame
>   root -> coreRootObject
>   axRoot -> rootObject (to be consistent with the rest of this file)
> 
> ... or something like that.
Sounds good to me. I will change those names.

Thank you for review.
I was also thinking about sharing WebKitWebViewBaseAccessible.[cpp|h] from UIProcess part. I think it's feasible. Let's have a separate bug on this.

[Krzysztof Czech]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 174638 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=174638&action=review
> > 
> > The changes look good to me. Actually the only comment I can make about them is about fixing an issue with naming I'm guilty of :/
> > 
> > Additionally, besides the code in this patch (which is about the AtkPlug part in the Web process only), I wonder if it could be possible to share the bits in WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.[cpp|h] as well, as they implement the other side of this thing (the AtkSocket part in the UI process).
> > 
> > Or maybe you were already thinking of doing that in a separate bug?
> > 
> > > Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:60
> > > +    Page* page = accessible->m_page->corePage();
> > > +    if (!page)
> > > +        return 0;
> > > +
> > > +    Frame* core = page->mainFrame();
> > > +    if (!core || !core->document())
> > > +        return 0;
> > > +
> > > +    AccessibilityObject* root = core->document()->axObjectCache()->rootObject();
> > > +    if (!root)
> > > +        return 0;
> > > +
> > > +    AtkObject* axRoot = root->wrapper();
> > 
> > I know I'm the one to blame for this in the first place, but perhaps this patch is a good moment to use more meaningful names here :)
> > 
> >   page -> corePage
> >   core -> coreFrame
> >   root -> coreRootObject
> >   axRoot -> rootObject (to be consistent with the rest of this file)
> > 
> > ... or something like that.
> Sounds good to me. I will change those names.
> 
> Thank you for review.
> I was also thinking about sharing WebKitWebViewBaseAccessible.[cpp|h] from UIProcess part. I think it's feasible. Let's have a separate bug on this.

I have opened ticket for this, https://bugs.webkit.org/show_bug.cgi?id=102502

[Krzysztof Czech]

Created attachment 174677 [details]
[WK2][GTK][EFL] Share WebKit2-GTK's WebProcess Accessibility implementation with other WebKit ports.

[WebKit Review Bot]

Attachment 174677 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:107:  web_page_accessibility_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:111:  web_page_accessibility_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObject.h:56:  web_page_accessibility_object_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 174677 [details]
[WK2][GTK][EFL] Share WebKit2-GTK's WebProcess Accessibility implementation with other WebKit ports.

Clearing flags on attachment: 174677

Committed r134951: <http://trac.webkit.org/changeset/134951>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.