Complete live region support in ChromeVox Next.

Complete live region support in ChromeVox Next.

Optimizes tree change notifications so that ChromeVox can only listen to
those relevant to live regions, and skip the rest.

Implements aria-atomic, and adds some code to avoid duplicate speaking of
changes to a live region due to multiple tree changes.

Improves speech queue mode support by always flushing following any key
event, queueing otherwise, and doing a category flush when a new live
region event happens 500 ms after the last one.

The last one is a heuristic, but the idea is that multiple live region
events at the same time should queue up, but a new live region some
discrete time later should interrupt previous live region events.

Adds some tests for live region output including flushing and queueing
behavior.

Manual tests include the live region test suite created for Google Docs
testing. There are probably some bugs and corner cases left, but this gets
us a lot closer.

BUG=478217
Committed: https://crrev.com/eaa0c42071267f268cf77b5fc66fbfc9b12d2e41
Cr-Commit-Position: refs/heads/master@{#362500}

[dmazzoni, 2015-11-20 00:08:02]

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

[Peter Lundblad, 2015-11-20 13:42:59]

Hi,

Nice!  IN addition to the comments below, I'd consider moving the
live region specific stuff out of background.js to keep that file managable.
Perhaps unit test some of that code separately.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:313:
* Set the type of tree change events you want to receive when
Did you autogenerate this or can you leave out the redundant documentation?
Not sure why it's there in some other places in this file.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:285:
AutomationUtil.isInSameWebpage = function(a, b) {
Could you add a unit test for this one?

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:290:
while (a) {
nit:
while (a && a.parent && AutomationUtil.isInSameTree(a.parent, a))

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:299:
while (b) {
Consider adding a utility function for this.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:86: *
Ids of nodes that have been announced as part of a live region since
nit: there are no ids stored in the set.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:88: *
@type {WeakSet}
{!WeakSet}
Does the compiler support
{!WeakSet<AutomationNode>}
? It worked when I used for a Map.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:531:
var skip = false;
Get rid of this and return early instead?

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:648: get
empty() {
Rename to hasSpeech or similar to make clear from the name that this is about
speech.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:188:
expectQueue: function(text) {
expectQueuedSpeech?

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:197:
expectFlush: function(text) {
expectFlushingSpeech?

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:207:
expectCategoryFlush: function(text) {
expectCategoryFlushSpeech?

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:582: [nocompile] static void
setTreeChangeObserverMask(
I assume this mask is per extension.  IN any case, since you can add more
than one observer, I think it'd make sense to allow a separate mask per
observer. That would allow diferent parts of the user of this api todo
be less coupled.

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:585:
tree_change_observer_mask_ =
What catches invalid values?

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.h (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.h:176:
api::automation::TreeChangeObserverMask tree_change_observer_mask_;
Just to make sure, thee's one of these per extension, right?

https://codereview.chromium.org/1457683009/diff/1/content/renderer/accessibil...
File content/renderer/accessibility/renderer_accessibility.cc (right):

https://codereview.chromium.org/1457683009/diff/1/content/renderer/accessibil...
content/renderer/accessibility/renderer_accessibility.cc:280: LOG(ERROR) <<
"Accessibility event: " << ui::ToString(event.event_type)
We all do the same... :-)

[dmazzoni, 2015-11-23 20:16:50]

I moved live regions handling and tests to separate files.

After discussing with David I added a new class ChromeVoxState that abstracts
the interface for accessing the ChromeVox mode and current range. I also got rid
of the setter for the range because it has side effects.

I think we should consider using this instead of calling global.backgroundObj,
but I didn't want to turn this change into a complete refactoring of everything,
I think it's large enough.

If you're reasonably happy with this, let's continue to evolve ChromeVoxState
and try to refactor other files to remove dependencies on Background.

Suggestions for other names other than ChromeVoxState are welcome. I'd also like
to consider renaming Background. If you don't have strong feelings, let's just
land this change with this name and rename both in a follow-up.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:313:
* Set the type of tree change events you want to receive when
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> Did you autogenerate this or can you leave out the redundant documentation?
> Not sure why it's there in some other places in this file.

I deleted the redundant documentation from this and nearby functions. The
autogeneration is still too problematic, I tried it before but spent more time
cleaning it up and merging with previous manual fixes that it wasn't worth it.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:285:
AutomationUtil.isInSameWebpage = function(a, b) {
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> Could you add a unit test for this one?

Sure, done. I had to modify runWithLoadedTree to support loading multiple tabs.
That might be useful for future tests.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:290:
while (a) {
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> nit:
> while (a && a.parent && AutomationUtil.isInSameTree(a.parent, a))

Done.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:299:
while (b) {
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> Consider adding a utility function for this.

Doesn't seem necessary now that it's a one-line loop and wouldn't be needed
elsewhere. Let me know if you'd still prefer that.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:86: *
Ids of nodes that have been announced as part of a live region since
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> nit: there are no ids stored in the set.

Done.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:88: *
@type {WeakSet}
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> {!WeakSet}
> Does the compiler support
> {!WeakSet<AutomationNode>}
> ? It worked when I used for a Map.

It worked! Thanks.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:531:
var skip = false;
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> Get rid of this and return early instead?

Done.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:648: get
empty() {
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> Rename to hasSpeech or similar to make clear from the name that this is about
> speech. 

Done.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:188:
expectQueue: function(text) {
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> expectQueuedSpeech?

Done.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:197:
expectFlush: function(text) {
On 2015/11/20 13:42:59, Peter Lundblad wrote:
> expectFlushingSpeech?

Done.

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:207:
expectCategoryFlush: function(text) {
On 2015/11/20 13:42:58, Peter Lundblad wrote:
> expectCategoryFlushSpeech?

Done.

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:582: [nocompile] static void
setTreeChangeObserverMask(
On 2015/11/20 13:42:59, Peter Lundblad wrote:
> I assume this mask is per extension.  IN any case, since you can add more
> than one observer, I think it'd make sense to allow a separate mask per
> observer. That would allow diferent parts of the user of this api todo
> be less coupled.

That would be ideal, but it would significantly complicate the implementation so
I'd prefer to leave it like this for now.

The issue is that the mask is implemented in C++, but the registration of
observers happens in the JS bindings layer. The significant performance benefit
of this change happens because we can avoid calls from C++ into JS for the vast
majority of tree changes now.

I'm open to fixing this later if we actually need it. OK to leave it like this
for now?

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:585:
tree_change_observer_mask_ =
On 2015/11/20 13:42:59, Peter Lundblad wrote:
> What catches invalid values?

ParseTreeChangeObserverMask is autogenerated, it throws an exception if it's not
passed a string that exactly matches one of the enum values, so additional
checks aren't necessary.

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.h (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.h:176:
api::automation::TreeChangeObserverMask tree_change_observer_mask_;
On 2015/11/20 13:42:59, Peter Lundblad wrote:
> Just to make sure, thee's one of these per extension, right?

Yes.

https://codereview.chromium.org/1457683009/diff/1/content/renderer/accessibil...
File content/renderer/accessibility/renderer_accessibility.cc (right):

https://codereview.chromium.org/1457683009/diff/1/content/renderer/accessibil...
content/renderer/accessibility/renderer_accessibility.cc:280: LOG(ERROR) <<
"Accessibility event: " << ui::ToString(event.event_type)
On 2015/11/20 13:42:59, Peter Lundblad wrote:
> We all do the same... :-)

Yeah, this should probably be VLOG(1) or something else we can change at
runtime.

[David Tseng, 2015-11-23 23:23:57]

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

[David Tseng, 2015-11-23 23:23:57]

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:132:
*/
Move this as well?

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:66:
global.backgroundObj.setCurrentRange(cursors.Range.fromNode(node));
Chang this to ChromeVoxState.

[dmazzoni, 2015-11-23 23:51:02]

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:66:
global.backgroundObj.setCurrentRange(cursors.Range.fromNode(node));
On 2015/11/23 23:23:57, David Tseng wrote:
> Chang this to ChromeVoxState.

Should Background pass ChromeVoxState to the DesktopAutomationHandler
constructor, or should there be a global.chromeVoxState that can be accessed
from anywhere?

I'm somewhat leaning towards trying to do everything with dependency injection,
it could benefit testing.

[Peter Lundblad, 2015-11-24 11:04:31]

lgtm

A few minor comments and will follow up on some of your responses
in a separate reply.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs:125:
});
Could test with one node being in the desktop tree as well.
Also, is it too annoying to test with iframes which will mean (or already do?)
multiple trees in a web page?

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:133:
Background.LIVE_REGION_QUEUE_TIME_MS = 500;
Move to LiveRegionHandler?

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js:64:
getCurrentRange: function() {
Perhaps make the get* methods private to enforce consistency.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js:71:
setCurrentRange: function(newRange) {
How about making all these 'stub' methods goog.abstract so they throw?
Then create a proper stub class if needed for testing.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:56:
* @param {chrome.automation.TreeChange} treeChange
Suggest using goog.scope like in some other files to make type names less
verbose.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:72:
if (!AutomationUtil.isInSameWebpage(node, currentRange.start.node))
Doesn't classic announce live region updates from background tabs?
I am not sure I personally think it is the right behaviour, but I think it has
been requested in the past and changed according to such request.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:94:
* @param {?string} opt_prependFormatStr If set, a format string for
Optional parameters should have an = suffix.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:110:
if (opt_prependFormatStr) {
nit: bracket inconsistency.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:115:
if (!output.hasSpeech && node.liveAtomic) {
Ditto.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions_test.extjs
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions_test.extjs:40:
* Create a function which perform the command |cmd|.
nit: performs

https://codereview.chromium.org/1457683009/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:588: // calling
addTreeChangeObserver.
If we keep this design, please add a note that the mask affects all listeners.

https://codereview.chromium.org/1457683009/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:594: // setTreeChangeObserverMask
for this to do anything.
Replace 'this to do anything' with 'any listeners to be called' or similar
to be more precise.

[Peter Lundblad, 2015-11-24 11:05:43]

dmazzoni@chromium.org writes:
> 
>
https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
> File
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
> (right):
> 
>
https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:66:
> global.backgroundObj.setCurrentRange(cursors.Range.fromNode(node));
> On 2015/11/23 23:23:57, David Tseng wrote:
> > Chang this to ChromeVoxState.
> 
> Should Background pass ChromeVoxState to the DesktopAutomationHandler
> constructor, or should there be a global.chromeVoxState that can be
> accessed from anywhere?
> 
> I'm somewhat leaning towards trying to do everything with dependency
> injection, it could benefit testing.

I agree. Having global objects is convenient but easily gets out of hand.

> https://codereview.chromium.org/1457683009/

-- 

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

[Peter Lundblad, 2015-11-24 13:03:26]

dmazzoni@chromium.org writes:
> I moved live regions handling and tests to separate files.
> 
> After discussing with David I added a new class ChromeVoxState that  
> abstracts
> the interface for accessing the ChromeVox mode and current range. I also  
> got rid
> of the setter for the range because it has side effects.

Good stuff!

> I think we should consider using this instead of calling  
> global.backgroundObj,
> but I didn't want to turn this change into a complete refactoring of  
> everything,
> I think it's large enough.

Agreed.

> Suggestions for other names other than ChromeVoxState are welcome. I'd also  
> like
> to consider renaming Background. If you don't have strong feelings, let's  
> just
> land this change with this name and rename both in a follow-up.

sgtm. I was thinkning NavigationState, but maybe that's too specific.

>
https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
> File
>
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
> (right):
> 
>
https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
>
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:313:
> * Set the type of tree change events you want to receive when
> On 2015/11/20 13:42:58, Peter Lundblad wrote:
> > Did you autogenerate this or can you leave out the redundant
> documentation?
> > Not sure why it's there in some other places in this file.
> 
> I deleted the redundant documentation from this and nearby functions.
> The autogeneration is still too problematic, I tried it before but spent
> more time cleaning it up and merging with previous manual fixes that it
> wasn't worth it.

I had the same experience and too be honest, it's not too big of a deal
to manually edit this. I was just asking since if you were using the
autogeneration
and it would be adding this redundant stuff, then it wouldn't be worth deleting
it.

>
https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
> File chrome/common/extensions/api/automation.idl (right):
> 
>
https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
> chrome/common/extensions/api/automation.idl:582: [nocompile] static void
> setTreeChangeObserverMask(
> On 2015/11/20 13:42:59, Peter Lundblad wrote:
> > I assume this mask is per extension.  IN any case, since you can add
> more
> > than one observer, I think it'd make sense to allow a separate mask
> per
> > observer. That would allow diferent parts of the user of this api todo
> > be less coupled.
> 
> That would be ideal, but it would significantly complicate the
> implementation so I'd prefer to leave it like this for now.
> 
> The issue is that the mask is implemented in C++, but the registration
> of observers happens in the JS bindings layer. The significant
> performance benefit of this change happens because we can avoid calls
> from C++ into JS for the vast majority of tree changes now.
> 
> I'm open to fixing this later if we actually need it. OK to leave it
> like this for now?

Wouldn't implementing this just be a matter of js setting the mask to the one
that provides most information in the set of listeners and update that every
time the list of listeners is changed, then duplicate the filtering in js
(or document that the mask is just a hint and no guarantee).
I think the API as it stands is very confusing and error prone.

The reason I brought this up was that I was thinking we might have use for this
api for braille. Currently, when something changes on scree that is shown
on the braille dispaly, the braille display won't be updated. I am talking
about general changes to, say, the name or alt text of something that is not
a live region or content of a text field that is handled separately.
This is not something I want to solve right now, just another use case that
could be considered when designing the api.  The way I would anticipate
using the tree change api to solve this issue would be to extend the mask
to a more generic filtering mechanism (say allowing limiting by ancestry).
Then one could temporarily add a listener with a filter to only get changes for
what's shown on the braille display without taking the performance hit of
getting all
tree updates.

I am happy to leave this for another day and rework the api later when needed,
just wanted to clarify my motivation a bit.

>
https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
> File chrome/renderer/extensions/automation_internal_custom_bindings.cc
> (right):
> 
>
https://codereview.chromium.org/1457683009/diff/1/chrome/renderer/extensions/...
> chrome/renderer/extensions/automation_internal_custom_bindings.cc:585:
> tree_change_observer_mask_ =
> On 2015/11/20 13:42:59, Peter Lundblad wrote:
> > What catches invalid values?
> 
> ParseTreeChangeObserverMask is autogenerated, it throws an exception if
> it's not passed a string that exactly matches one of the enum values, so
> additional checks aren't necessary.

Actually, it is even validated before reaching C++, I figured out, but the
effect is the same.

Thanks,
//Peter

-- 
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, 2015-11-24 15:58:05]

On Tue, Nov 24, 2015 at 5:03 AM <plundblad@chromium.org> wrote:

> dmazzoni@chromium.org writes:
> > > I assume this mask is per extension.  IN any case, since you can add
> > more
> > > than one observer, I think it'd make sense to allow a separate mask
> > per
> > > observer. That would allow diferent parts of the user of this api todo
> > > be less coupled.
> >
> > That would be ideal, but it would significantly complicate the
> > implementation so I'd prefer to leave it like this for now.
> >
> > The issue is that the mask is implemented in C++, but the registration
> > of observers happens in the JS bindings layer. The significant
> > performance benefit of this change happens because we can avoid calls
> > from C++ into JS for the vast majority of tree changes now.
> >
> > I'm open to fixing this later if we actually need it. OK to leave it
> > like this for now?
>
> Wouldn't implementing this just be a matter of js setting the mask to the
> one that provides most information in the set of listeners and update that
> every
> time the list of listeners is changed, then duplicate the filtering in js
> (or document that the mask is just a hint and no guarantee).
> I think the API as it stands is very confusing and error prone.


> The reason I brought this up was that I was thinking we might have use for
> this
> api for braille. Currently, when something changes on scree that is shown
> on the braille dispaly, the braille display won't be updated. I am talking
> about general changes to, say, the name or alt text of something that is
> not
> a live region or content of a text field that is handled separately.
> This is not something I want to solve right now, just another use case that
> could be considered when designing the api.  The way I would anticipate
> using the tree change api to solve this issue would be to extend the mask
> to a more generic filtering mechanism (say allowing limiting by ancestry).
> Then one could temporarily add a listener with a filter to only get
> changes for
> what's shown on the braille display without taking the performance hit of
> getting all
> tree updates.
>
> I am happy to leave this for another day and rework the api later when
> needed,
> just wanted to clarify my motivation a bit.
>

Thanks, your motivation makes a lot of sense.

For the specific braille case where a few nodes are on the screen, I wonder
if it might make sense to have a separate hook for
AXTree::OnAtomicUpdateFinished, or a generic event listener that listens to
all events - both of those would allow you to get one callback every time
anything changes, then you could poll the small number of nodes that are
currently visible on the braille display.

Making it possible to listen to changes only in one subtree would work too.

I'm hesitant about the idea of duplicating the filtering in JS. I don't
want to create an API that's easy to use but hides performance problems. I
also worry that even in the optimized case it'd be hard to do this without
introducing unnecessary overhead (checking the filter for every single node
even after already filtering in C++).

Right now what I'm leaning towards is that we should actually keep track of
the list of observers in C++ for maximum performance, but using observer
IDs to dispatch the notifications to the right callback rather than dealing
with owning V8 function objects in C++.

Maybe that wouldn't be so bad.

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

[David Tseng, 2015-11-24 18:46:30]

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:317:
chrome.automation.setTreeChangeObserverMask = function(mask) {};
Can we get a more specific type for mask?

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:545:
if (relevant.indexOf('additions') >= 0 &&
Where do relevant strings come from? This looks pretty error prone.

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:320: enum TreeChangeObserverMask {
This isn't really a mask since I don't think they're bit-wise orable?

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:582: [nocompile] static void
setTreeChangeObserverMask(
On 2015/11/23 20:16:50, dmazzoni wrote:
> On 2015/11/20 13:42:59, Peter Lundblad wrote:
> > I assume this mask is per extension.  IN any case, since you can add more
> > than one observer, I think it'd make sense to allow a separate mask per
> > observer. That would allow diferent parts of the user of this api todo
> > be less coupled.
> 
> That would be ideal, but it would significantly complicate the implementation
so
> I'd prefer to leave it like this for now.
> 
> The issue is that the mask is implemented in C++, but the registration of
> observers happens in the JS bindings layer. The significant performance
benefit
> of this change happens because we can avoid calls from C++ into JS for the
vast
> majority of tree changes now.
> 
> I'm open to fixing this later if we actually need it. OK to leave it like this
> for now?

I think, this looks a little like re-inventing the node-based eventing we have
(just articulated a little differently). Having listeners there that add
listeners for specific events, looks kind of like what we're adding onto here.

[dmazzoni, 2015-11-26 00:08:04]

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:582: [nocompile] static void
setTreeChangeObserverMask(
On 2015/11/24 18:46:30, David Tseng wrote:
> On 2015/11/23 20:16:50, dmazzoni wrote:
> > On 2015/11/20 13:42:59, Peter Lundblad wrote:
> > > I assume this mask is per extension.  IN any case, since you can add more
> > > than one observer, I think it'd make sense to allow a separate mask per
> > > observer. That would allow diferent parts of the user of this api todo
> > > be less coupled.
> > 
> > That would be ideal, but it would significantly complicate the
implementation
> so
> > I'd prefer to leave it like this for now.
> > 
> > The issue is that the mask is implemented in C++, but the registration of
> > observers happens in the JS bindings layer. The significant performance
> benefit
> > of this change happens because we can avoid calls from C++ into JS for the
> vast
> > majority of tree changes now.
> > 
> > I'm open to fixing this later if we actually need it. OK to leave it like
this
> > for now?
> 
> I think, this looks a little like re-inventing the node-based eventing we have
> (just articulated a little differently). Having listeners there that add
> listeners for specific events, looks kind of like what we're adding onto here.

The exception here is the onRemoved callback. Right now it gets called *before*
the node gets removed, but when the tree is in a possibly inconsistent state.

Getting rid of the tree change observers and replacing them with events would
mean:

* Rewriting the unserialization algorithm to compute the set of nodes that will
be removed before actually removing anything
* Potentially extra overhead due to event bubbling, etc.

I'm not convinced that events are better than tree change observers. Node that
the HTML DOM has both events and mutation observers, and the original mutation
events actually had to be deprecated because of similar performance issues.

I think what'd be ideal would be to make the tree change observers a lot like
mutation observers, where you could specify exactly what types of changes you're
interested in, and scope it to just part of the tree. The question is how much
we want to take on now, since we don't need all of that for live regions.

[dmazzoni, 2015-11-30 22:00:47]

Ready for another look. I changed it so that you can pass a filter for each
observer and it keeps track of the observers in C++.

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1457683009/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:320: enum TreeChangeObserverMask {
On 2015/11/24 18:46:30, David Tseng wrote:
> This isn't really a mask since I don't think they're bit-wise orable?

Changed to Filter.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs:125:
});
On 2015/11/24 11:04:30, Peter Lundblad wrote:
> Could test with one node being in the desktop tree as well.
> Also, is it too annoying to test with iframes which will mean (or already do?)
> multiple trees in a web page?

All good ideas. I think it was worth adding a test with an iframe, maybe we can
generalize it in the future.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:132:
*/
On 2015/11/23 23:23:57, David Tseng wrote:
> Move this as well?

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:133:
Background.LIVE_REGION_QUEUE_TIME_MS = 500;
On 2015/11/24 11:04:30, Peter Lundblad wrote:
> Move to LiveRegionHandler?

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js:64:
getCurrentRange: function() {
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> Perhaps make the get* methods private to enforce consistency.

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js:71:
setCurrentRange: function(newRange) {
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> How about making all these 'stub' methods goog.abstract so they throw?
> Then create a proper stub class if needed for testing.

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:66:
global.backgroundObj.setCurrentRange(cursors.Range.fromNode(node));
OK, I made ChromeVoxState a singleton for now because it wasn't obvious how to
pass the cvox2 background into the constructor from TabsApiHandler.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:56:
* @param {chrome.automation.TreeChange} treeChange
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> Suggest using goog.scope like in some other files to make type names less
> verbose.

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:72:
if (!AutomationUtil.isInSameWebpage(node, currentRange.start.node))
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> Doesn't classic announce live region updates from background tabs?
> I am not sure I personally think it is the right behaviour, but I think it has
> been requested in the past and changed according to such request.

Good catch.

I switched it but made it an option so it's easy to flip and maybe make it a
preference in the future.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:94:
* @param {?string} opt_prependFormatStr If set, a format string for
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> Optional parameters should have an = suffix.

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:110:
if (opt_prependFormatStr) {
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> nit: bracket inconsistency.

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:115:
if (!output.hasSpeech && node.liveAtomic) {
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> Ditto.

Done.

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions_test.extjs
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions_test.extjs:40:
* Create a function which perform the command |cmd|.
On 2015/11/24 11:04:31, Peter Lundblad wrote:
> nit: performs

Done.

[dmazzoni, 2015-11-30 22:02:48]

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1457683009/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:545:
if (relevant.indexOf('additions') >= 0 &&
On 2015/11/24 18:46:30, David Tseng wrote:
> Where do relevant strings come from? This looks pretty error prone.

This is from the ARIA spec, I'm not sure we should define them ourselves.

FWIW, we don't try to parse the string because this is the format some native
APIs want anyway (they just want the validated string value of the ARIA
attribute).

[dmazzoni, 2015-12-01 08:19:47]

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js
(right):

https://codereview.chromium.org/1457683009/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/chromevox_state.js:71:
setCurrentRange: function(newRange) {
On 2015/11/30 22:00:47, dmazzoni wrote:
> On 2015/11/24 11:04:31, Peter Lundblad wrote:
> > How about making all these 'stub' methods goog.abstract so they throw?
> > Then create a proper stub class if needed for testing.
> 
> Done.

Hmmm, I tried this but got weird errors because the function signatures didn't
match. I made these throw an error instead, hope that's okay for now. We can
address in a follow-up if you thing that goog.abstractMethod() is supposed to
work here.

[Peter Lundblad, 2015-12-01 12:49:58]

lgtm

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
(right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:496:
chrome.automation.AutomationNode.prototype.liveStatus;
Can you make sure these are included in AutomationNode in automation.idl for
documentation?

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs
(right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs:147:
var innerframe;
s/innerframe/innerFrame/

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
(right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:19:
* @param {ChromeVoxState} chromeVoxState The ChromeVox state object,
Make non-nullable.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:25:
* @type {ChromeVoxState}
Make non-nullable.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:32:
* @type {Date}
Non-nullable?

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:40:
* @type {WeakSet<AutomationNode>}
Ditto.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:59:
* @private
@type ...

https://codereview.chromium.org/1457683009/diff/80001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:779: for
(const auto& iter : tree_change_observers_) {
Mildly confusing to call something that is not an iterator 'iter'. observer?

[dmazzoni, 2015-12-01 19:19:01]

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
(right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:496:
chrome.automation.AutomationNode.prototype.liveStatus;
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> Can you make sure these are included in AutomationNode in automation.idl for
> documentation?

Done.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs
(right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util_test.extjs:147:
var innerframe;
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> s/innerframe/innerFrame/

Done.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
(right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:19:
* @param {ChromeVoxState} chromeVoxState The ChromeVox state object,
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> Make non-nullable.

Done.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:25:
* @type {ChromeVoxState}
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> Make non-nullable.

Done.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:32:
* @type {Date}
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> Non-nullable?

Done.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:40:
* @type {WeakSet<AutomationNode>}
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> Ditto.

Done.

https://codereview.chromium.org/1457683009/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js:59:
* @private
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> @type ...

Done.

https://codereview.chromium.org/1457683009/diff/80001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1457683009/diff/80001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:779: for
(const auto& iter : tree_change_observers_) {
On 2015/12/01 12:49:58, Peter Lundblad wrote:
> Mildly confusing to call something that is not an iterator 'iter'. observer?

Done.

[dmazzoni, 2015-12-01 19:21:08]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-01 19:21:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1457683009/#ps120001
(title: "Addressed last feedback")

[commit-bot: I haz the power, 2015-12-01 19:24:25]

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

[commit-bot: I haz the power, 2015-12-01 19:47:43]

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

[commit-bot: I haz the power, 2015-12-01 19:47:44]

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

[dmazzoni, 2015-12-01 19:58:57]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-12-01 20:00:48]

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

[commit-bot: I haz the power, 2015-12-01 21:17:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-01 21:18:03]

Description was changed from

==========
Complete live region support in ChromeVox Next.

Optimizes tree change notifications so that ChromeVox can only listen to
those relevant to live regions, and skip the rest.

Implements aria-atomic, and adds some code to avoid duplicate speaking of
changes to a live region due to multiple tree changes.

Improves speech queue mode support by always flushing following any key
event, queueing otherwise, and doing a category flush when a new live
region event happens 500 ms after the last one.

The last one is a heuristic, but the idea is that multiple live region
events at the same time should queue up, but a new live region some
discrete time later should interrupt previous live region events.

Adds some tests for live region output including flushing and queueing
behavior.

Manual tests include the live region test suite created for Google Docs
testing. There are probably some bugs and corner cases left, but this gets
us a lot closer.

BUG=478217
==========

to

==========
Complete live region support in ChromeVox Next.

Optimizes tree change notifications so that ChromeVox can only listen to
those relevant to live regions, and skip the rest.

Implements aria-atomic, and adds some code to avoid duplicate speaking of
changes to a live region due to multiple tree changes.

Improves speech queue mode support by always flushing following any key
event, queueing otherwise, and doing a category flush when a new live
region event happens 500 ms after the last one.

The last one is a heuristic, but the idea is that multiple live region
events at the same time should queue up, but a new live region some
discrete time later should interrupt previous live region events.

Adds some tests for live region output including flushing and queueing
behavior.

Manual tests include the live region test suite created for Google Docs
testing. There are probably some bugs and corner cases left, but this gets
us a lot closer.

BUG=478217

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

[commit-bot: I haz the power, 2015-12-01 21:18:04]

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

[David Tseng, 2015-12-07 16:39:59]

lgtm