Adds a BrowserAccessibilityManager accessor to RenderWidgetHostView.

Adds a BrowserAccessibilityManager accessor to RenderWidgetHostView.

Upcoming test infrastructure requires access to this meember.

TEST=try bots.
BUG=none.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114537

[dmazzoni, 2011-12-12 23:46:55]

As discussed, let's move ownership of the BrowserAccessibilityManager* to the
base class, since most of the subclasses need it anyway - then you just need a
simple accessor rather than a virtual method.

[David Tseng, 2011-12-13 01:26:38]

PTAL.

scoped_ptr in lines a destructor which calls delete on the ptr. The
new patch has a #include to our browser_accessibility_manager.h header
as a consequence. Not sure if that's ok.



On 12/12/11, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> As discussed, let's move ownership of the BrowserAccessibilityManager* to
> the
> base class, since most of the subclasses need it anyway - then you just
> need a
> simple accessor rather than a virtual method.
>
>
> http://codereview.chromium.org/8917020/
>

[dmazzoni, 2011-12-13 05:57:24]

The coding style guide specifically says you should be able to forward-declare
for scoped_ptr - maybe we just need to take inline methods out of
browser_accessibility_manager.h?

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Forward...

http://codereview.chromium.org/8917020/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8917020/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:318: void
browser_accessibility_manager(BrowserAccessibilityManager* manager) {
This should be called set_browser_accessibility_manager - for some examples of
this pattern, see e.g. chrome/browser/ui/browser.h

[David Tseng, 2011-12-13 19:27:04]

Figured it out. I can't in-line any calls to scoped_ptr methods as they require
a complete type.

http://codereview.chromium.org/8917020/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8917020/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:318: void
browser_accessibility_manager(BrowserAccessibilityManager* manager) {
On 2011/12/13 05:57:24, Dominic Mazzoni wrote:
> This should be called set_browser_accessibility_manager - for some examples of
> this pattern, see e.g. chrome/browser/ui/browser.h

Realized this after uploading :). Done and for the other instances of this.

[David Tseng, 2011-12-13 20:43:39]

+ sky for OWNERS approval.

[dmazzoni, 2011-12-13 20:52:36]

lgtm

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view.cc (right):

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.cc:8: #include
"base/memory/scoped_ptr.h"
Not needed, I think, if included by the .h file?

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.cc:46: return
browser_accessibility_manager_.get();
Check indentation

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.cc:51:
browser_accessibility_manager_.reset(manager);
Check indentation

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:316:
BrowserAccessibilityManager* browser_accessibility_manager();
This can be a const method.

[sky, 2011-12-13 21:37:12]

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:316:
BrowserAccessibilityManager* browser_accessibility_manager();
If you use unix_hacker_style, inline int.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:317: void
set_browser_accessibility_manager(BrowserAccessibilityManager* manager);
Since this runs a destructor, don't use unix_hacker_style.

ALso, these methods don't relate to OnAccessibility and reserved_contents, so
add newlines above and below your new methods.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:361:
scoped_ptr<BrowserAccessibilityManager> browser_accessibility_manager_;
Make this private.

[David Tseng, 2011-12-13 23:59:21]

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view.cc (right):

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.cc:8: #include
"base/memory/scoped_ptr.h"
On 2011/12/13 20:52:36, Dominic Mazzoni wrote:
> Not needed, I think, if included by the .h file?

Done.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.cc:46: return
browser_accessibility_manager_.get();
On 2011/12/13 20:52:36, Dominic Mazzoni wrote:
> Check indentation

Done.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.cc:51:
browser_accessibility_manager_.reset(manager);
On 2011/12/13 20:52:36, Dominic Mazzoni wrote:
> Check indentation

Done.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:316:
BrowserAccessibilityManager* browser_accessibility_manager();
On 2011/12/13 20:52:36, Dominic Mazzoni wrote:
> This can be a const method.

Done.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:316:
BrowserAccessibilityManager* browser_accessibility_manager();
On 2011/12/13 21:37:12, sky wrote:
> If you use unix_hacker_style, inline int.

Reverted to Get*/Set* style.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:317: void
set_browser_accessibility_manager(BrowserAccessibilityManager* manager);
On 2011/12/13 21:37:12, sky wrote:
> Since this runs a destructor, don't use unix_hacker_style.
> 
> ALso, these methods don't relate to OnAccessibility and reserved_contents, so
> add newlines above and below your new methods.

Done.

http://codereview.chromium.org/8917020/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view.h:361:
scoped_ptr<BrowserAccessibilityManager> browser_accessibility_manager_;
On 2011/12/13 21:37:12, sky wrote:
> Make this private.

Done.

[sky, 2011-12-14 15:29:03]

LGTM

[commit-bot: I haz the power, 2011-12-14 18:06:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/8917020/8010

[commit-bot: I haz the power, 2011-12-14 18:22:06]

Try job failure for 8917020-8010 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-14 21:01:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/8917020/8010

[commit-bot: I haz the power, 2011-12-14 21:31:31]

Try job failure for 8917020-8010 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-14 22:47:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/8917020/13004

[commit-bot: I haz the power, 2011-12-15 00:06:52]

Change committed as 114537