Added auto-scan, made some small changes to how prefs are stored, refactored.

Added auto-scan, made some small changes to how prefs are stored, refactored.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2777203006
Cr-Commit-Position: refs/heads/master@{#461047}
Committed: https://chromium.googlesource.com/chromium/src/+/ac413e5b5d9da338a6b151a583a7fed1482899c8

[elichtenberg, 2017-03-28 23:28:23]

Description was changed from

==========
Added auto-scan, made some small changes to how prefs are stored, refactored.

BUG=593885
==========

to

==========
Added auto-scan, made some small changes to how prefs are stored, refactored.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[elichtenberg, 2017-03-28 23:29:36]

elichtenberg@google.com changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[elichtenberg, 2017-03-28 23:30:35]

The CQ bit was checked by elichtenberg@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 23:31:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 01:14:12]

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

[commit-bot: I haz the power, 2017-03-29 01:14:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[elichtenberg, 2017-03-29 04:31:22]

The CQ bit was checked by elichtenberg@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 04:31:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 05:31:26]

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

[commit-bot: I haz the power, 2017-03-29 05:31:27]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-03-29 05:52:20]

lgtm

All of the refactoring looks great!

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js
(right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:96: *
@param {number} scanTime
Document "in seconds" here

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/closure_support.js (right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/closure_support.js:8: let
AutomationNode = chrome.automation.AutomationNode;
This doesn't look like a shortcut for the Closure Compiler anymore.
I think it'd be better to delete this file and put this in any scoped block
where it'd be useful for readability.

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/manifest.json.jinja2
(right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/manifest.json.jinja2:17:
"tree_walker.js",
maybe sort these?

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/prefs.js (right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/prefs.js:65: let event = new
CustomEvent('prefsUpdate', {'detail': updatedPrefs});
Now that you have a nice SwitchAccessInterface class, it might be more
consistent to call this.switchAccess_.updatedPrefs() to be consistent
with the rest of the code rather than firing an event here

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/switch_access.js (right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/switch_access.js:9: *
@implements {SwitchAccessInterface}
This worked out well!

[elichtenberg, 2017-03-29 19:12:46]

The CQ bit was checked by elichtenberg@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 19:13:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[elichtenberg, 2017-03-29 19:15:11]

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js
(right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:96: *
@param {number} scanTime
On 2017/03/29 05:52:20, dmazzoni wrote:
> Document "in seconds" here

Done.

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/closure_support.js (right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/closure_support.js:8: let
AutomationNode = chrome.automation.AutomationNode;
On 2017/03/29 05:52:20, dmazzoni wrote:
> This doesn't look like a shortcut for the Closure Compiler anymore.
> I think it'd be better to delete this file and put this in any scoped block
> where it'd be useful for readability.

Done. Creating scopes caused some issues with closure, so just used full type
name in annotations.

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/manifest.json.jinja2
(right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/manifest.json.jinja2:17:
"tree_walker.js",
On 2017/03/29 05:52:20, dmazzoni wrote:
> maybe sort these?

The scripts get run in the order listed, so can't be put in alphabetical order.

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/prefs.js (right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/prefs.js:65: let event = new
CustomEvent('prefsUpdate', {'detail': updatedPrefs});
On 2017/03/29 05:52:20, dmazzoni wrote:
> Now that you have a nice SwitchAccessInterface class, it might be more
> consistent to call this.switchAccess_.updatedPrefs() to be consistent
> with the rest of the code rather than firing an event here

Currently, the options page(s) also listen(s) to 'prefsUpdate' events, which
occur on the background document, so in order to not use events here, our
switchAccess object would need to have access to all of the options pages. We
could have each options page register itself on the background document when it
is created, but not sure I like that solution, especially since the options page
would have to be unregistered once it is closed. So not changing for now.

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/switch_access.js (right):

https://codereview.chromium.org/2777203006/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/switch_access.js:9: *
@implements {SwitchAccessInterface}
On 2017/03/29 05:52:20, dmazzoni wrote:
> This worked out well!

Thanks!

[David Tseng, 2017-03-29 21:57:21]

lgtm

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js
(right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:32:
.getPref('autoScanTime'));
Optional: you could get more type safety if you added methods like
getNumberPref, etc. This could avoid the cast and throw if the pref isn't found.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:47:
this.intervalID_ = 0;
Would be clearer to make this undefined. setInterval doesn't appear to return id
0, but I don't see why it couldn't.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:59:
this.scanTime_ * 1000);
Any reason to not just store the scan time in ms?

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js
(left):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js:17:
getNextNode: function(node) {
I don't see that much value in having this in its own file since it has one
consumer. Up to you though. I would put it in the unit test file. Also, since
tests don't get compiled, you could access private members of the object.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js
(right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js:7: * for
testing.
@extends {AutomationTreeWalker}
(I know this doesn't get compiled).

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/tree_walker.js (right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:23: let next =
this.getNextNode_(node);
You could just have
do {
    node = this...
  while (...);

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:67: }
You can combine this function with the one above and take a direction param.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:97: return aunt;
Comment: very gender specific :).

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:118: let
descendant = this.getYoungestDescendant_(sibling);
return this.getYoungestDescendant_(sibling) || sibling;

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:125: let parent =
node.parent;
return node.parent; (no need to do any of the below).

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/switch_access/tree_walker_unittest.gtestjs
(right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker_unittest.gtestjs:10:
function TreeWalkerUnitTest () {
AutomationTreeWalkerUnitTest

[commit-bot: I haz the power, 2017-03-30 00:36:43]

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

[commit-bot: I haz the power, 2017-03-30 00:36:44]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[elichtenberg, 2017-03-30 21:46:57]

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js
(right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:32:
.getPref('autoScanTime'));
On 2017/03/29 21:57:20, David Tseng wrote:
> Optional: you could get more type safety if you added methods like
> getNumberPref, etc. This could avoid the cast and throw if the pref isn't
found.

Done.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:47:
this.intervalID_ = 0;
On 2017/03/29 21:57:20, David Tseng wrote:
> Would be clearer to make this undefined. setInterval doesn't appear to return
id
> 0, but I don't see why it couldn't.

Done. I think the documentation said that the interval ID would always be
non-zero, but I agree that undefined is probably clearer anyways.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/auto_scan_manager.js:59:
this.scanTime_ * 1000);
On 2017/03/29 21:57:20, David Tseng wrote:
> Any reason to not just store the scan time in ms?

Done. Didn't have a reason besides that the options page displays the time in
seconds.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js
(left):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js:17:
getNextNode: function(node) {
On 2017/03/29 21:57:21, David Tseng wrote:
> I don't see that much value in having this in its own file since it has one
> consumer. Up to you though. I would put it in the unit test file. Also, since
> tests don't get compiled, you could access private members of the object.

I agree. I'm just going to access the private members of AutomationTreeWalker,
and so I'm deleting this file.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js
(right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/testable_tree_walker.js:7: * for
testing.
On 2017/03/29 21:57:20, David Tseng wrote:
> @extends {AutomationTreeWalker}
> (I know this doesn't get compiled).

Done.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/tree_walker.js (right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:23: let next =
this.getNextNode_(node);
On 2017/03/29 21:57:21, David Tseng wrote:
> You could just have
> do {
>     node = this...
>   while (...);

Done.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:67: }
On 2017/03/29 21:57:21, David Tseng wrote:
> You can combine this function with the one above and take a direction param.

Done.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:97: return aunt;
On 2017/03/29 21:57:21, David Tseng wrote:
> Comment: very gender specific :).

Haha, no gender-neutral word for aunt or uncle yet (to my knowledge).

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:118: let
descendant = this.getYoungestDescendant_(sibling);
On 2017/03/29 21:57:21, David Tseng wrote:
> return this.getYoungestDescendant_(sibling) || sibling;

Done.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:125: let parent =
node.parent;
On 2017/03/29 21:57:21, David Tseng wrote:
> return node.parent; (no need to do any of the below).

Done.

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/switch_access/tree_walker_unittest.gtestjs
(right):

https://codereview.chromium.org/2777203006/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/tree_walker_unittest.gtestjs:10:
function TreeWalkerUnitTest () {
On 2017/03/29 21:57:21, David Tseng wrote:
> AutomationTreeWalkerUnitTest

Done.

[elichtenberg, 2017-03-30 21:51:04]

The CQ bit was checked by elichtenberg@google.com

[elichtenberg, 2017-03-30 21:51:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2777203006/#ps40001
(title: "Responding to comments. Refactored tree_walker. Removed
testable_tree_walker.")

[commit-bot: I haz the power, 2017-03-30 21:52:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 00:16:39]

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

[commit-bot: I haz the power, 2017-03-31 00:16:40]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[elichtenberg, 2017-03-31 03:38:16]

The CQ bit was checked by elichtenberg@google.com

[commit-bot: I haz the power, 2017-03-31 03:38:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 05:20:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490931496634280,
"parent_rev": "048c6e39319b24da880452acd1f96ea8c78e515d", "commit_rev":
"ac413e5b5d9da338a6b151a583a7fed1482899c8"}

[commit-bot: I haz the power, 2017-03-31 05:21:24]

Description was changed from

==========
Added auto-scan, made some small changes to how prefs are stored, refactored.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Added auto-scan, made some small changes to how prefs are stored, refactored.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2777203006
Cr-Commit-Position: refs/heads/master@{#461047}
Committed:
https://chromium.googlesource.com/chromium/src/+/ac413e5b5d9da338a6b151a583a7...
==========

[commit-bot: I haz the power, 2017-03-31 05:21:25]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ac413e5b5d9da338a6b151a583a7...