Adds AXExpandedChanged for general roles.

Adds AXExpandedChanged for general roles.

This patch introduces AXExpandedChanged with reference to wkrev.com/168432
and modified it for chromium.

It posts AXExpandedChanged notification when aria-expanded is updated on
non Row or TreeItem role.

BUG=608300
Committed: https://crrev.com/9f72890f36f89d8704a515f1fcfc48b3f8d94dc4
Cr-Commit-Position: refs/heads/master@{#391798}

[je_julie(Not used), 2016-05-02 10:34:27]

Description was changed from

==========
Adds AXExpandedChanged for general roles.

This patch introduces AXExpandedChanged with reference to wkrev.com/168432
and modified it for chromium.

It posts AXExpandedChanged notification when aria-expanded is updated on
non Row or TreeItem role.

BUG=
==========

to

==========
Adds AXExpandedChanged for general roles.

This patch introduces AXExpandedChanged with reference to wkrev.com/168432
and modified it for chromium.

It posts AXExpandedChanged notification when aria-expanded is updated on
non Row or TreeItem role.

BUG=608300
==========

[je_julie(Not used), 2016-05-02 13:02:22]

je_julie.kim@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[je_julie(Not used), 2016-05-02 13:02:57]

Hi Dominic, PTAL.

[blink-reviews, 2016-05-02 16:02:56]

Great work as always. I have only a few comments about 
browser_accessibility_manager_mac.mm. I'll let Dominic do the rest of 
the review.
I left a TODO:
// TODO(nektar): Add events for busy and expanded changed.
Could you add your code under that TODO and modify it accordingly? Also, 
I would prefer if we use constants for everything. It's harder to 
misspell a constant because the compiler will warn you. So instead of 
@"AXExpandedChanged", you could create a constant called 
NSAccessibilityExpandedChanged at the top of the file in the anonymous 
namespace. I placed many other constants there. I think they are in the 
same order as they appear in the switch statement, so that they are 
easier to find. Lastly, could you ensure that any case we add to the 
switch statement in this file matches the order in the WebKit code? The 
reason is that it's easier in the future to quickly glance through the 
file and determine what is missing.

Something like:
     // TODO(nektar): Add an event for expanded changed.
  case ui::AX_EVENT_EXPANDED_CHANGED:
  mac_notification = NSAccessibilityExpandedChangeD;
+ break;

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

[chromium-reviews, 2016-05-02 16:02:57]

Great work as always. I have only a few comments about 
browser_accessibility_manager_mac.mm. I'll let Dominic do the rest of 
the review.
I left a TODO:
// TODO(nektar): Add events for busy and expanded changed.
Could you add your code under that TODO and modify it accordingly? Also, 
I would prefer if we use constants for everything. It's harder to 
misspell a constant because the compiler will warn you. So instead of 
@"AXExpandedChanged", you could create a constant called 
NSAccessibilityExpandedChanged at the top of the file in the anonymous 
namespace. I placed many other constants there. I think they are in the 
same order as they appear in the switch statement, so that they are 
easier to find. Lastly, could you ensure that any case we add to the 
switch statement in this file matches the order in the WebKit code? The 
reason is that it's easier in the future to quickly glance through the 
file and determine what is missing.

Something like:
     // TODO(nektar): Add an event for expanded changed.
  case ui::AX_EVENT_EXPANDED_CHANGED:
  mac_notification = NSAccessibilityExpandedChangeD;
+ break;

-- 
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.

[dmazzoni, 2016-05-02 17:37:56]

lgtm once Nektarios's concern is addressed, thanks!

https://codereview.chromium.org/1937873002/diff/1/ui/accessibility/ax_enums.idl
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/1937873002/diff/1/ui/accessibility/ax_enums.i...
ui/accessibility/ax_enums.idl:35: expanded_changed,
Nit: add a "Web" comment to this line - we're trying to keep track of which
events come from the web and which come from native views, or both.

[je_julie(Not used), 2016-05-03 12:12:09]

Patchset #2 (id:20001) has been deleted

[je_julie(Not used), 2016-05-03 14:27:57]

je_julie.kim@samsung.com changed reviewers:
+ dglazkov+blink@chromium.org, nektar@chromium.org

[je_julie(Not used), 2016-05-03 14:27:58]

Thanks for feedback.
I checked WebKit code and updated patchset addressing comment.
Nektarious, could you take another look?

dglazkov, please review for third_party/WebKit/Source/core/dom/AXObjectCache.h?

On 2016/05/02 16:02:57, chromium-reviews wrote:
> I left a TODO:
> // TODO(nektar): Add events for busy and expanded changed.
> Could you add your code under that TODO and modify it accordingly? Also, 
Done.

> I would prefer if we use constants for everything. It's harder to 
> misspell a constant because the compiler will warn you. So instead of 
> @"AXExpandedChanged", you could create a constant called 
> NSAccessibilityExpandedChanged at the top of the file in the anonymous 
> namespace. I placed many other constants there. I think they are in the 
Done.

> same order as they appear in the switch statement, so that they are 
> easier to find. Lastly, could you ensure that any case we add to the 
> switch statement in this file matches the order in the WebKit code? The 
> reason is that it's easier in the future to quickly glance through the 
> file and determine what is missing.
I checked the order from WebKit code and make switch statement match the order.

https://codereview.chromium.org/1937873002/diff/1/ui/accessibility/ax_enums.idl
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/1937873002/diff/1/ui/accessibility/ax_enums.i...
ui/accessibility/ax_enums.idl:35: expanded_changed,
On 2016/05/02 17:37:56, dmazzoni wrote:
> Nit: add a "Web" comment to this line - we're trying to keep track of which
> events come from the web and which come from native views, or both.

Done.

[je_julie(Not used), 2016-05-04 14:00:12]

Nektarious and dglazkov, PTAL.

On 2016/05/03 14:27:58, je_julie wrote:
> Thanks for feedback.
> I checked WebKit code and updated patchset addressing comment.
> Nektarious, could you take another look?
> 
> dglazkov, please review for
third_party/WebKit/Source/core/dom/AXObjectCache.h?
> 
> On 2016/05/02 16:02:57, chromium-reviews wrote:
> > I left a TODO:
> > // TODO(nektar): Add events for busy and expanded changed.
> > Could you add your code under that TODO and modify it accordingly? Also, 
> Done.
> 
> > I would prefer if we use constants for everything. It's harder to 
> > misspell a constant because the compiler will warn you. So instead of 
> > @"AXExpandedChanged", you could create a constant called 
> > NSAccessibilityExpandedChanged at the top of the file in the anonymous 
> > namespace. I placed many other constants there. I think they are in the 
> Done.
> 
> > same order as they appear in the switch statement, so that they are 
> > easier to find. Lastly, could you ensure that any case we add to the 
> > switch statement in this file matches the order in the WebKit code? The 
> > reason is that it's easier in the future to quickly glance through the 
> > file and determine what is missing.
> I checked the order from WebKit code and make switch statement match the
order.
> 
>
https://codereview.chromium.org/1937873002/diff/1/ui/accessibility/ax_enums.idl
> File ui/accessibility/ax_enums.idl (right):
> 
>
https://codereview.chromium.org/1937873002/diff/1/ui/accessibility/ax_enums.i...
> ui/accessibility/ax_enums.idl:35: expanded_changed,
> On 2016/05/02 17:37:56, dmazzoni wrote:
> > Nit: add a "Web" comment to this line - we're trying to keep track of which
> > events come from the web and which come from native views, or both.
> 
> Done.

[nektarios, 2016-05-04 14:46:39]

Just a few nits.

content/browser/accessibility/browser_accessibility_manager_mac.mm

// TODO(nektar): Add events for busy and expanded changed.
Since you added expanded_changed now, shouldn't we modify this to say
// TODO(nektar): Add event for busy.

content/test/data/accessibility/event/expanded-change-expected-mac.txt
Nit: ensure that the file ends on a newline.

/content/test/data/accessibility/event/expanded-change.html
nit

    if (list.style.visibility != 'hidden') 
    {
    
    Move "{" to the line above.
    
    Make sure the file ends on a newline.

[nektarios, 2016-05-04 14:46:57]

lgtm

[je_julie(Not used), 2016-05-05 11:06:07]

je_julie.kim@samsung.com changed reviewers:
+ schenney@chromium.org
- dglazkov+blink@chromium.org

[je_julie(Not used), 2016-05-05 11:09:37]

On 2016/05/04 14:46:57, nektarios wrote:
> lgtm

Thanks, nektarios.
I completed comment updating and editing new line.

schenney, could you take a look for AXObjectCache.h?

Missing OWNER reviewers for these files:
    third_party/WebKit/Source/core/dom/AXObjectCache.h

Suggested OWNERS: (Use "git-cl owners" to interactively select owners.)
    schenney@chromium.org

As for the trybot fails, I confirmed it's not related to this CL.

Thanks.

[haraken, 2016-05-05 11:11:56]

On 2016/05/05 11:09:37, je_julie wrote:
> On 2016/05/04 14:46:57, nektarios wrote:
> > lgtm
> 
> Thanks, nektarios.
> I completed comment updating and editing new line.
> 
> schenney, could you take a look for AXObjectCache.h?
> 
> Missing OWNER reviewers for these files:
>     third_party/WebKit/Source/core/dom/AXObjectCache.h
> 
> Suggested OWNERS: (Use "git-cl owners" to interactively select owners.)
>     mailto:schenney@chromium.org
> 
> As for the trybot fails, I confirmed it's not related to this CL.
> 
> Thanks.

owner LGTM for core/

[je_julie(Not used), 2016-05-05 12:22:14]

On 2016/05/05 11:11:56, haraken wrote:
> On 2016/05/05 11:09:37, je_julie wrote:
> > On 2016/05/04 14:46:57, nektarios wrote:
> > > lgtm
> > 
> > Thanks, nektarios.
> > I completed comment updating and editing new line.
> > 
> > schenney, could you take a look for AXObjectCache.h?
> > 
> > Missing OWNER reviewers for these files:
> >     third_party/WebKit/Source/core/dom/AXObjectCache.h
> > 
> > Suggested OWNERS: (Use "git-cl owners" to interactively select owners.)
> >     mailto:schenney@chromium.org
> > 
> > As for the trybot fails, I confirmed it's not related to this CL.
> > 
> > Thanks.
> 
> owner LGTM for core/

Thanks, haraken!

[je_julie(Not used), 2016-05-05 12:22:25]

The CQ bit was checked by je_julie.kim@samsung.com

[je_julie(Not used), 2016-05-05 12:22:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, nektar@chromium.org
Link to the patchset: https://codereview.chromium.org/1937873002/#ps60001
(title: "addressed comment")

[commit-bot: I haz the power, 2016-05-05 12:22:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1937873002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1937873002/60001

[commit-bot: I haz the power, 2016-05-05 13:54:01]

Description was changed from

==========
Adds AXExpandedChanged for general roles.

This patch introduces AXExpandedChanged with reference to wkrev.com/168432
and modified it for chromium.

It posts AXExpandedChanged notification when aria-expanded is updated on
non Row or TreeItem role.

BUG=608300
==========

to

==========
Adds AXExpandedChanged for general roles.

This patch introduces AXExpandedChanged with reference to wkrev.com/168432
and modified it for chromium.

It posts AXExpandedChanged notification when aria-expanded is updated on
non Row or TreeItem role.

BUG=608300
==========

[commit-bot: I haz the power, 2016-05-05 13:54:02]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-05 13:55:40]

Description was changed from

==========
Adds AXExpandedChanged for general roles.

This patch introduces AXExpandedChanged with reference to wkrev.com/168432
and modified it for chromium.

It posts AXExpandedChanged notification when aria-expanded is updated on
non Row or TreeItem role.

BUG=608300
==========

to

==========
Adds AXExpandedChanged for general roles.

This patch introduces AXExpandedChanged with reference to wkrev.com/168432
and modified it for chromium.

It posts AXExpandedChanged notification when aria-expanded is updated on
non Row or TreeItem role.

BUG=608300

Committed: https://crrev.com/9f72890f36f89d8704a515f1fcfc48b3f8d94dc4
Cr-Commit-Position: refs/heads/master@{#391798}
==========

[commit-bot: I haz the power, 2016-05-05 13:55:44]

Patchset 3 (id:??) landed as
https://crrev.com/9f72890f36f89d8704a515f1fcfc48b3f8d94dc4
Cr-Commit-Position: refs/heads/master@{#391798}