Added tab notification to accessibility_event_router_views.

Added tab notification to accessibility_event_router_views.
Tabs names are now accessible through touch.

BUG=393768
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285227

[dmazzoni, 2014-07-16 15:08:25]

Looks fine, just one small issue and then fix the unit test that also creates a
subclass of TabController (see below).

+pkasting for OWNERS review

https://codereview.chromium.org/393033006/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/393033006/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_controller.h:25: class TabController {
You will probably need to update tab_unittest.cc as well. Try building
"unit_tests" with ninja and then run out/Debug/unit_tests
--gtest_filter="TabTest*"

https://codereview.chromium.org/393033006/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_controller.h:97: virtual int tab_count() const
= 0;
As a convention, we only use the lowercase_with_underscores style for simple
accessors that aren't virtual. So change this to GetTabCount(), and change other
places in the code that call it.

[evy, 2014-07-16 16:44:26]

https://codereview.chromium.org/393033006/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/393033006/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_controller.h:25: class TabController {
On 2014/07/16 15:08:25, dmazzoni wrote:
> You will probably need to update tab_unittest.cc as well. Try building
> "unit_tests" with ninja and then run out/Debug/unit_tests
> --gtest_filter="TabTest*"

Done. Is it okay if they return 0 for now?

https://codereview.chromium.org/393033006/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_controller.h:97: virtual int tab_count() const
= 0;
On 2014/07/16 15:08:25, dmazzoni wrote:
> As a convention, we only use the lowercase_with_underscores style for simple
> accessors that aren't virtual. So change this to GetTabCount(), and change
other
> places in the code that call it.

Done.

[dmazzoni, 2014-07-16 17:06:59]

On Wed, Jul 16, 2014 at 9:44 AM, <evy@chromium.org> wrote:

>
> https://codereview.chromium.org/393033006/diff/1/chrome/
> browser/ui/views/tabs/tab_controller.h
> File chrome/browser/ui/views/tabs/tab_controller.h (right):
>
> https://codereview.chromium.org/393033006/diff/1/chrome/
> browser/ui/views/tabs/tab_controller.h#newcode25
> chrome/browser/ui/views/tabs/tab_controller.h:25: class TabController {
> On 2014/07/16 15:08:25, dmazzoni wrote:
>
>> You will probably need to update tab_unittest.cc as well. Try building
>> "unit_tests" with ninja and then run out/Debug/unit_tests
>> --gtest_filter="TabTest*"
>>
>
> Done. Is it okay if they return 0 for now?


If the tests pass then it's fine.

I'd probably return "1" for GetTabCount because 0 doesn't make sense.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[evy, 2014-07-16 17:15:36]

On 2014/07/16 17:06:59, dmazzoni wrote:
> On Wed, Jul 16, 2014 at 9:44 AM, <mailto:evy@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/393033006/diff/1/chrome/
> > browser/ui/views/tabs/tab_controller.h
> > File chrome/browser/ui/views/tabs/tab_controller.h (right):
> >
> > https://codereview.chromium.org/393033006/diff/1/chrome/
> > browser/ui/views/tabs/tab_controller.h#newcode25
> > chrome/browser/ui/views/tabs/tab_controller.h:25: class TabController {
> > On 2014/07/16 15:08:25, dmazzoni wrote:
> >
> >> You will probably need to update tab_unittest.cc as well. Try building
> >> "unit_tests" with ninja and then run out/Debug/unit_tests
> >> --gtest_filter="TabTest*"
> >>
> >
> > Done. Is it okay if they return 0 for now?
> 
> 
> If the tests pass then it's fine.
> 
> I'd probably return "1" for GetTabCount because 0 doesn't make sense.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sounds good - reuploaded

[Peter Kasting, 2014-07-16 23:40:54]

It looks OK to me, but sky should review this as the owner of all things tabs to
ensure the new virtual methods you're adding are the right way to go.

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:299:
std::string context = GetViewContext(view);
Nit: I suggest inlining these two below to avoid temps

[sky, 2014-07-17 04:06:10]

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_controller.h:94: virtual int
GetModelIndexOfTab(const Tab* tab) const = 0;
If we can I would like to avoid exposing this state. Could you instead add an
explicit function that adds the state you care about for accessibility.
Something like GetTabAccessibilityState(Tab* ...)?

[evy, 2014-07-21 17:54:15]

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:299:
std::string context = GetViewContext(view);
On 2014/07/16 23:40:54, Peter Kasting wrote:
> Nit: I suggest inlining these two below to avoid temps

This pattern was actually copied from another function in this file.
Would it be good to change all of the places this pattern occurs?

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_controller.h:94: virtual int
GetModelIndexOfTab(const Tab* tab) const = 0;
On 2014/07/17 04:06:10, sky wrote:
> If we can I would like to avoid exposing this state. Could you instead add an
> explicit function that adds the state you care about for accessibility.
> Something like GetTabAccessibilityState(Tab* ...)?

That's a great way to do it! Thanks

I wasn't sure if you meant "explicit" as in using the explicit keyword - I'm not
as familiar with it. I looked into it a bit and I noticed it's mostly used for
constructors, so I didn't include it here but I figured I'd ask just in case.

[Peter Kasting, 2014-07-21 19:06:07]

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/393033006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:299:
std::string context = GetViewContext(view);
On 2014/07/21 17:54:15, evy wrote:
> On 2014/07/16 23:40:54, Peter Kasting wrote:
> > Nit: I suggest inlining these two below to avoid temps
> 
> This pattern was actually copied from another function in this file.
> Would it be good to change all of the places this pattern occurs?

You can if you want.  It doesn't matter much to me either way.

[sky, 2014-07-23 17:38:42]

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_controller.h:94: ui::AXViewState* state) = 0;
nit: fix indentation.

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:1187: ui::AXViewState* state) {
nit: indentation.

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:1189: state->index =
GetModelIndexOfTab(tab);
Do you care if this returns -1?

[evy, 2014-07-23 18:44:11]

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_controller.h:94: ui::AXViewState* state) = 0;
On 2014/07/23 17:38:42, sky wrote:
> nit: fix indentation.

Done.

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:1187: ui::AXViewState* state) {
On 2014/07/23 17:38:42, sky wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:1189: state->index =
GetModelIndexOfTab(tab);
On 2014/07/23 17:38:42, sky wrote:
> Do you care if this returns -1?

Is there a case where the user hovers over/touches an existing tab and the index
is -1?
And in this case, I think the speech would say "tab 0 of $COUNT" - do you think
we should do something to change what's read in this case?

[sky, 2014-07-23 19:15:34]

On Wed, Jul 23, 2014 at 11:44 AM,  <evy@chromium.org> wrote:
>
>
https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/tabs/tab_controller.h (right):
>
>
https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/tabs/tab_controller.h:94: ui::AXViewState*
> state) = 0;
> On 2014/07/23 17:38:42, sky wrote:
>>
>> nit: fix indentation.
>
>
> Done.
>
>
>
https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
>
>
https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/tabs/tab_strip.cc:1187: ui::AXViewState* state)
> {
> On 2014/07/23 17:38:42, sky wrote:
>>
>> nit: indentation.
>
>
> Done.
>
>
>
https://codereview.chromium.org/393033006/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/tabs/tab_strip.cc:1189: state->index =
> GetModelIndexOfTab(tab);
> On 2014/07/23 17:38:42, sky wrote:
>>
>> Do you care if this returns -1?
>
>
> Is there a case where the user hovers over/touches an existing tab and
> the index is -1?

If the tab is in the process of closing index -1.

> And in this case, I think the speech would say "tab 0 of $COUNT" - do
> you think we should do something to change what's read in this case?

I don't know enough about expectations to say what should happen. If
the tab is closing it'll go away soon. Will the speech APIs detect
this and report the new tab when the close is done?

  -Scott

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-07-23 21:07:50]

>
> I don't know enough about expectations to say what should happen. If
> the tab is closing it'll go away soon. Will the speech APIs detect
> this and report the new tab when the close is done?
>

I think the only time this is going to happen is if you touch a tab while
it's in the process of going away. In that case I'd be just as happy if it
says nothing. You could modify accessibility_event_router_views to return
if it gets an index or size of -1 from a tab.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-07-23 23:39:33]

LGTM

[evy1, 2014-07-24 03:18:04]

The CQ bit was checked by evy@google.com

[commit-bot: I haz the power, 2014-07-24 03:19:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/evy@chromium.org/393033006/120001

[commit-bot: I haz the power, 2014-07-24 13:32:54]

Change committed as 285227