Add tab status to accessibility labels

Add tab status to accessibility labels
We display various visual tab indicators but there is no corresponding indicator for accessible labels. The updated accessible label is in the format of:

Page title - Tab status

For tabs in incognito this is a format:
Page title - Tab status (incognito)

This fixes the crashes by adding a check for the active tab index.

BUG=635610
Review-Url: https://codereview.chromium.org/2581023002
Cr-Commit-Position: refs/heads/master@{#441738}
Committed: https://chromium.googlesource.com/chromium/src/+/bfeeddcdf2925a4fcd7bcf9666c8af703fa35e29

[edwardjung, 2016-12-15 23:55:02]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 23:56:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[edwardjung, 2016-12-15 23:58:19]

edwardjung@chromium.org changed reviewers:
+ dmazzoni@chromium.org, sky@chromium.org

[edwardjung, 2016-12-15 23:58:19]

sky, please take a look. I've added a check in patchset #2 for a positive active
tab index.

Thanks

[edwardjung, 2016-12-15 23:59:54]

Description was changed from

==========
Add tab status to accessibility labels

BUG=635610
==========

to

==========
Add tab status to accessibility labels
We display various visual tab indicators but there is no corresponding indicator
for accessible labels. The updated accessible label is in the format of:

Page title - Tab status

For tabs in incognito this is a format:
Page title - Tab status (incognito)

This fixes the crashes by adding a check for the active tab index.

BUG=635610
==========

[sky, 2016-12-16 00:44:10]

Can you land the fix for the crash separate from this?

[sky, 2016-12-16 00:52:52]

https://codereview.chromium.org/2581023002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2581023002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14222: +      <message
name="IDS_TAB_AX_LABEL_MEDIA_RECORDING_FORMAT" desc="Accessibility label text,
when the tab is recording media. Example: 'Google Hangouts - Camera or
microphone recording'">
Why the _FORMAT? I don't think it adds anything here, I recommend removing it.

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1622:
browser_->GetWindowTitleForCurrentTab(include_app_name);
Shouldn't you only do this for the active tab? Maybe you want
Browser::GetWindowTitleForTab() and you supply the tab (that doesn't exist now,
I'm suggesting you create it).

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1626: tab_label =
l10n_util::GetStringFUTF16(IDS_TAB_AX_LABEL_CRASHED_FORMAT,
As you only set the label and then return at the end of the function early
return from all of these rather than adding an unnecessary local (when you do
that, don't use else ifs, see style guide for details).

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:436: return
BrowserView::GetBrowserViewForBrowser(browser_)
Can you change BrowserTabStripController to take a BrowserView? Less lookup
then.

Do all tabs really share the same accessible name? Shouldn't the name be
specific to the tab?

[commit-bot: I haz the power, 2016-12-16 02:05:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 02:05:44]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2016-12-16 09:15:13]

On 2016/12/16 00:44:10, sky wrote:
> Can you land the fix for the crash separate from this?

The original CL was reverted (2579893002) as being in a different time zone
there would have been a delay getting a fix in place. 

Everything in PS#1 was as is originally submitted in 2497373003 and PS#2 has the
crash fix.

[edwardjung, 2016-12-19 12:53:01]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 12:53:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[edwardjung, 2016-12-19 12:59:26]

Addressed the comments. 

Thanks for catching that I was just getting the active tab title rather than the
specific tab title.

https://codereview.chromium.org/2581023002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2581023002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14222: +      <message
name="IDS_TAB_AX_LABEL_MEDIA_RECORDING_FORMAT" desc="Accessibility label text,
when the tab is recording media. Example: 'Google Hangouts - Camera or
microphone recording'">
On 2016/12/16 00:52:51, sky wrote:
> Why the _FORMAT? I don't think it adds anything here, I recommend removing it.

I was following a lot of the strings that ate postfixed with _format for strings
that have string substitution.

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1622:
browser_->GetWindowTitleForCurrentTab(include_app_name);
On 2016/12/16 00:52:51, sky wrote:
> Shouldn't you only do this for the active tab? Maybe you want
> Browser::GetWindowTitleForTab() and you supply the tab (that doesn't exist
now,
> I'm suggesting you create it).

Good point, changed.

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1626: tab_label =
l10n_util::GetStringFUTF16(IDS_TAB_AX_LABEL_CRASHED_FORMAT,
On 2016/12/16 00:52:51, sky wrote:
> As you only set the label and then return at the end of the function early
> return from all of these rather than adding an unnecessary local (when you do
> that, don't use else ifs, see style guide for details).

Done.

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

https://codereview.chromium.org/2581023002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:436: return
BrowserView::GetBrowserViewForBrowser(browser_)
On 2016/12/16 00:52:51, sky wrote:
> Can you change BrowserTabStripController to take a BrowserView? Less lookup
> then.
> 
> Do all tabs really share the same accessible name? Shouldn't the name be
> specific to the tab?

Good point, fixed.

[commit-bot: I haz the power, 2016-12-19 13:05:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 13:05:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[edwardjung, 2016-12-19 13:30:45]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 13:30:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 14:16:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 14:16:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[edwardjung, 2016-12-20 15:15:54]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 15:16:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 16:05:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 16:05:10]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-03 23:11:42]

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1606: } else {
no else after return (see chromium style guide for details).

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1618: if (index > -1) {
When is this called with -1? I think callers should ensure they don't do that
and this conditional should become a DCHECK.

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1621: TabRendererData data =
tabstrip_->tab_at(index)->data();
const TabRendererData&

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:406: if
(browser_view_ && !immersive_reveal_lock_.get()) {
browser_view_ should never be null, right?

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.h (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:38: BrowserView*
browser_view);
BrowserView has a browser() function, so that now that this depends upon
BrowserView you shouldn't need the Browser* member.

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_strip.cc:1329: return
controller_->GetAccessibleTabName(tab);
You should only call to controller_ if tab is in tabs_.  See description of
tabs_ for details.

[edwardjung, 2017-01-05 13:38:21]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 13:38:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 14:09:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 14:09:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[edwardjung, 2017-01-05 16:56:25]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 16:56:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[edwardjung, 2017-01-05 16:59:45]

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1606: } else {
On 2017/01/03 23:11:41, sky wrote:
> no else after return (see chromium style guide for details).

Done.

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1618: if (index > -1) {
On 2017/01/03 23:11:41, sky wrote:
> When is this called with -1? I think callers should ensure they don't do that
> and this conditional should become a DCHECK.

This was a fix for ChromeVox which would call this on browser set up and would
then crash. I introduced the DCHECK but this causes immediate crashes with
ChromeVox. 

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1621: TabRendererData data =
tabstrip_->tab_at(index)->data();
On 2017/01/03 23:11:41, sky wrote:
> const TabRendererData&

Done.

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:406: if
(browser_view_ && !immersive_reveal_lock_.get()) {
On 2017/01/03 23:11:41, sky wrote:
> browser_view_ should never be null, right?

Done.

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.h (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.h:38: BrowserView*
browser_view);
On 2017/01/03 23:11:41, sky wrote:
> BrowserView has a browser() function, so that now that this depends upon
> BrowserView you shouldn't need the Browser* member.

Done.

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_strip.cc:1329: return
controller_->GetAccessibleTabName(tab);
On 2017/01/03 23:11:41, sky wrote:
> You should only call to controller_ if tab is in tabs_.  See description of
> tabs_ for details.

Done.

[sky, 2017-01-05 17:45:17]

LGTM

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1618: if (index > -1) {
On 2017/01/05 16:59:45, edwardjung wrote:
> On 2017/01/03 23:11:41, sky wrote:
> > When is this called with -1? I think callers should ensure they don't do
that
> > and this conditional should become a DCHECK.
> 
> This was a fix for ChromeVox which would call this on browser set up and would
> then crash. I introduced the DCHECK but this causes immediate crashes with
> ChromeVox. 
> 
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

Ok, fair enough. Can you make 1617 and early return, e.g.

if (index == -1)
  return std::string16(); ?

[commit-bot: I haz the power, 2017-01-05 17:55:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 17:55:13]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2017-01-05 18:41:14]

Thanks for the quick turnaround. Happy New Year!

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2581023002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1618: if (index > -1) {
On 2017/01/05 17:45:17, sky wrote:
> On 2017/01/05 16:59:45, edwardjung wrote:
> > On 2017/01/03 23:11:41, sky wrote:
> > > When is this called with -1? I think callers should ensure they don't do
> that
> > > and this conditional should become a DCHECK.
> > 
> > This was a fix for ChromeVox which would call this on browser set up and
would
> > then crash. I introduced the DCHECK but this causes immediate crashes with
> > ChromeVox. 
> > 
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> 
> Ok, fair enough. Can you make 1617 and early return, e.g.
> 
> if (index == -1)
>   return std::string16(); ?

Done. I also added a comment for this check.

[edwardjung, 2017-01-05 18:41:24]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 18:41:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 19:28:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 19:28:04]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2017-01-05 19:48:32]

The CQ bit was checked by edwardjung@chromium.org

[edwardjung, 2017-01-05 19:48:33]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2581023002/#ps200001
(title: "Return early if invalid tab index")

[commit-bot: I haz the power, 2017-01-05 19:48:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 19:54:16]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1483645712568290,
"parent_rev": "9758ade87950804041be8a2e1725aad2811f318a", "commit_rev":
"bfeeddcdf2925a4fcd7bcf9666c8af703fa35e29"}

[commit-bot: I haz the power, 2017-01-05 19:54:47]

Description was changed from

==========
Add tab status to accessibility labels
We display various visual tab indicators but there is no corresponding indicator
for accessible labels. The updated accessible label is in the format of:

Page title - Tab status

For tabs in incognito this is a format:
Page title - Tab status (incognito)

This fixes the crashes by adding a check for the active tab index.

BUG=635610
==========

to

==========
Add tab status to accessibility labels
We display various visual tab indicators but there is no corresponding indicator
for accessible labels. The updated accessible label is in the format of:

Page title - Tab status

For tabs in incognito this is a format:
Page title - Tab status (incognito)

This fixes the crashes by adding a check for the active tab index.

BUG=635610

Review-Url: https://codereview.chromium.org/2581023002
Cr-Commit-Position: refs/heads/master@{#441738}
Committed:
https://chromium.googlesource.com/chromium/src/+/bfeeddcdf2925a4fcd7bcf9666c8...
==========

[commit-bot: I haz the power, 2017-01-05 19:54:48]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/bfeeddcdf2925a4fcd7bcf9666c8...