Add a treeChange type to Automation

Add a treeChange type to Automation

ChromeVox now processes text changed tree events in place of node changed events (which were too noisy and didn't reflect actual live region changes).

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live regions such as "you are online" are announced.

Committed: https://crrev.com/f5424b37108e4d0fee0f23f089203872369f9982
Cr-Commit-Position: refs/heads/master@{#376913}

[David Tseng, 2016-02-19 01:24:35]

Description was changed from

==========
Add a tree_changed type to AXTreeDelegate.
==========

to

==========
Add a tree_changed type to AXTreeDelegate.

1 adds two sets that keep track of which nodes have been created and which nodes
have had text changed
2 we need the node created set because the new_nodes set isn't always correct.
Sometimes, it reports nodes as being "new" but they already exist in the tree
before an update
3 ChromeVox processes text changed tree events in place of node changed events
(which were too noisy and didn't reflect actual live region changes).
4 adjust the ergion output rule to just compute descendants. We should iterate
on this one.

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
regions such as "you are online" are announced.
==========

[David Tseng, 2016-02-19 01:26:43]

FYI; tests are pending a more general approach review.

On Thu, Feb 18, 2016 at 5:25 PM, <dtseng@chromium.org> wrote:

> Reviewers:
> CL: https://codereview.chromium.org/1716663002/
>
> Description:
> Add a tree_changed type to AXTreeDelegate.
>
> 1 adds two sets that keep track of which nodes have been created and which
nodes
> have had text changed
> 2 we need the node created set because the new_nodes set isn't always
correct.
> Sometimes, it reports nodes as being "new" but they already exist in the tree
> before an update
> 3 ChromeVox processes text changed tree events in place of node changed
events
> (which were too noisy and didn't reflect actual live region changes).
> 4 adjust the ergion output rule to just compute descendants. We should
iterate
> on this one.
>
> TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
> regions such as "you are online" are announced.
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+46, -15 lines):
>   M
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
>   M chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
>   M chrome/common/extensions/api/automation.idl
>   M chrome/renderer/extensions/automation_internal_custom_bindings.cc
>   M ui/accessibility/ax_tree.h
>   M ui/accessibility/ax_tree.cc
>
>
> Index:
chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
> diff --git
a/chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
b/chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
> index
0269a5da1fa647a452088549c6bf0471511defcd..8cce149ab2c834b0b67ac9eaab3b22646d229dd6
100644
> ---
a/chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
> +++
b/chrome/browser/resources/chromeos/chromevox/cvox2/background/live_regions.js
> @@ -96,7 +96,7 @@ LiveRegions.prototype = {
>        this.outputLiveRegionChange_(node, null);
>      }
>
> -    if (relevant.indexOf('text') >= 0 && type == 'nodeChanged')
> +    if (relevant.indexOf('text') >= 0 && type == 'textChanged')
>        this.outputLiveRegionChange_(node, null);
>
>      if (relevant.indexOf('removals') >= 0 && type == 'nodeRemoved')
> Index: chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> diff --git
a/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
b/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> index
5f89c3142fdb494ca30f157b3e85b19fc7e487e2..f3bfa177ba5c29270869955104f24d7bb7d710cb
100644
> --- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> +++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> @@ -261,10 +261,6 @@ Output.ROLE_INFO_ = {
>    radioGroup: {
>      msgId: 'role_radiogroup',
>    },
> -  region: {
> -    msgId: 'role_region',
> -    inherits: 'abstractContainer'
> -  },
>    rowHeader: {
>      msgId: 'role_rowheader',
>      inherits: 'abstractContainer'
> @@ -477,6 +473,9 @@ Output.RULES = {
>        enter: '$name',
>        speak: '$if($name, $name, $docUrl)'
>      },
> +    region: {
> +      speak: '$descendants'
> +    },
>      row: {
>        enter: '@row_granularity $tableRowIndex'
>      },
> Index: chrome/common/extensions/api/automation.idl
> diff --git a/chrome/common/extensions/api/automation.idl
b/chrome/common/extensions/api/automation.idl
> index
96b1d37550897f53848bce9d324857359b9cb75f..50f58b72f2dfa2a3e1d9a0aa6a8204c0fd54aa79
100644
> --- a/chrome/common/extensions/api/automation.idl
> +++ b/chrome/common/extensions/api/automation.idl
> @@ -239,6 +239,11 @@
>       */
>      nodeChanged,
>
> +        /**
> +     * This node's text (name) changed.
> +     */
> +    textChanged,
> +
>      /**
>       * This node was removed.
>       */
> Index: chrome/renderer/extensions/automation_internal_custom_bindings.cc
> diff --git a/chrome/renderer/extensions/automation_internal_custom_bindings.cc
b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
> index
3a05d188a739a69f1cb5d5ffedf60d255fdde848..605a857e97966b1eaae87eb944e1bddb10bd4c29
100644
> --- a/chrome/renderer/extensions/automation_internal_custom_bindings.cc
> +++ b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
> @@ -956,6 +956,10 @@ void
AutomationInternalCustomBindings::OnAtomicUpdateFinished(
>              api::automation::TREE_CHANGE_TYPE_NODECHANGED,
>              tree, node);
>          break;
> +      case TEXT_CHANGED:
> +        SendTreeChangeEvent(api::automation::TREE_CHANGE_TYPE_TEXTCHANGED,
tree,
> +                            node);
> +        break;
>      }
>    }
>  }
> Index: ui/accessibility/ax_tree.cc
> diff --git a/ui/accessibility/ax_tree.cc b/ui/accessibility/ax_tree.cc
> index
bd879eaac441754030ef96bae00cdbec65370f2d..3958a4db241d5dc5d4a784679c412f4c1c275774
100644
> --- a/ui/accessibility/ax_tree.cc
> +++ b/ui/accessibility/ax_tree.cc
> @@ -113,10 +113,27 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
>        update_state.pending_nodes.insert(node);
>      }
>    }
> -
> +  std::set<int> node_created_ids;
> +  std::set<int> text_changed_ids;
>    for (size_t i = 0; i < update.nodes.size(); ++i) {
> +    AXNode* original_node = GetFromId(update.nodes[i].id);
> +    std::string original_text;
> +    if (original_node)
> +      original_text =
original_node->data().GetStringAttribute(AX_ATTR_NAME);
> +
>      if (!UpdateNode(update.nodes[i], &update_state))
>        return false;
> +
> +    AXNode* updated_node = GetFromId(update.nodes[i].id);
> +    std::string updated_text;
> +    if (updated_node)
> +      updated_text = updated_node->data().GetStringAttribute(AX_ATTR_NAME);
> +    if (!original_node && updated_node)
> +      node_created_ids.insert(update.nodes[i].id);
> +    if (!original_text.empty() && !updated_text.empty() &&
> +        updated_text != original_text) {
> +      text_changed_ids.insert(update.nodes[i].id);
> +    }
>    }
>
>    if (!update_state.pending_nodes.empty()) {
> @@ -139,12 +156,22 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
>            changes.push_back(
>                AXTreeDelegate::Change(node,
AXTreeDelegate::SUBTREE_CREATED));
>          } else {
> -          changes.push_back(
> -              AXTreeDelegate::Change(node, AXTreeDelegate::NODE_CREATED));
> +          // Node creation based on |new_nodes| is sometimes unreliable.
Define
> +          // creation based on this tree.
> +          if (node_created_ids.find(node->data().id) !=
> +              node_created_ids.end()) {
> +            changes.push_back(
> +                AXTreeDelegate::Change(node, AXTreeDelegate::NODE_CREATED));
> +          }
>          }
>        } else {
> -        changes.push_back(
> -            AXTreeDelegate::Change(node, AXTreeDelegate::NODE_CHANGED));
> +        if (text_changed_ids.find(node->data().id) == text_changed_ids.end())
{
> +          changes.push_back(
> +              AXTreeDelegate::Change(node, AXTreeDelegate::NODE_CHANGED));
> +        } else {
> +          changes.push_back(
> +              AXTreeDelegate::Change(node, AXTreeDelegate::TEXT_CHANGED));
> +        }
>        }
>      }
>      delegate_->OnAtomicUpdateFinished(
> Index: ui/accessibility/ax_tree.h
> diff --git a/ui/accessibility/ax_tree.h b/ui/accessibility/ax_tree.h
> index
62b0709fc35aaa9f4d25f0a14c5c71699727e355..2f21bd36906e8561ce62f4a8c3b3445ad739c543
100644
> --- a/ui/accessibility/ax_tree.h
> +++ b/ui/accessibility/ax_tree.h
> @@ -62,11 +62,7 @@ class AX_EXPORT AXTreeDelegate {
>    // the middle of an update, don't walk the parents and children now.
>    virtual void OnNodeChanged(AXTree* tree, AXNode* node) = 0;
>
> -  enum ChangeType {
> -    NODE_CREATED,
> -    SUBTREE_CREATED,
> -    NODE_CHANGED
> -  };
> +  enum ChangeType { NODE_CREATED, SUBTREE_CREATED, NODE_CHANGED, TEXT_CHANGED
};
>
>    struct Change {
>      Change(AXNode* node, ChangeType type) {
>
>
>
> --
> 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.
>

-- 
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-02-19 04:17:31]

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

[dmazzoni, 2016-02-19 04:17:32]

Love the idea, but I'd prefer two things:
1. I think we could do it without needing to keep track of additional sets
2. I'd prefer if AXTree didn't care about AX_ATTR_NAME, we should be able to
solve this generally without special-casing NAME in AXTree, which mostly doesn't
care about the actual data of nodes.

A very specific idea is below. This is something I've wanted for a while anyway,
it will allow us to clean up some Windows code, too!

https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc
File ui/accessibility/ax_tree.cc (right):

https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc...
ui/accessibility/ax_tree.cc:210: node->SetData(src);
I think this would be the place to hook it. Right before this line of code,
|node->data()| is the old data, and |src| is the new data. I suggest just
directly calling a new delegate function OnNodeWillChange that can take both
|node| and |src| as arguments.

Then the logic of deciding whether to do something with text changed, or role
changed, or whatever is up to the delegate, rather than making AX_ATTR_NAME
somehow magical.

This also avoids needing to keep track of sets here.

In automation_custom_bindings, we shouldn't call into JS when we get
OnNodeWillChange because the tree is in the middle of an update. We should see
if the name changed, and if so, add it to a list of nodes - then actually do the
javascript callbacks the same way we handle SendNodesRemovedEvent.

[David Tseng, 2016-02-19 23:07:42]

On Fri, Feb 19, 2016 at 1:22 PM, <dmazzoni@chromium.org> wrote:
I think this assumes that childTreeIds are only populated by calls to
enableFrame, but in the case of cross-process iframes, it's implicit.

Shortly I'm planning to land a change that makes all iframes into separate
AXTrees, so then pretty much any web page with iframes would have childTreeIds
in them.


Is this an issue in the current implementation?



The other issue I'm worried about is that sometimes the childTreeId changes,
like when the page navigates. It's not obvious to me how it works in this case.

Does the hosting webView get destroyed as well? Do you have a snippet that does
this?

https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/automation_custom_bindings.js
(right):

https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/automation_custom_bindings.js:214:
privates(node).impl.ChildTree = root;
Should this be childTree instead of ChildTree?

Oops; had it as setChildTree earlier. Done.


https://codereview.chromium.org/1719473002/
On 2016/02/19 04:17:32, dmazzoni wrote:
> Love the idea, but I'd prefer two things:
> 1. I think we could do it without needing to keep track of additional sets
> 2. I'd prefer if AXTree didn't care about AX_ATTR_NAME, we should be able to
> solve this generally without special-casing NAME in AXTree, which mostly
doesn't
> care about the actual data of nodes.
>
> A very specific idea is below. This is something I've wanted for a while
anyway,
> it will allow us to clean up some Windows code, too!
>
> https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc
> File ui/accessibility/ax_tree.cc (right):
>
>
https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc...
> ui/accessibility/ax_tree.cc:210: node->SetData(src);
> I think this would be the place to hook it. Right before this line of code,
> |node->data()| is the old data, and |src| is the new data. I suggest just
> directly calling a new delegate function OnNodeWillChange that can take both
> |node| and |src| as arguments.
>
> Then the logic of deciding whether to do something with text changed, or role
> changed, or whatever is up to the delegate, rather than making AX_ATTR_NAME
> somehow magical.
>
> This also avoids needing to keep track of sets here.

There's a problem with node created; it doesn't match up with the check I made
in this patchset. If you have any ideas as to why this happens, then we should
fix it. Otherwise, I'm inclined to introduce that part of the change in a
followup cl for m50. It's causing the Docs use case to be very unusable.
>
> In automation_custom_bindings, we shouldn't call into JS when we get
> OnNodeWillChange because the tree is in the middle of an update. We should see
> if the name changed, and if so, add it to a list of nodes - then actually do
the
> javascript callbacks the same way we handle SendNodesRemovedEvent.

I went ahead and did this in the new patchset. PTAL. Went with a tree change
listener solution.

[David Tseng, 2016-02-19 23:12:07]

Sorry; copy/pasted other messages:



https://codereview.chromium.org/1719473002/
On 2016/02/19 04:17:32, dmazzoni wrote:
> Love the idea, but I'd prefer two things:
> 1. I think we could do it without needing to keep track of additional sets
> 2. I'd prefer if AXTree didn't care about AX_ATTR_NAME, we should be able
to
> solve this generally without special-casing NAME in AXTree, which mostly
doesn't
> care about the actual data of nodes.
>
> A very specific idea is below. This is something I've wanted for a while
anyway,
> it will allow us to clean up some Windows code, too!
>
>
https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc
> File ui/accessibility/ax_tree.cc (right):
>
>
https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc...
> ui/accessibility/ax_tree.cc:210: node->SetData(src);
> I think this would be the place to hook it. Right before this line of
code,
> |node->data()| is the old data, and |src| is the new data. I suggest just
> directly calling a new delegate function OnNodeWillChange that can take
both
> |node| and |src| as arguments.
>
> Then the logic of deciding whether to do something with text changed, or
role
> changed, or whatever is up to the delegate, rather than making
AX_ATTR_NAME
> somehow magical.
>
> This also avoids needing to keep track of sets here.

There's a problem with node created; it doesn't match up with the check I
made in this patchset. If you have any ideas as to why this happens, then
we should fix it. Otherwise, I'm inclined to introduce that part of the
change in a followup cl for m50. It's causing the Docs use case to be very
unusable.
>
> In automation_custom_bindings, we shouldn't call into JS when we get
> OnNodeWillChange because the tree is in the middle of an update. We
should see
> if the name changed, and if so, add it to a list of nodes - then actually
do the
> javascript callbacks the same way we handle SendNodesRemovedEvent.

I went ahead and did this in the new patchset. PTAL. Went with a tree
change listener solution.

On Fri, Feb 19, 2016 at 3:07 PM, <dtseng@chromium.org> wrote:

>
>
> On Fri, Feb 19, 2016 at 1:22 PM, <dmazzoni@chromium.org> wrote:
> I think this assumes that childTreeIds are only populated by calls to
> enableFrame, but in the case of cross-process iframes, it's implicit.
>
> Shortly I'm planning to land a change that makes all iframes into separate
> AXTrees, so then pretty much any web page with iframes would have
childTreeIds
> in them.
>
>
> Is this an issue in the current implementation?
>
>
>
> The other issue I'm worried about is that sometimes the childTreeId changes,
> like when the page navigates. It's not obvious to me how it works in this
case.
>
> Does the hosting webView get destroyed as well? Do you have a snippet that
does
> this?
>
https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
> File chrome/renderer/resources/extensions/automation_custom_bindings.js
> (right):
>
https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
> chrome/renderer/resources/extensions/automation_custom_bindings.js:214
<https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...>:
> privates(node).impl.ChildTree = root;
> Should this be childTree instead of ChildTree?
>
> Oops; had it as setChildTree earlier. Done.
>
> https://codereview.chromium.org/1719473002/
> On 2016/02/19 04:17:32, dmazzoni wrote:
> > Love the idea, but I'd prefer two things:
> > 1. I think we could do it without needing to keep track of additional sets
> > 2. I'd prefer if AXTree didn't care about AX_ATTR_NAME, we should be able
to
> > solve this generally without special-casing NAME in AXTree, which mostly
> doesn't
> > care about the actual data of nodes.
> >
> > A very specific idea is below. This is something I've wanted for a while
> anyway,
> > it will allow us to clean up some Windows code, too!
> >
> >
https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc
> > File ui/accessibility/ax_tree.cc (right):
> >
>
>https://codereview.chromium.org/1716663002/diff/1/ui/accessibility/ax_tree.cc#newcode210
> > ui/accessibility/ax_tree.cc:210: node->SetData(src);
> > I think this would be the place to hook it. Right before this line of code,
> > |node->data()| is the old data, and |src| is the new data. I suggest just
> > directly calling a new delegate function OnNodeWillChange that can take
both
> > |node| and |src| as arguments.
> >
> > Then the logic of deciding whether to do something with text changed, or
role
> > changed, or whatever is up to the delegate, rather than making AX_ATTR_NAME
> > somehow magical.
> >
> > This also avoids needing to keep track of sets here.
>
> There's a problem with node created; it doesn't match up with the check I
made
> in this patchset. If you have any ideas as to why this happens, then we
should
> fix it. Otherwise, I'm inclined to introduce that part of the change in a
> followup cl for m50. It's causing the Docs use case to be very unusable.
> >
> > In automation_custom_bindings, we shouldn't call into JS when we get
> > OnNodeWillChange because the tree is in the middle of an update. We should
see
> > if the name changed, and if so, add it to a list of nodes - then actually
do
> the
> > javascript callbacks the same way we handle SendNodesRemovedEvent.
>
> I went ahead and did this in the new patchset. PTAL. Went with a tree change
> listener solution.
> https://codereview.chromium.org/1716663002/
>
> --
> 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.
>

-- 
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-02-19 23:20:24]

lgtm with a few suggestions

How about modifying one of the existing live region tests so that it modifies a
node in a live region that contains text, but in a way that doesn't change the
node's text? Basically the test result should be the same but it asserts that
non-text changes to a live region don't result in any speaking

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

https://codereview.chromium.org/1716663002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:242: /**
nit: indentation

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

https://codereview.chromium.org/1716663002/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1064:
text_changed_node_ids_.clear();
This clear should go after the for loop, not inside it.

https://codereview.chromium.org/1716663002/diff/20001/ui/accessibility/ax_tree.h
File ui/accessibility/ax_tree.h (right):

https://codereview.chromium.org/1716663002/diff/20001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree.h:45: virtual void OnNodeDataWillChange(const
AXNodeData& old_node_data,
For completeness I'd pass the AXTree* and AXNode* here to match the other
interfaces

[David Tseng, 2016-02-22 17:47:52]

Description was changed from

==========
Add a tree_changed type to AXTreeDelegate.

1 adds two sets that keep track of which nodes have been created and which nodes
have had text changed
2 we need the node created set because the new_nodes set isn't always correct.
Sometimes, it reports nodes as being "new" but they already exist in the tree
before an update
3 ChromeVox processes text changed tree events in place of node changed events
(which were too noisy and didn't reflect actual live region changes).
4 adjust the ergion output rule to just compute descendants. We should iterate
on this one.

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
regions such as "you are online" are announced.
==========

to

==========
Add a treeChange type to Automation

ChromeVox now processes text changed tree events in place of node changed events
(which were too noisy and didn't reflect actual live region changes).

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
regions such as "you are online" are announced.
==========

[David Tseng, 2016-02-22 17:47:52]

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

[David Tseng, 2016-02-22 17:48:50]

New test added that hoepfully demonstrates the lack of output when no text
changes, but a node change occurs.

+ rdevlin for c/c/extensions

[Devlin, 2016-02-22 18:21:45]

extensions lgtm with nits

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

https://codereview.chromium.org/1716663002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:242: /**
nit: indentation

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

https://codereview.chromium.org/1716663002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1041: for
(auto change : changes) {
drive-by: can this be const auto&?

https://codereview.chromium.org/1716663002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1062: for
(size_t i = 0; i < text_changed_node_ids_.size(); ++i) {
nit: prefer range-based (C++11-style) for-loops.

[David Tseng, 2016-02-22 22:55:49]



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

https://codereview.chromium.org/1716663002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:242: /**
On 2016/02/22 18:21:45, Devlin wrote:
> nit: indentation

Done.

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

https://codereview.chromium.org/1716663002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1041: for
(auto change : changes) {
On 2016/02/22 18:21:45, Devlin wrote:
> drive-by: can this be const auto&?

Done.

https://codereview.chromium.org/1716663002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1062: for
(size_t i = 0; i < text_changed_node_ids_.size(); ++i) {
On 2016/02/22 18:21:45, Devlin wrote:
> nit: prefer range-based (C++11-style) for-loops.

Done.

[David Tseng, 2016-02-22 22:55:50]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-02-22 22:55:50]

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

[commit-bot: I haz the power, 2016-02-22 22:58:39]

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

[commit-bot: I haz the power, 2016-02-22 23:10:51]

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

[commit-bot: I haz the power, 2016-02-22 23:10:52]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[David Tseng, 2016-02-23 00:02:38]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-02-23 00:02:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1716663002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-02-23 00:04:25]

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

[commit-bot: I haz the power, 2016-02-23 00:52:00]

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

[commit-bot: I haz the power, 2016-02-23 00:52:01]

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

[David Tseng, 2016-02-23 02:12:55]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-02-23 02:12:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1716663002/#ps100001
(title: "git cl set_commit")

[commit-bot: I haz the power, 2016-02-23 02:13:47]

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

[David Tseng, 2016-02-23 02:46:13]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-02-23 03:22:02]

Description was changed from

==========
Add a treeChange type to Automation

ChromeVox now processes text changed tree events in place of node changed events
(which were too noisy and didn't reflect actual live region changes).

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
regions such as "you are online" are announced.
==========

to

==========
Add a treeChange type to Automation

ChromeVox now processes text changed tree events in place of node changed events
(which were too noisy and didn't reflect actual live region changes).

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
regions such as "you are online" are announced.
==========

[commit-bot: I haz the power, 2016-02-23 03:22:03]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-02-23 03:23:03]

Description was changed from

==========
Add a treeChange type to Automation

ChromeVox now processes text changed tree events in place of node changed events
(which were too noisy and didn't reflect actual live region changes).

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
regions such as "you are online" are announced.
==========

to

==========
Add a treeChange type to Automation

ChromeVox now processes text changed tree events in place of node changed events
(which were too noisy and didn't reflect actual live region changes).

TEST=in ChromeVox Next with braille mode off, start typing. Ensure no live
regions such as "you are online" are announced.

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

[commit-bot: I haz the power, 2016-02-23 03:23:05]

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