Move some accessibility methods, enums and interfaces into the content/public API.

Move some accessibility interfaces into the content/public API.

To facilitate creating the necessary APIs for a chrome://accessibility page, move some accessibility-related methods into the content/public API:
- Promote Get/SetAccessibilityMode() from RenderWidgetHostImpl into RenderWidgetHost, and move the AccessibilityMode enum from view_message_enums.h into its own file in content/public/common
- Promote Get/SetAccessibilityMode() from BrowserAccessibilityStateImpl into BrowserAccessibilityState (this is the global accessibility state, as opposed to the per-renderer state in RenderWidgetHost)
- Create a public interface for what was previous called DumpAccessibilityTreeHelper, now called AccessibilityTreeFormatter

BUG=178756

[dmazzoni, 2013-02-26 22:37:36]

Hmmm, I'm starting to think it'd be cleaner to define AccessibilityMode in a new
file content/public/common/accessibility_mode.h.

There are other similarly short files there, like:

content/public/common/context_menu_source_type.h

https://codereview.chromium.org/12335101/diff/4001/content/browser/accessibil...
File content/browser/accessibility/dump_accessibility_tree_helper.h (right):

https://codereview.chromium.org/12335101/diff/4001/content/browser/accessibil...
content/browser/accessibility/dump_accessibility_tree_helper.h:21: class
DumpAccessibilityTreeHelper : public AccessibilityTreeFormatter {
DumpAccessibilityTreeHelper should be renamed AccessibilityTreeFormatterImpl

https://codereview.chromium.org/12335101/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/12335101/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:123: virtual void
SetAccessibilityEnabled(bool enabled) OVERRIDE;
This is now redundant with EnableFullAccessibilityMode, please merge them.

https://codereview.chromium.org/12335101/diff/4001/content/public/browser/acc...
File content/public/browser/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/12335101/diff/4001/content/public/browser/acc...
content/public/browser/accessibility_tree_formatter.h:18: static
AccessibilityTreeFormatter* From(RenderViewHost* rvh);
I think this should be called Create. "From" is normally used as a way to
typecast something or access something, not a way to create something.

[dmazzoni, 2013-02-26 22:38:27]

Also, please create a bug to capture the chrome://accessibility/ work and add
more detail to your change description.

[aboxhall, 2013-02-27 01:44:04]

On 2013/02/26 22:38:27, Dominic Mazzoni wrote:
> Also, please create a bug to capture the chrome://accessibility/ work and add
> more detail to your change description.

Will do; I'm kind of holding off until I hear back from dbeam/arv though. Maybe
I should just go ahead and create it anyway.

[aboxhall, 2013-02-27 01:44:15]

https://codereview.chromium.org/12335101/diff/4001/content/browser/accessibil...
File content/browser/accessibility/dump_accessibility_tree_helper.h (right):

https://codereview.chromium.org/12335101/diff/4001/content/browser/accessibil...
content/browser/accessibility/dump_accessibility_tree_helper.h:21: class
DumpAccessibilityTreeHelper : public AccessibilityTreeFormatter {
On 2013/02/26 22:37:36, Dominic Mazzoni wrote:
> DumpAccessibilityTreeHelper should be renamed AccessibilityTreeFormatterImpl

Done.

https://codereview.chromium.org/12335101/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/12335101/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:123: virtual void
SetAccessibilityEnabled(bool enabled) OVERRIDE;
On 2013/02/26 22:37:36, Dominic Mazzoni wrote:
> This is now redundant with EnableFullAccessibilityMode, please merge them.

Since AccessibilityMode is now in content/public, I pulled
Get/SetAccessibilityMode up into this class, removed these bool methods, and
left EnableFullAccessibilityMode as is. What do you think?

https://codereview.chromium.org/12335101/diff/4001/content/public/browser/acc...
File content/public/browser/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/12335101/diff/4001/content/public/browser/acc...
content/public/browser/accessibility_tree_formatter.h:18: static
AccessibilityTreeFormatter* From(RenderViewHost* rvh);
On 2013/02/26 22:37:36, Dominic Mazzoni wrote:
> I think this should be called Create. "From" is normally used as a way to
> typecast something or access something, not a way to create something.

Done.

[dmazzoni, 2013-02-27 16:41:01]

Add a bug number and another sentence about what this
is for to the description, otherwise this is looking good.

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
File content/browser/accessibility/accessibility_tree_formatter_impl.h (right):

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
content/browser/accessibility/accessibility_tree_formatter_impl.h:18: // A
utility class for retrieving platform specific accessibility information.
Maybe update this comment to say "formatting" platform-specific accessibility
information, for debugging and developer tools.

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_state_impl.h (right):

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_state_impl.h:53: virtual
bool GetAccessibilityEnabled() OVERRIDE;
These shouldn't be needed any more, right? Let's prefer
GetAccessibilityMode/SetAccessibilityMode everywhere.

https://codereview.chromium.org/12335101/diff/15/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/12335101/diff/15/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:122: virtual void
EnableFullAccessibilityMode() OVERRIDE;
Get rid of this variant now that we have SetAccessibilityMode.

https://codereview.chromium.org/12335101/diff/15/content/public/browser/acces...
File content/public/browser/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/12335101/diff/15/content/public/browser/acces...
content/public/browser/accessibility_tree_formatter.h:20: // Formats the
BrowserAccessibility tree for this render view host into
BrowserAccessibility -> browser accessibility (since BrowserAccessibility is
part of the hidden implementation of content/)

https://codereview.chromium.org/12335101/diff/15/content/public/browser/brows...
File content/public/browser/browser_accessibility_state.h (right):

https://codereview.chromium.org/12335101/diff/15/content/public/browser/brows...
content/public/browser/browser_accessibility_state.h:32: virtual bool
GetAccessibilityEnabled() = 0;
Make GetAccessibilityMode virtual instead.

[aboxhall, 2013-02-27 18:58:50]

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
File content/browser/accessibility/accessibility_tree_formatter_impl.h (right):

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
content/browser/accessibility/accessibility_tree_formatter_impl.h:18: // A
utility class for retrieving platform specific accessibility information.
On 2013/02/27 16:41:01, Dominic Mazzoni wrote:
> Maybe update this comment to say "formatting" platform-specific accessibility
> information, for debugging and developer tools.

Done.

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_state_impl.h (right):

https://codereview.chromium.org/12335101/diff/15/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_state_impl.h:53: virtual
bool GetAccessibilityEnabled() OVERRIDE;
On 2013/02/27 16:41:01, Dominic Mazzoni wrote:
> These shouldn't be needed any more, right? Let's prefer
> GetAccessibilityMode/SetAccessibilityMode everywhere.

Oops, I thought I got rid of these. Done.

https://codereview.chromium.org/12335101/diff/15/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/12335101/diff/15/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:122: virtual void
EnableFullAccessibilityMode() OVERRIDE;
On 2013/02/27 16:41:01, Dominic Mazzoni wrote:
> Get rid of this variant now that we have SetAccessibilityMode.

Done.

https://codereview.chromium.org/12335101/diff/15/content/public/browser/acces...
File content/public/browser/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/12335101/diff/15/content/public/browser/acces...
content/public/browser/accessibility_tree_formatter.h:20: // Formats the
BrowserAccessibility tree for this render view host into
On 2013/02/27 16:41:01, Dominic Mazzoni wrote:
> BrowserAccessibility -> browser accessibility (since BrowserAccessibility is
> part of the hidden implementation of content/)

Done.

https://codereview.chromium.org/12335101/diff/15/content/public/browser/brows...
File content/public/browser/browser_accessibility_state.h (right):

https://codereview.chromium.org/12335101/diff/15/content/public/browser/brows...
content/public/browser/browser_accessibility_state.h:32: virtual bool
GetAccessibilityEnabled() = 0;
On 2013/02/27 16:41:01, Dominic Mazzoni wrote:
> Make GetAccessibilityMode virtual instead.

Done.

[dmazzoni, 2013-02-27 19:26:43]

+jam

lgtm

[jam, 2013-02-27 19:35:50]

https://codereview.chromium.org/12335101/diff/14016/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/12335101/diff/14016/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_application_mac.mm:23: #include
"content/public/common/accessibility_mode.h"
nit: this seems redundant since by definition if you're calling a method which
takes an enum value, that header must have included the enum

https://codereview.chromium.org/12335101/diff/14016/content/public/browser/ac...
File content/public/browser/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/12335101/diff/14016/content/public/browser/ac...
content/public/browser/accessibility_tree_formatter.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
why is this in the public api? i don't see it called from chrome

https://codereview.chromium.org/12335101/diff/14016/content/public/common/acc...
File content/public/common/accessibility_mode.h (right):

https://codereview.chromium.org/12335101/diff/14016/content/public/common/acc...
content/public/common/accessibility_mode.h:13: AccessibilityModeOff,
nit: per chrome style guide, macros should be all in caps. so
ACCESSIBILITY_MODE_OFF, ACCESSIBILITY_MODE_EDITABLE_TEXT_ONLY,
ACCESSIBILITY_MODE_COMPLETE

[aboxhall, 2013-02-27 21:19:58]

https://codereview.chromium.org/12335101/diff/14016/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/12335101/diff/14016/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_application_mac.mm:23: #include
"content/public/common/accessibility_mode.h"
On 2013/02/27 19:35:50, jam wrote:
> nit: this seems redundant since by definition if you're calling a method which
> takes an enum value, that header must have included the enum

Fair enough. Removed.

https://codereview.chromium.org/12335101/diff/14016/content/public/browser/ac...
File content/public/browser/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/12335101/diff/14016/content/public/browser/ac...
content/public/browser/accessibility_tree_formatter.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/02/27 19:35:50, jam wrote:
> why is this in the public api? i don't see it called from chrome

It isn't yet, but it will be once the chrome://accessibility page is created.

https://codereview.chromium.org/12335101/diff/14016/content/public/common/acc...
File content/public/common/accessibility_mode.h (right):

https://codereview.chromium.org/12335101/diff/14016/content/public/common/acc...
content/public/common/accessibility_mode.h:13: AccessibilityModeOff,
On 2013/02/27 19:35:50, jam wrote:
> nit: per chrome style guide, macros should be all in caps. so
> ACCESSIBILITY_MODE_OFF, ACCESSIBILITY_MODE_EDITABLE_TEXT_ONLY,
> ACCESSIBILITY_MODE_COMPLETE

Done.

[aboxhall, 2013-02-27 22:29:07]

Pulled accessibility_test_utils_win over into content/browser/accessibility to
make it compile on Windows (and renamed it to
accessibility_tree_formatter_utils_win which unfortunately makes it one
character too long to fit in an 80-char line as a #include - any suggestions?)

PTAL

[dmazzoni, 2013-02-27 22:39:49]

On 2013/02/27 22:29:07, aboxhall wrote:
> Pulled accessibility_test_utils_win over into content/browser/accessibility to
> make it compile on Windows (and renamed it to
> accessibility_tree_formatter_utils_win which unfortunately makes it one
> character too long to fit in an 80-char line as a #include - any suggestions?)

I don't think the 80-char rule applies to #includes or include guards.

But, what about accessibility_tree_format_utils_win to save 3 chars? Or,
accessibility_formatter_utils_win (and switch to accessibility_formatter) -
perhaps "tree" is implied.

[jam, 2013-02-27 22:50:23]

On 2013/02/27 22:39:49, Dominic Mazzoni wrote:
> On 2013/02/27 22:29:07, aboxhall wrote:
> > Pulled accessibility_test_utils_win over into content/browser/accessibility
to
> > make it compile on Windows (and renamed it to
> > accessibility_tree_formatter_utils_win which unfortunately makes it one
> > character too long to fit in an 80-char line as a #include - any
suggestions?)
> 
> I don't think the 80-char rule applies to #includes or include guards.

correct

> 
> But, what about accessibility_tree_format_utils_win to save 3 chars? Or,
> accessibility_formatter_utils_win (and switch to accessibility_formatter) -
> perhaps "tree" is implied.

[jam, 2013-02-27 22:51:32]

https://codereview.chromium.org/12335101/diff/14016/content/public/browser/ac...
File content/public/browser/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/12335101/diff/14016/content/public/browser/ac...
content/public/browser/accessibility_tree_formatter.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/02/27 21:19:59, aboxhall wrote:
> On 2013/02/27 19:35:50, jam wrote:
> > why is this in the public api? i don't see it called from chrome
> 
> It isn't yet, but it will be once the chrome://accessibility page is created.

you can now implement webui pages inside content (the motivation was actually
stuff like this, i.e. not to add stuff to content api just because of webui
pages). what will be in the webui page, is it just stuff inside content? if so
it should be implemented inside content. see
content_web_ui_controller_factory.cc

[dmazzoni, 2013-02-27 23:00:13]

On 2013/02/27 22:51:32, jam wrote:
> you can now implement webui pages inside content (the motivation was actually
> stuff like this, i.e. not to add stuff to content api just because of webui
> pages). what will be in the webui page, is it just stuff inside content? if so
> it should be implemented inside content. see
> content_web_ui_controller_factory.cc

Unfortunately content doesn't have extension APIs, right?
New webui pages are supposed to strongly prefer extension APIs
(including private extension APIs as needed) instead of
chrome.send.

Or is there a good example of a content-based webui page that
doesn't use chrome.send?

[jam, 2013-02-27 23:10:23]

On 2013/02/27 23:00:13, Dominic Mazzoni wrote:
> On 2013/02/27 22:51:32, jam wrote:
> > you can now implement webui pages inside content (the motivation was
actually
> > stuff like this, i.e. not to add stuff to content api just because of webui
> > pages). what will be in the webui page, is it just stuff inside content? if
so
> > it should be implemented inside content. see
> > content_web_ui_controller_factory.cc
> 
> Unfortunately content doesn't have extension APIs, right?

extensions is a chrome feature, so content doesn't know anything about it.

> New webui pages are supposed to strongly prefer extension APIs
> (including private extension APIs as needed) instead of
> chrome.send.

oh, this is news to me.

it seems fine to use this in webui pages that are for chrome.

however webui pages for features that are in content should reside in content.
otherwise we expose internal details of content externally just to implement the
webui.

see http://crbug.com/169170 for more info

> 
> Or is there a good example of a content-based webui page that
> doesn't use chrome.send?

[dmazzoni, 2013-02-27 23:28:03]

On 2013/02/27 23:10:23, jam wrote:
> > New webui pages are supposed to strongly prefer extension APIs
> > (including private extension APIs as needed) instead of
> > chrome.send.
> 
> oh, this is news to me.

Apparently multiple security exploits have used chrome.send. :)

> it seems fine to use this in webui pages that are for chrome.
> 
> however webui pages for features that are in content should reside in content.

...then we need a mechanism other than chrome.send to expose features from
content.

The number of APIs we need for this feature is small, and they'll be high-level.
OK to implement it in Chrome now, exposing some minimal APIs from content, and
file a bug to port to Content once a secure solution opens up?

[jam, 2013-02-27 23:32:37]

On 2013/02/27 23:28:03, Dominic Mazzoni wrote:
> On 2013/02/27 23:10:23, jam wrote:
> > > New webui pages are supposed to strongly prefer extension APIs
> > > (including private extension APIs as needed) instead of
> > > chrome.send.
> > 
> > oh, this is news to me.
> 
> Apparently multiple security exploits have used chrome.send. :)

I'm still trying to understand this. will all existing webuis be rewritten? If
not, then I'm not sure why this one new webui should have a higher bar
(especially since doing so would mean that it would act unnecessary APIs to the
content api).

> 
> > it seems fine to use this in webui pages that are for chrome.
> > 
> > however webui pages for features that are in content should reside in
content.
> 
> ...then we need a mechanism other than chrome.send to expose features from
> content.
> 
> The number of APIs we need for this feature is small, and they'll be
high-level.
> OK to implement it in Chrome now, exposing some minimal APIs from content, and
> file a bug to port to Content once a secure solution opens up?

A huge amount of effort has gone to keep content api as minimal as possible.
With increasing use of content by other projects, I think it's even more
critical to keep the API small. So I don't want to add stuff to it if it can be
avoided, which in this case it still seems that it can be.