Add initial GTK web accessibility framework.

Add initial GTK web accessibility framework.

This enables Linux desktop assistive technology such as the Orca screen
reader to access the web contents. Builds on the same accessibility stack
used by Mac & Windows already.

This change works with Orca now, but it's minimal: all that works is getting
the correct feedback when you tab through focusable links and controls in
a webpage. Future changes will add the rest of the support.

BUG=24585
TEST=Run Chrome on desktop Linux with Orca turned on, tab through links.
TBR=darin
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135160

[dmazzoni, 2012-05-01 22:58:01]

Evan, when you have time could you review the GTK code here for proper style,
readability, and proper use of GTK memory management / conventions?

David is the primary reviewer for the accessibility logic and integration with
existing accessibility code. We've already gone over this privately, so first
we'd like a review of all of the GTK parts from you if you're up for it.

Thanks!

[Evan Stade, 2012-05-02 00:03:29]

much of this is Greek to me

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.h (right):

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.h:197:
BrowserAccessibilityGtk* toBrowserAccessibilityGtk();
ToBrowserAccessibilityGtk

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_gtk.cc (right):

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_gtk.cc:86:
->ref_state_set(atk_object);
-> at end of previous line

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_gtk.cc:168: #define
WAI_TYPE_NAME_LEN (30) /* Enough for prefix + 5 hex chars (max) */
you can ask someone else if you like, but I don't think we ever inline #defines
like this. It looks local but really it's not.

I would just make it a static const. (array initialization still works, compiler
might even be smart enough to still use the stack)

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_gtk.cc:116: return
((RenderWidgetHostViewGtk*)userdata)->GetAccessible();
C++ casts (static_cast)

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_gtk.cc:179:
GTK_PRESERVE_WINDOW(widget), GetAccessible, (void *)host_view);
C++ casts or GTK casts. I'm surprised that this cast is even necessary though.

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1468:
->SetAccessibilityMode(AccessibilityModeComplete);
-> on previous line

http://codereview.chromium.org/9839069/diff/14006/ui/gfx/gtk_preserve_window.cc
File ui/gfx/gtk_preserve_window.cc (right):

http://codereview.chromium.org/9839069/diff/14006/ui/gfx/gtk_preserve_window....
ui/gfx/gtk_preserve_window.cc:260: ->get_accessible(widget);
-> previous line

http://codereview.chromium.org/9839069/diff/14006/ui/gfx/gtk_preserve_window.h
File ui/gfx/gtk_preserve_window.h (right):

http://codereview.chromium.org/9839069/diff/14006/ui/gfx/gtk_preserve_window....
ui/gfx/gtk_preserve_window.h:71: void* userdata);
gpointer

[dmazzoni, 2012-05-02 06:27:32]

Thanks.

> much of this is Greek to me

David and I are learning the ATK stuff as we go here. Is there anyone else you'd
recommend who knows GTK guts well enough they may have some insight?

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.h (right):

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.h:197:
BrowserAccessibilityGtk* toBrowserAccessibilityGtk();
On 2012/05/02 00:03:30, Evan Stade wrote:
> ToBrowserAccessibilityGtk

Fixed throughout.

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_gtk.cc (right):

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_gtk.cc:86:
->ref_state_set(atk_object);
On 2012/05/02 00:03:30, Evan Stade wrote:
> -> at end of previous line

Done.

http://codereview.chromium.org/9839069/diff/14006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_gtk.cc:168: #define
WAI_TYPE_NAME_LEN (30) /* Enough for prefix + 5 hex chars (max) */
On 2012/05/02 00:03:30, Evan Stade wrote:
> you can ask someone else if you like, but I don't think we ever inline
#defines
> like this. It looks local but really it's not.
> 
> I would just make it a static const. (array initialization still works,
compiler
> might even be smart enough to still use the stack)

Done. Moved to top and made a static const int.

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_gtk.cc:116: return
((RenderWidgetHostViewGtk*)userdata)->GetAccessible();
On 2012/05/02 00:03:30, Evan Stade wrote:
> C++ casts (static_cast)

Done.

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_gtk.cc:179:
GTK_PRESERVE_WINDOW(widget), GetAccessible, (void *)host_view);
On 2012/05/02 00:03:30, Evan Stade wrote:
> C++ casts or GTK casts. I'm surprised that this cast is even necessary though.

You're right, it wasn't necessary.

http://codereview.chromium.org/9839069/diff/14006/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1468:
->SetAccessibilityMode(AccessibilityModeComplete);
On 2012/05/02 00:03:30, Evan Stade wrote:
> -> on previous line

Done.

[David Tseng, 2012-05-02 18:02:58]

http://codereview.chromium.org/9839069/diff/20011/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_gtk.cc (right):

http://codereview.chromium.org/9839069/diff/20011/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_gtk.cc:41: switch
(type) {
Might have to switch on all enum values (somewhat remember it being a clang
warning -> error on one of the bots). Run it through the try bots to see.

http://codereview.chromium.org/9839069/diff/20011/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_gtk.cc:46:
g_signal_emit_by_name(atk_object, "focus-event", true);
Add a note here to pick the authoritative way of sending focus events (state
changed, focus event, or focus tracker notify) once the alternatives actually
work (reference 
https://bugzilla.gnome.org/show_bug.cgi?id=649575#c4
)

[dmazzoni, 2012-05-02 19:33:05]

http://codereview.chromium.org/9839069/diff/20011/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_gtk.cc (right):

http://codereview.chromium.org/9839069/diff/20011/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_gtk.cc:41: switch
(type) {
On 2012/05/02 18:02:58, David Tseng wrote:
> Might have to switch on all enum values (somewhat remember it being a clang
> warning -> error on one of the bots). Run it through the try bots to see.

It's okay as long as there's a default in the switch. Often it's better to not
have a default so that you get a compiler error if you miss an enum, but for now
this is probably better since we've hardly implemented any of them.

http://codereview.chromium.org/9839069/diff/20011/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_gtk.cc:46:
g_signal_emit_by_name(atk_object, "focus-event", true);
On 2012/05/02 18:02:58, David Tseng wrote:
> Add a note here to pick the authoritative way of sending focus events (state
> changed, focus event, or focus tracker notify) once the alternatives actually
> work (reference 
> https://bugzilla.gnome.org/show_bug.cgi?id=649575#c4
> )

Done.

[dmazzoni, 2012-05-02 20:24:04]

+sky for OWNERS approval

[Evan Stade, 2012-05-02 20:30:18]

>  Is there anyone else you'd
> recommend who knows GTK guts well enough they may have some insight?


Elliot knows it better than me

[Elliot Glaysher, 2012-05-02 21:15:21]

I don't know any of this. You are probably the most qualified person about ATK,
Dominic. I've done a quick sanity check on your gobject code; lgtm.

Good luck.

[David Tseng, 2012-05-02 21:18:19]

lgtm

On 5/2/12, erg@chromium.org <erg@chromium.org> wrote:
> I don't know any of this. You are probably the most qualified person about
>
> ATK,
> Dominic. I've done a quick sanity check on your gobject code; lgtm.
>
> Good luck.
>
> http://codereview.chromium.org/9839069/
>

[sky, 2012-05-02 21:22:13]

Rubber stamp LGTM

[commit-bot: I haz the power, 2012-05-02 21:23:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/9839069/35007

[commit-bot: I haz the power, 2012-05-02 21:23:46]

Presubmit check for 9839069-35007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 3.1s to calculate.

[dmazzoni, 2012-05-02 21:26:28]

Adding TBR=darin for content/OWNERS

[commit-bot: I haz the power, 2012-05-02 21:26:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/9839069/35007

[commit-bot: I haz the power, 2012-05-02 23:01:05]

Change committed as 135020