Make ChromeVox cursor robust to deleted nodes

Make ChromeVox cursor robust to deleted nodes

If a node becomes invalid, the cursor snaps to the nearest valid ancestor.
Fixing this uncovered a lot of places where we were assuming that a cursor's
node must be valid, but it's possible that even with this new check that
a cursor could still be invalid, so I added checks in a lot of places.

BUG=613694
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/787fcfd375bb7e761a2ccfb869b1bb4877a2f337
Cr-Commit-Position: refs/heads/master@{#397860}

[dmazzoni, 2016-05-24 21:35:13]

Description was changed from

==========
Make ChromeVox cursor robust to deleted nodes

If a node becomes invalid, the cursor snaps to the nearest valid ancestor.
Fixing this uncovered a lot of places where we were assuming that a cursor's
node must be valid, but it's possible that even with this new check that
a cursor could still be invalid, so I added checks in a lot of places.

BUG=613694
==========

to

==========
Make ChromeVox cursor robust to deleted nodes

If a node becomes invalid, the cursor snaps to the nearest valid ancestor.
Fixing this uncovered a lot of places where we were assuming that a cursor's
node must be valid, but it's possible that even with this new check that
a cursor could still be invalid, so I added checks in a lot of places.

BUG=613694
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-05-24 21:35:36]

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

[David Tseng, 2016-05-25 20:58:10]

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

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:110: *
@return {?AutomationNode}
You don't need the '?' (objects are nullable by default).

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:113: for
(var index = 0; index < this.ancestry_.length; index++) {
The dual use of index here is confusing. Maybe use for (let x of y).

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:114: var
foo = this.ancestry_[index];
How about firstValidNode?

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:115: if
(foo != null && foo.role !== undefined)
This won't work. There are nodes that have a role but have no root; check for
the presence of root as well.

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:116:
return foo;
Do you need to do bookkeeping on the ancestry chain?

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:118:
this.index_ = 0;
Reset to cursors.NODE_INDEX

[dmazzoni, 2016-06-01 16:36:50]

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

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:110: *
@return {?AutomationNode}
On 2016/05/25 20:58:10, David Tseng wrote:
> You don't need the '?' (objects are nullable by default).

Done.

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:113: for
(var index = 0; index < this.ancestry_.length; index++) {
On 2016/05/25 20:58:10, David Tseng wrote:
> The dual use of index here is confusing. Maybe use for (let x of y).

Fixed, but the Closure compiler doesn't let us use ES6
syntax as currently configured so I kept it a simple
loop for now. Should we enable ES6 in a follow-on change?

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:114: var
foo = this.ancestry_[index];
On 2016/05/25 20:58:10, David Tseng wrote:
> How about firstValidNode?

Acknowledged.

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:115: if
(foo != null && foo.role !== undefined)
On 2016/05/25 20:58:10, David Tseng wrote:
> This won't work. There are nodes that have a role but have no root; check for
> the presence of root as well.

Done.

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:116:
return foo;
On 2016/05/25 20:58:10, David Tseng wrote:
> Do you need to do bookkeeping on the ancestry chain?

I don't think so, because this object is basically immutable - if you call it
again it will search again, but as soon as you move to another node, it creates
a new Cursor with a new ancestry.

https://codereview.chromium.org/2007183002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:118:
this.index_ = 0;
On 2016/05/25 20:58:10, David Tseng wrote:
> Reset to cursors.NODE_INDEX

Thanks, that made it work better.

[David Tseng, 2016-06-01 20:20:25]

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(right):

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:815:
var doCmd = this.doCmd.bind(this);
You don't need to bind; also, let's export this on window.

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

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:82: if
(nodeWalker == node.root)
I think this should use our root predicate (currently
AutomationUtil.isTraversalRoot). We would fail to recover if a frame
disappeared.

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:119: //
If we have to walk up to an ancestor, reset the index to zero.
Update this as well.

[dmazzoni, 2016-06-01 22:46:56]

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(right):

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:815:
var doCmd = this.doCmd.bind(this);
On 2016/06/01 20:20:25, David Tseng wrote:
> You don't need to bind; also, let's export this on window.

Done.

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

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:82: if
(nodeWalker == node.root)
On 2016/06/01 20:20:25, David Tseng wrote:
> I think this should use our root predicate (currently
> AutomationUtil.isTraversalRoot). We would fail to recover if a frame
> disappeared.

Done.

https://codereview.chromium.org/2007183002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:119: //
If we have to walk up to an ancestor, reset the index to zero.
On 2016/06/01 20:20:25, David Tseng wrote:
> Update this as well.

Done.

[David Tseng, 2016-06-01 23:40:36]

lgtm

[dmazzoni, 2016-06-02 16:37:58]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-02 16:37:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2007183002/#ps60001
(title: "null check")

[commit-bot: I haz the power, 2016-06-02 16:38:23]

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

[commit-bot: I haz the power, 2016-06-02 17:21:38]

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

[commit-bot: I haz the power, 2016-06-02 17:21:40]

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

[dmazzoni, 2016-06-03 21:10:12]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-03 21:10:12]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2007183002/#ps80001
(title: "Add one more null check")

[commit-bot: I haz the power, 2016-06-03 21:10:52]

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

[commit-bot: I haz the power, 2016-06-03 21:13:56]

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

[commit-bot: I haz the power, 2016-06-03 21:13:57]

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

[dmazzoni, 2016-06-03 23:14:47]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-03 23:14:48]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2007183002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-03 23:15:42]

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

[commit-bot: I haz the power, 2016-06-04 00:43:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-04 00:46:00]

Description was changed from

==========
Make ChromeVox cursor robust to deleted nodes

If a node becomes invalid, the cursor snaps to the nearest valid ancestor.
Fixing this uncovered a lot of places where we were assuming that a cursor's
node must be valid, but it's possible that even with this new check that
a cursor could still be invalid, so I added checks in a lot of places.

BUG=613694
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Make ChromeVox cursor robust to deleted nodes

If a node becomes invalid, the cursor snaps to the nearest valid ancestor.
Fixing this uncovered a lot of places where we were assuming that a cursor's
node must be valid, but it's possible that even with this new check that
a cursor could still be invalid, so I added checks in a lot of places.

BUG=613694
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/787fcfd375bb7e761a2ccfb869b1bb4877a2f337
Cr-Commit-Position: refs/heads/master@{#397860}
==========

[commit-bot: I haz the power, 2016-06-04 00:46:01]

Patchset 6 (id:??) landed as
https://crrev.com/787fcfd375bb7e761a2ccfb869b1bb4877a2f337
Cr-Commit-Position: refs/heads/master@{#397860}