Create content API for BrowserAccessibilityState

Create content API for BrowserAccessibilityState

BUG=98716
TEST=none
TBR=sky


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

[dmazzoni, 2012-02-20 19:30:04]

It seemed silly to create content/public/browser/accessibility - is it okay that
the directories don't match?

[jam, 2012-02-20 21:13:01]

thanks for working on this!

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_state_impl.h (right):

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_state_impl.h:12: #include
"content/common/content_export.h"
nit: not needed now

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_state_impl.h:38: static
BrowserAccessibilityStateImpl* GetInstance();
nit: we only add static getters for the Impl if it's used by content. it doesn't
look like this is needed since there are no public methods that aren't on the
implementation other than the interface

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
File content/public/browser/browser_accessibility_state.h (right):

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:11: // The
BrowserAccessibilityState class is used to determine if Chrome should be
nit: we don't mention chrome in content. so can just say "if the browser"

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:17:
BrowserAccessibilityState() { }
nit: not needed

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:33: friend class
BrowserAccessibilityStateImpl;
why is this needed?

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:35:
DISALLOW_COPY_AND_ASSIGN(BrowserAccessibilityState);
nit: not needed on interfaces since they're not copyable

[dmazzoni, 2012-02-21 05:32:17]

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_state_impl.h (right):

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_state_impl.h:12: #include
"content/common/content_export.h"
On 2012/02/20 21:13:01, John Abd-El-Malek wrote:
> nit: not needed now

Done.

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_state_impl.h:38: static
BrowserAccessibilityStateImpl* GetInstance();
On 2012/02/20 21:13:01, John Abd-El-Malek wrote:
> nit: we only add static getters for the Impl if it's used by content. it
doesn't
> look like this is needed since there are no public methods that aren't on the
> implementation other than the interface

It's actually needed because Singleton<> only allows a method called GetInstance
on the class that owns the Singleton to access it:

class Singleton {
 private:
  // Classes using the Singleton<T> pattern should declare a GetInstance()
  // method and call Singleton::get() from within that.
  friend Type* Type::GetInstance();

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
File content/public/browser/browser_accessibility_state.h (right):

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:11: // The
BrowserAccessibilityState class is used to determine if Chrome should be
On 2012/02/20 21:13:01, John Abd-El-Malek wrote:
> nit: we don't mention chrome in content. so can just say "if the browser"

Done.

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:17:
BrowserAccessibilityState() { }
On 2012/02/20 21:13:01, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:33: friend class
BrowserAccessibilityStateImpl;
On 2012/02/20 21:13:01, John Abd-El-Malek wrote:
> why is this needed?

Forgot to delete this - I was trying to figure out a way around the GetInstance
issue...

http://codereview.chromium.org/9421030/diff/1/content/public/browser/browser_...
content/public/browser/browser_accessibility_state.h:35:
DISALLOW_COPY_AND_ASSIGN(BrowserAccessibilityState);
On 2012/02/20 21:13:01, John Abd-El-Malek wrote:
> nit: not needed on interfaces since they're not copyable

Done.

[jam, 2012-02-21 07:22:17]

lgtm

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_state_impl.h (right):

http://codereview.chromium.org/9421030/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_state_impl.h:38: static
BrowserAccessibilityStateImpl* GetInstance();
On 2012/02/21 05:32:17, Dominic Mazzoni wrote:
> On 2012/02/20 21:13:01, John Abd-El-Malek wrote:
> > nit: we only add static getters for the Impl if it's used by content. it
> doesn't
> > look like this is needed since there are no public methods that aren't on
the
> > implementation other than the interface
> 
> It's actually needed because Singleton<> only allows a method called
GetInstance
> on the class that owns the Singleton to access it:
> 
> class Singleton {
>  private:
>   // Classes using the Singleton<T> pattern should declare a GetInstance()
>   // method and call Singleton::get() from within that.
>   friend Type* Type::GetInstance();

ah, got it, thanks. i must have been thinking of static getters without the
singleton

[commit-bot: I haz the power, 2012-02-21 15:56:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/9421030/31

[commit-bot: I haz the power, 2012-02-21 15:57:03]

Presubmit check for 9421030-31 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/views/toolbar_view.cc,chrome/browser/ui/views/frame/browser_frame_win.cc

Presubmit checks took 1.2s to calculate.

[dmazzoni, 2012-02-21 16:00:46]

+sky for chrome/browser/ui

[jam, 2012-02-21 18:18:28]

On 2012/02/21 16:00:46, Dominic Mazzoni wrote:
> +sky for chrome/browser/ui

btw for the content refactoring when it's simple file path/function
renaming/moving to namespace etc, we TBR it to owners or ignore owners check.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/9421030/31

[commit-bot: I haz the power, 2012-02-21 19:53:29]

Change committed as 122856