Bug 101748

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

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

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.

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.

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

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.

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.

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

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

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

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.

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

This patch eliminates some code duplication in terms of Accessibility (see 99578) Ping reviewers ?.

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.

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.

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

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

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.

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.

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

(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

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

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.

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>

All reviewed patches have been landed. Closing bug.