Allow Extensions to have MSAA information

Allow Extensions to have MSAA information.

The whole extension shelf and its items were missing MSAA information. Some of them were uninitialized due to the custom components not having any AccessibleRole. 

Some extensions have no name, this could happen where the user didn't put a name for their browser action in the manifest. If such case happens, we will use the extension name itself.

BUG=36289
TEST=Extension shelf now has MSAA information according to Inspect32. Screenshot available on the issue tracker.

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

[Mohamed Mansour, 2010-03-20 15:46:04]

Hi Chris, would you mind reviewing this for me? Small change that exposes MSAA
information for ResizeGripper, and the Container.

Finnur, would you mind checking line 167 in browser_actions_container.cc,
sometimes the tooltip becomes empty. This could happen if the browser action has
no name within the manifest. If such case happens, I use the extension name
instead. I was lucky to spot that because one of my extensions had no name for
browser action and it was empty.

Thanks in advance!

[Finnur, 2010-03-20 17:05:17]

LGTM

On 2010/03/20 15:46:04, Mohamed Mansour wrote:
> Hi Chris, would you mind reviewing this for me? Small change that exposes MSAA
> information for ResizeGripper, and the Container.
> 
> Finnur, would you mind checking line 167 in browser_actions_container.cc,
> sometimes the tooltip becomes empty. This could happen if the browser action
has
> no name within the manifest. If such case happens, I use the extension name
> instead. I was lucky to spot that because one of my extensions had no name for
> browser action and it was empty.
> 
> Thanks in advance!

[tfarina (gmail-do not use), 2010-03-20 19:47:20]

http://codereview.chromium.org/1105008/diff/1/3
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1105008/diff/1/3#newcode314
chrome/browser/views/browser_actions_container.cc:314: *role =
AccessibilityTypes::ROLE_GROUPING;
nit: could you add the following lines?
if (!role)
  return false;

http://codereview.chromium.org/1105008/diff/1/3#newcode320
chrome/browser/views/browser_actions_container.cc:320: *name =
l10n_util::GetString(IDS_ACCNAME_EXTENSIONS_BROWSER_ACTION);
nit: could you add the following lines?

if (!name)
  return false;

http://codereview.chromium.org/1105008/diff/1/3#newcode810
chrome/browser/views/browser_actions_container.cc:810: *role =
AccessibilityTypes::ROLE_GROUPING;
nit: could you add the following lines?
if (!role)
  return false;

http://codereview.chromium.org/1105008/diff/1/3#newcode816
chrome/browser/views/browser_actions_container.cc:816: *name =
l10n_util::GetString(IDS_ACCNAME_EXTENSIONS);
nit: could you add the following lines?
if (!name)
  return false;

http://codereview.chromium.org/1105008/diff/1/5
File views/controls/resize_gripper.cc (right):

http://codereview.chromium.org/1105008/diff/1/5#newcode81
views/controls/resize_gripper.cc:81: *role = AccessibilityTypes::ROLE_SEPARATOR;
nit: could you add the following lines?
if (!role)
  return false;

http://codereview.chromium.org/1105008/diff/1/5#newcode87
views/controls/resize_gripper.cc:87: if (!accessible_name_.empty()) {
nit: could you add the following lines?
if (!name)
  return false;

[Finnur, 2010-03-20 20:36:18]

No, Thiago. It may seem counter-intuitive, but these types of checks are frowned
upon in the Chromium codebase.

Please read the bottom section of this page:

http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

I am actually also against writing:

DCHECK(pointer)
pointer->foo()

... because it serves no purpose. It will be just as obvious to the developer
that the pointer is null if it crashes in Debug build as if it DCHECKs. End
result: It just adds bloat to the debug build.


On 2010/03/20 19:47:20, tfarina wrote:
> http://codereview.chromium.org/1105008/diff/1/3
> File chrome/browser/views/browser_actions_container.cc (right):
> 
> http://codereview.chromium.org/1105008/diff/1/3#newcode314
> chrome/browser/views/browser_actions_container.cc:314: *role =
> AccessibilityTypes::ROLE_GROUPING;
> nit: could you add the following lines?
> if (!role)
>   return false;
> 
> http://codereview.chromium.org/1105008/diff/1/3#newcode320
> chrome/browser/views/browser_actions_container.cc:320: *name =
> l10n_util::GetString(IDS_ACCNAME_EXTENSIONS_BROWSER_ACTION);
> nit: could you add the following lines?
> 
> if (!name)
>   return false;
> 
> http://codereview.chromium.org/1105008/diff/1/3#newcode810
> chrome/browser/views/browser_actions_container.cc:810: *role =
> AccessibilityTypes::ROLE_GROUPING;
> nit: could you add the following lines?
> if (!role)
>   return false;
> 
> http://codereview.chromium.org/1105008/diff/1/3#newcode816
> chrome/browser/views/browser_actions_container.cc:816: *name =
> l10n_util::GetString(IDS_ACCNAME_EXTENSIONS);
> nit: could you add the following lines?
> if (!name)
>   return false;
> 
> http://codereview.chromium.org/1105008/diff/1/5
> File views/controls/resize_gripper.cc (right):
> 
> http://codereview.chromium.org/1105008/diff/1/5#newcode81
> views/controls/resize_gripper.cc:81: *role =
AccessibilityTypes::ROLE_SEPARATOR;
> nit: could you add the following lines?
> if (!role)
>   return false;
> 
> http://codereview.chromium.org/1105008/diff/1/5#newcode87
> views/controls/resize_gripper.cc:87: if (!accessible_name_.empty()) {
> nit: could you add the following lines?
> if (!name)
>   return false;

[tfarina (gmail-do not use), 2010-03-20 20:39:49]

On 2010/03/20 20:36:18, Finnur wrote:
> No, Thiago. It may seem counter-intuitive, but these types of checks are
frowned
> upon in the Chromium codebase.
> 
> Please read the bottom section of this page:
> 
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
> 
> I am actually also against writing:
> 
> DCHECK(pointer)
> pointer->foo()
> 
> ... because it serves no purpose. It will be just as obvious to the developer
> that the pointer is null if it crashes in Debug build as if it DCHECKs. End
> result: It just adds bloat to the debug build.
> 

I don't agree, why we would allow dereferencing the pointer without checking it
first?

[tfarina (gmail-do not use), 2010-03-20 20:41:47]

On 2010/03/20 20:39:49, tfarina wrote:
> On 2010/03/20 20:36:18, Finnur wrote:
> > No, Thiago. It may seem counter-intuitive, but these types of checks are
> frowned
> > upon in the Chromium codebase.
> > 
> > Please read the bottom section of this page:
> > 
> >
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
> > 
> > I am actually also against writing:
> > 
> > DCHECK(pointer)
> > pointer->foo()
> > 
> > ... because it serves no purpose. It will be just as obvious to the
developer
> > that the pointer is null if it crashes in Debug build as if it DCHECKs. End
> > result: It just adds bloat to the debug build.
> > 
> 
> I don't agree, why we would allow dereferencing the pointer without checking
it
> first?

Also, the check is not for the DEBUG mode, but for the release mode, since
DCHECK is disabled in release builds.

[Finnur, 2010-03-20 20:42:56]

You may disagree with it, but this is a conscious decision we made early on for
the Chromium project, is not up for debate and is explained in the document I
linked.

 On 2010/03/20 20:39:49, tfarina wrote:
> On 2010/03/20 20:36:18, Finnur wrote:
> > No, Thiago. It may seem counter-intuitive, but these types of checks are
> frowned
> > upon in the Chromium codebase.
> > 
> > Please read the bottom section of this page:
> > 
> >
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
> > 
> > I am actually also against writing:
> > 
> > DCHECK(pointer)
> > pointer->foo()
> > 
> > ... because it serves no purpose. It will be just as obvious to the
developer
> > that the pointer is null if it crashes in Debug build as if it DCHECKs. End
> > result: It just adds bloat to the debug build.
> > 
> 
> I don't agree, why we would allow dereferencing the pointer without checking
it
> first?

[Chris Guillory, 2010-03-22 20:32:06]

LGTM

[M-A Ruel, 2010-03-23 00:35:49]

http://codereview.chromium.org/1105008/diff/1/3
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1105008/diff/1/3#newcode313
chrome/browser/views/browser_actions_container.cc:313: DCHECK(role);
drive-by:
you should have removed the DCHECK and set the return value to void. There's
nothing the caller can do on the return value and the DCHECK doesn't add
anything.

http://codereview.chromium.org/1105008/diff/1/3#newcode318
chrome/browser/views/browser_actions_container.cc:318: bool
BrowserActionView::GetAccessibleName(std::wstring* name) {
Same.

[Finnur, 2010-03-23 00:44:03]

I agree with you on the DCHECK, but this function is an override from View, so
changing the return value is not an option unless we change it everywhere. 

See:
http://src.chromium.org/viewvc/chrome/trunk/src/views/view.h?revision=42239&v...


On 2010/03/23 00:35:49, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/1105008/diff/1/3
> File chrome/browser/views/browser_actions_container.cc (right):
> 
> http://codereview.chromium.org/1105008/diff/1/3#newcode313
> chrome/browser/views/browser_actions_container.cc:313: DCHECK(role);
> drive-by:
> you should have removed the DCHECK and set the return value to void. There's
> nothing the caller can do on the return value and the DCHECK doesn't add
> anything.
> 
> http://codereview.chromium.org/1105008/diff/1/3#newcode318
> chrome/browser/views/browser_actions_container.cc:318: bool
> BrowserActionView::GetAccessibleName(std::wstring* name) {
> Same.