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)

BUG=635610
Committed: https://crrev.com/e67fb93083c60c0189cb06f272eca9c1672f62a8
Cr-Commit-Position: refs/heads/master@{#438128}

[dmazzoni, 2016-11-14 21:59:01]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2016-11-14 21:59:02]

Overall this approach seems reasonable to me, you
should probably reach out to other owners for any
other thoughts

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_utils.cc:63: const int kNetworkStateError = 3;
Can you use TabRendererData::NetworkState?

Maybe that should be pulled up to a more general source
file?

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.cc:324:
tab_label.append(l10n_util::GetStringUTF16(IDS_TAB_AX_LABEL_NETWORK_ERROR));
Maybe need whitesapce between the existing tab title and the extra label?

Or maybe a hyphen, like the Chrome main window title?

"Google Hangouts - Audio Playing"

[edwardjung, 2016-11-18 18:13:10]

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

[commit-bot: I haz the power, 2016-11-18 18:14:09]

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-11-18 18:18:12]

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

[commit-bot: I haz the power, 2016-11-18 18:18:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[edwardjung, 2016-11-18 18:25:49]

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

[commit-bot: I haz the power, 2016-11-18 18:26:58]

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-11-18 19:03:53]

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

[commit-bot: I haz the power, 2016-11-18 19:03:53]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[edwardjung, 2016-11-22 12:40:31]

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

[commit-bot: I haz the power, 2016-11-22 12:40:50]

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-11-22 12:53:12]

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

[commit-bot: I haz the power, 2016-11-22 12:53:13]

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-11-22 13:58:37]

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

[commit-bot: I haz the power, 2016-11-22 13:58:54]

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-11-22 14:53:47]

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

[commit-bot: I haz the power, 2016-11-22 14:53:47]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2016-11-24 11:36:42]

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

[edwardjung, 2016-11-24 11:36:43]

@sky PTAL, let me know if this approach is reasonable. 

Thanks.

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_utils.cc:63: const int kNetworkStateError = 3;
On 2016/11/14 21:59:02, dmazzoni wrote:
> Can you use TabRendererData::NetworkState?
> 
> Maybe that should be pulled up to a more general source
> file?
> 

Makes sense, separating this tooltip change into a new CL.

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2497373003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.cc:324:
tab_label.append(l10n_util::GetStringUTF16(IDS_TAB_AX_LABEL_NETWORK_ERROR));
> Or maybe a hyphen, like the Chrome main window title?
> 
> "Google Hangouts - Audio Playing"

Switched to hyphen. Also on the advice of lpalmaro@, 'incognito' is appended in
addition to any other tab states.

[edwardjung, 2016-11-24 11:41:06]

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)

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

[sky, 2016-11-26 17:19:33]

https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (left):

https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1603: return
browser_->GetWindowTitleForCurrentTab(include_app_name);
GetWindowTitleForCurrentTab() has a couple of heuristics for getting the title
that you're missing. I would prefer GetAccessibleTabLabel is here, and not the
in the tab class, mostly because I think you want parts of what
GetWindowTitleForCurrentTab does.

[edwardjung, 2016-12-05 19:02:21]

On 2016/11/26 17:19:33, sky wrote:
>
https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
> File chrome/browser/ui/views/frame/browser_view.cc (left):
> 
>
https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
> chrome/browser/ui/views/frame/browser_view.cc:1603: return
> browser_->GetWindowTitleForCurrentTab(include_app_name);
> GetWindowTitleForCurrentTab() has a couple of heuristics for getting the title
> that you're missing. I would prefer GetAccessibleTabLabel is here, and not the
> in the tab class, mostly because I think you want parts of what
> GetWindowTitleForCurrentTab does.

If I move GetAccessibleTabLabel to BrowserView, what's the best way to access
BrowserView from Tab?

[sky, 2016-12-05 22:37:13]

It'll be a couple of hops. Tab has a TabController (that is the
TabStrip) and TabStrip has a TabStripController. TabStripController is
implemented by BrowserTabStripController and knows about BrowserView.

On Mon, Dec 5, 2016 at 11:02 AM,  <edwardjung@chromium.org> wrote:
> On 2016/11/26 17:19:33, sky wrote:
>>
>
https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
>> File chrome/browser/ui/views/frame/browser_view.cc (left):
>>
>>
>
https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
>> chrome/browser/ui/views/frame/browser_view.cc:1603: return
>> browser_->GetWindowTitleForCurrentTab(include_app_name);
>> GetWindowTitleForCurrentTab() has a couple of heuristics for getting the
>> title
>> that you're missing. I would prefer GetAccessibleTabLabel is here, and not
>> the
>> in the tab class, mostly because I think you want parts of what
>> GetWindowTitleForCurrentTab does.
>
> If I move GetAccessibleTabLabel to BrowserView, what's the best way to
> access
> BrowserView from Tab?
>
> https://codereview.chromium.org/2497373003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[edwardjung, 2016-12-06 16:13:58]

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

[commit-bot: I haz the power, 2016-12-06 16:14:10]

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-06 18:04:00]

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

[commit-bot: I haz the power, 2016-12-06 18:04:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[edwardjung, 2016-12-06 20:23:16]

Thanks for the pointers, I've made the change. 

One think I'm not having a problem with is that a couple of the unit tests
crash. Probably the way I've implemented getting the BrowserView from Tab.

Log below: 

[6507/6507] TabStripTest.PinnedTabTitleChangedIndicatorHidesOnSelect (CRASHED)
Retrying 2 tests (retry #3)
[ RUN      ] TabStripTest.TabCloseButtonVisibilityWhenStacked

Backtrace:

	Tab::GetAccessibleNodeData [0x03DD92E0+67]

	views::NativeViewAccessibility::GetData [0x02D58840+96]

	ui::AXPlatformNodeBase::GetData [0x03664945+70]

	ui::AXPlatformNodeWin::NotifyAccessibilityEvent [0x036631DF+38]

	views::View::NotifyAccessibilityEvent [0x02D37E47+88]

	TabStrip::SetSelection [0x03DED533+328]

	FakeBaseTabStripController::SelectTab [0x017CC9DE+75]

	TabStrip::SelectTab [0x03DED12D+45]

	TabStripTest_TabCloseButtonVisibilityWhenStacked_Test::TestBody
[0x017D14A2+1788]

	testing::internal::HandleExceptionsInMethodIfSupported<testing::TestCase,void>
[0x02925906+32]

	testing::Test::Run [0x0292C8DC+93]

	testing::TestInfo::Run [0x0292CA5D+126]

	testing::TestCase::Run [0x0292C988+133]

	testing::internal::UnitTestImpl::RunAllTests [0x0292CD07+433]

	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
[0x0292594A+32]

	testing::UnitTest::Run [0x0292CB31+133]

	base::TestSuite::Run [0x02755B8D+95]

	base::internal::RunMixin<base::Callback<bool __cdecl(void),1,1> >::Run
[0x00F067D6+25]

	base::LaunchUnitTests [0x02757676+752]

	base::LaunchUnitTests [0x027573D3+77]

	main [0x04EBACDE+176]

	__scrt_common_main_seh [0x04E810FF+249]
(f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)

	BaseThreadInitThunk [0x771F336A+18]

	RtlInitializeExceptionChain [0x77A89882+99]

	RtlInitializeExceptionChain [0x77A89855+54]

[6508/6509] TabStripTest.TabCloseButtonVisibilityWhenStacked (CRASHED)
[ RUN      ] TabStripTest.PinnedTabTitleChangedIndicatorHidesOnSelect

Backtrace:

	Tab::GetAccessibleNodeData [0x03DD92E0+67]

	views::NativeViewAccessibility::GetData [0x02D58840+96]

	ui::AXPlatformNodeBase::GetData [0x03664945+70]

	ui::AXPlatformNodeWin::NotifyAccessibilityEvent [0x036631DF+38]

	views::View::NotifyAccessibilityEvent [0x02D37E47+88]

	TabStrip::SetSelection [0x03DED533+328]

	FakeBaseTabStripController::SelectTab [0x017CC9DE+75]

	TabStripTest_PinnedTabTitleChangedIndicatorHidesOnSelect_Test::TestBody
[0x017D08FC+616]

	testing::internal::HandleExceptionsInMethodIfSupported<testing::TestCase,void>
[0x02925906+32]

	testing::Test::Run [0x0292C8DC+93]

	testing::TestInfo::Run [0x0292CA5D+126]

	testing::TestCase::Run [0x0292C988+133]

	testing::internal::UnitTestImpl::RunAllTests [0x0292CD07+433]

	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
[0x0292594A+32]

	testing::UnitTest::Run [0x0292CB31+133]

	base::TestSuite::Run [0x02755B8D+95]

	base::internal::RunMixin<base::Callback<bool __cdecl(void),1,1> >::Run
[0x00F067D6+25]

	base::LaunchUnitTests [0x02757676+752]

	base::LaunchUnitTests [0x027573D3+77]

	main [0x04EBACDE+176]

	__scrt_common_main_seh [0x04E810FF+249]
(f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)

	BaseThreadInitThunk [0x771F336A+18]

	RtlInitializeExceptionChain [0x77A89882+99]

	RtlInitializeExceptionChain [0x77A89855+54]

https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (left):

https://codereview.chromium.org/2497373003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1603: return
browser_->GetWindowTitleForCurrentTab(include_app_name);
On 2016/11/26 17:19:33, sky wrote:
> GetWindowTitleForCurrentTab() has a couple of heuristics for getting the title
> that you're missing. I would prefer GetAccessibleTabLabel is here, and not the
> in the tab class, mostly because I think you want parts of what
> GetWindowTitleForCurrentTab does.

Done.

[sky, 2016-12-06 23:18:33]

https://codereview.chromium.org/2497373003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2497373003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1108: BrowserTabStripController*
browser_tab_strip_controller =
You shouldn't directly extract these values, rather you should add functions to
TabController and TabStripController that route to the right place. Specifically
TabController::GetAccessibleTabName, that is implemented by TabStrip and calls
to TabStripController::GetAccessibleTabName. BrowserTabStripController can then
have the logic you have here.

[edwardjung, 2016-12-12 12:41:53]

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

[commit-bot: I haz the power, 2016-12-12 12:42:02]

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-12 13:06:53]

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

[commit-bot: I haz the power, 2016-12-12 13:06:54]

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-12 15:27:08]

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

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

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-12 16:27:07]

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

[commit-bot: I haz the power, 2016-12-12 16:27:07]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2016-12-12 16:50:43]

https://codereview.chromium.org/2497373003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2497373003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1108: BrowserTabStripController*
browser_tab_strip_controller =
Done, thanks for the pointers.

On 2016/12/06 23:18:32, sky wrote:
> You shouldn't directly extract these values, rather you should add functions
to
> TabController and TabStripController that route to the right place.
Specifically
> TabController::GetAccessibleTabName, that is implemented by TabStrip and calls
> to TabStripController::GetAccessibleTabName. BrowserTabStripController can
then
> have the logic you have here.

[sky, 2016-12-12 17:25:09]

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

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1650: default:
You shouldn't need the default state.

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.h:475: // Creates an accessible tab
label for screen readers that includes the tab
Please move this above testing interface functions (better to keep those grouped
at the end).

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.h:478: TabRendererData data) const;
const TabRendererData&

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_controller.h:132: // Returns the accessible tab
name for the current tab.
current -> this

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_unittest.cc (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_unittest.cc:87: };
no ;

[dmazzoni, 2016-12-12 18:26:01]

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14283: +        <ph
name="WINDOW_TITLE">$1</ph> - Camera or microphone recording
It might be better if the hyphen and whitespace is not part of the translated
string.

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14286: +        <ph
name="WINDOW_TITLE">$1</ph> - Tab content shared
This might need more explanation. It's not clear at all to me how "tab content
is being captured" relates to "Tab content shared". Please describe this more
clearly so translators can pick the best translation of "shared"

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14303: +      <message
name="IDS_TAB_AX_LABEL_CRASHED_FORMAT" desc="Accessibility label text, when tab
has a network error">
Update description (should say "when tab has crashed")

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc:154: return
title;
Just return base::string16();

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_unittest.cc (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_unittest.cc:86: return title;
Same: just return base::string16();

[dmazzoni, 2016-12-12 18:32:18]

I'm just thinking of the potential for error for
translating the strings with hyphens and whitespace
is high.

I realize that GetWindowTitleForCurrentTab does it
this way now, with a string "$1 - Google Chrome".

How about a bunch of strings for the tab status, like
"Network error" and "Audio playing", then a string
that joins them, like "$1 - $2"? Or am I overthinking
this?

[edwardjung, 2016-12-12 19:16:21]

> How about a bunch of strings for the tab status, like
> "Network error" and "Audio playing", then a string
> that joins them, like "$1 - $2"? Or am I overthinking
> this?

I don't think this a particularly complicated string to translate compared to
the network error ones I've constructed in the past. I think adding examples in
the description and within the PH tag would help make the string clearer.

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14283: +        <ph
name="WINDOW_TITLE">$1</ph> - Camera or microphone recording
On 2016/12/12 18:26:01, dmazzoni wrote:
> It might be better if the hyphen and whitespace is not part of the translated
> string.

There a number of other strings that use hyphen as a separator before a status
such as IDS_DEVICE_CHOOSER_PAIRED_STATUS_TEXT and IDS
DOWNLOAD_STATUS_INTERRUPTED so I'm not so worried about this. I can provide an
example for each string in the description if that helps.

Accessibility label text, when the tab is recording media. Example: 'Google
Hangouts - Camera or microphone recording'

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14286: +        <ph
name="WINDOW_TITLE">$1</ph> - Tab content shared
On 2016/12/12 18:26:00, dmazzoni wrote:
> This might need more explanation. It's not clear at all to me how "tab content
> is being captured" relates to "Tab content shared". Please describe this more
> clearly so translators can pick the best translation of "shared"

Done.

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14303: +      <message
name="IDS_TAB_AX_LABEL_CRASHED_FORMAT" desc="Accessibility label text, when tab
has a network error">
On 2016/12/12 18:26:00, dmazzoni wrote:
> Update description (should say "when tab has crashed")

Done.

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

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1650: default:
On 2016/12/12 17:25:08, sky wrote:
> You shouldn't need the default state.

Done.

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.h:475: // Creates an accessible tab
label for screen readers that includes the tab
On 2016/12/12 17:25:09, sky wrote:
> Please move this above testing interface functions (better to keep those
grouped
> at the end).

Done.

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.h:478: TabRendererData data) const;
On 2016/12/12 17:25:08, sky wrote:
> const TabRendererData&

Done.

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc:154: return
title;
On 2016/12/12 18:26:01, dmazzoni wrote:
> Just return base::string16();

Done.

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_controller.h:132: // Returns the accessible tab
name for the current tab.
On 2016/12/12 17:25:09, sky wrote:
> current -> this

Done.

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_unittest.cc (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_unittest.cc:86: return title;
On 2016/12/12 18:26:01, dmazzoni wrote:
> Same: just return base::string16();

Done.

https://codereview.chromium.org/2497373003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_unittest.cc:87: };
On 2016/12/12 17:25:09, sky wrote:
> no ;

Done.

[dmazzoni, 2016-12-12 19:22:46]

lgtm

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14283: +        <ph
name="WINDOW_TITLE">$1</ph> - Camera or microphone recording
On 2016/12/12 19:16:20, edwardjung wrote:
> On 2016/12/12 18:26:01, dmazzoni wrote:
> > It might be better if the hyphen and whitespace is not part of the
translated
> > string.
> 
> There a number of other strings that use hyphen as a separator before a status
> such as IDS_DEVICE_CHOOSER_PAIRED_STATUS_TEXT and IDS
> DOWNLOAD_STATUS_INTERRUPTED so I'm not so worried about this. I can provide an
> example for each string in the description if that helps.
> 
> Accessibility label text, when the tab is recording media. Example: 'Google
> Hangouts - Camera or microphone recording'
> 

Yes, an example would help.

[edwardjung, 2016-12-12 19:43:00]

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2497373003/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:14283: +        <ph
name="WINDOW_TITLE">$1</ph> - Camera or microphone recording
On 2016/12/12 19:22:46, dmazzoni wrote:
> On 2016/12/12 19:16:20, edwardjung wrote:
> > On 2016/12/12 18:26:01, dmazzoni wrote:
> > > It might be better if the hyphen and whitespace is not part of the
> translated
> > > string.
> > 
> > There a number of other strings that use hyphen as a separator before a
status
> > such as IDS_DEVICE_CHOOSER_PAIRED_STATUS_TEXT and IDS
> > DOWNLOAD_STATUS_INTERRUPTED so I'm not so worried about this. I can provide
an
> > example for each string in the description if that helps.
> > 
> > Accessibility label text, when the tab is recording media. Example: 'Google
> > Hangouts - Camera or microphone recording'
> > 
> 
> Yes, an example would help.

Added examples to all of these strings

[sky, 2016-12-12 21:06:00]

LGTM

[edwardjung, 2016-12-13 10:33:41]

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

[commit-bot: I haz the power, 2016-12-13 10:34:11]

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-13 11:29:02]

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

[commit-bot: I haz the power, 2016-12-13 11:29:02]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2016-12-13 11:30:06]

The CQ bit was checked by edwardjung@chromium.org

[edwardjung, 2016-12-13 11:30:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2497373003/#ps260001
(title: "Add examples to new strings")

[commit-bot: I haz the power, 2016-12-13 11:30:31]

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-13 11:37:10]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1481628606729980,
"parent_rev": "2c79b9009c73d32c5805f5450770b67ad4df9f4d", "commit_rev":
"9bacff8f15c7c8b6aa4790e937558316e5860f16"}

[commit-bot: I haz the power, 2016-12-13 11:37:40]

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)

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)

BUG=635610

Review-Url: https://codereview.chromium.org/2497373003
==========

[commit-bot: I haz the power, 2016-12-13 11:37:42]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-12-13 11:39:08]

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)

BUG=635610

Review-Url: https://codereview.chromium.org/2497373003
==========

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)

BUG=635610

Committed: https://crrev.com/e67fb93083c60c0189cb06f272eca9c1672f62a8
Cr-Commit-Position: refs/heads/master@{#438128}
==========

[commit-bot: I haz the power, 2016-12-13 11:39:10]

Patchset 14 (id:??) landed as
https://crrev.com/e67fb93083c60c0189cb06f272eca9c1672f62a8
Cr-Commit-Position: refs/heads/master@{#438128}