Begin introducing some of the initial pieces for ChromeVox next commands.

Begin introducing some of the initial pieces for ChromeVox next commands.

This cl adds the basic ChromeVox key bindings (Search+Shift+<arrows>). The command names currently serve as a placeholder pending a decision on what we should map them to.

TEST=Manually verify that the onCommand handler gets called for each command on each key combo only on the whitelisted page. Also, verify that ChromeVox uses the automation tree on the whitelisted pages while allowing content script control on all other pages. Desktop tree should always verbalize.

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

[David Tseng, 2014-09-03 23:38:53]

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

[dmazzoni, 2014-09-04 15:55:42]

This is great.

I really think we need to start adding high-level tests. How about adding a
SpokenFeedbackBrowserTest that runs with the chromevox_next compiler flag and
runs some tests?

One idea: in addition to whitelisting chromevox.com, maybe whitelist anything
that has chromevox_next in the url fragment - that way in a test you could
visit, say, chrome://settings/#chromevox_next and test that out.

https://codereview.chromium.org/536233002/diff/1/chrome/browser/extensions/gl...
File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right):

https://codereview.chromium.org/536233002/diff/1/chrome/browser/extensions/gl...
chrome/browser/extensions/global_shortcut_listener_chromeos.cc:102: LOG(ERROR)
<< "processing!";
delete this?

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2 (right):

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:24:
"commandLinePrivate",
nit: indentation

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js
(right):

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:54:
chrome.commands.onCommand.removeListener(this.onGotCommand);
Unless I'm misunderstanding, I don't think you want to do this when a tab is
updated. I'm pretty sure onTabUpdated can get called on a background tab, like
if a background tab decides to refresh or navigate on its own. I think you
should only do this in onActivated.

Also, if you remove the listener do you also need to enable ChromeVox (via
javascript)?

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:87:
cvox.ChromeVox.tts.speak(output, 0);
please use QUEUE_MODE_FLUSH instead of 0

[David Tseng, 2014-09-04 18:37:43]

https://codereview.chromium.org/536233002/diff/1/chrome/browser/extensions/gl...
File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right):

https://codereview.chromium.org/536233002/diff/1/chrome/browser/extensions/gl...
chrome/browser/extensions/global_shortcut_listener_chromeos.cc:102: LOG(ERROR)
<< "processing!";
On 2014/09/04 15:55:42, dmazzoni wrote:
> delete this?

Hmm...weird rebasing issue. The native files in this patchset are from an
upstream branch.

https://codereview.chromium.org/536233002/diff/1/chrome/browser/extensions/gl...
chrome/browser/extensions/global_shortcut_listener_chromeos.cc:102: LOG(ERROR)
<< "processing!";
On 2014/09/04 15:55:42, dmazzoni wrote:
> delete this?

Done.

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2 (right):

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:24:
"commandLinePrivate",
On 2014/09/04 15:55:42, dmazzoni wrote:
> nit: indentation

Done.

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js
(right):

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:54:
chrome.commands.onCommand.removeListener(this.onGotCommand);
On 2014/09/04 15:55:42, dmazzoni wrote:
> Unless I'm misunderstanding, I don't think you want to do this when a tab is
> updated. I'm pretty sure onTabUpdated can get called on a background tab, like
> if a background tab decides to refresh or navigate on its own. I think you
> should only do this in onActivated.
> 
> Also, if you remove the listener do you also need to enable ChromeVox (via
> javascript)?

Done.

Disabling only occurs if the tab is whitelisted. A tab should be enabled by
default.

https://codereview.chromium.org/536233002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:87:
cvox.ChromeVox.tts.speak(output, 0);
On 2014/09/04 15:55:42, dmazzoni wrote:
> please use QUEUE_MODE_FLUSH instead of 0

Done.

[David Tseng, 2014-09-04 22:47:49]

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

[David Tseng, 2014-09-04 22:47:50]

+ Peter; would love your feedback/thoughts as well.
Still looking into a test.

[Peter Lundblad, 2014-09-05 07:21:54]

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right):

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_chromeos.cc:102: LOG(ERROR)
<< "processing!";
Don't forget this one.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2 (right):

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:23:
"commands.accessibility",
Should this not be conditioned on chromevox_next?

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:56:
"description": "Moves to the next element",
Either a TODO or ideally don't defer i18n here.  You mentioned these are
placeholders,
but keep it in mind.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js
(right):

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:22:
* A list of sites to use with ChromeVox next.
Define more precisely what the string should be.  From the function that checks
this it looks like it just needs to be a substring.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:25:
this.whitelist_ = ['http://www.chromevox.com/', 'chromevox_next'];
What site is 'chromevox_next'?

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:29:
//accessibility_api_handler.js.
nit: space after //

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:42:
/**
nit: indent

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:47:
onTabActivated: function(activatedInfo) {
Does this logic work in web content that are not tabs such as webui, login
screen, dialogs etc?

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:58:
chrome.commands.onCommand.addListener(this.onGotCommand.bind(this));
nit: indent

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:100:
* @return {boolean} Whether the given |url| is whitelisted.
nit: indent

[David Tseng, 2014-09-08 19:24:35]

PTAL.
@dmazzoni. onActivated doesn't seem to be the right tabs API. onUpdated seems to
still be the right place to listen for tab updates and to add/remove listeners
for cvox next commands. I was seeing onActivated called only when switching
tabs. The other issue was that onActivated sometimes didn't have a url
populated.

@plundblad

Fixed the test. The upstream changes still seem to show up here; Rietveld's not
working as advertised it seems or I'm doing something wrong. Docs say that sub
branches with parent issues assigned to another codereview should just work.

This cl still depends on some commands/keyboard fixes pending.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right):

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_chromeos.cc:102: LOG(ERROR)
<< "processing!";
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> Don't forget this one.

Rebasing/rietveld showing upstream for whatever reason. Seems to be better in
latest patchset.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2 (right):

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:23:
"commands.accessibility",
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> Should this not be conditioned on chromevox_next?

Done.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:56:
"description": "Moves to the next element",
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> Either a TODO or ideally don't defer i18n here.  You mentioned these are
> placeholders,
> but keep it in mind.

Acknowledged.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js
(right):

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:22:
* A list of sites to use with ChromeVox next.
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> Define more precisely what the string should be.  From the function that
checks
> this it looks like it just needs to be a substring.

Done.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:25:
this.whitelist_ = ['http://www.chromevox.com/', 'chromevox_next'];
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> What site is 'chromevox_next'?

See new comments. Matches substrings now, so chromevox_next is meant to match a
test site.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:29:
//accessibility_api_handler.js.
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> nit: space after //

Done.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:42:
/**
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> nit: indent

Done.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:47:
onTabActivated: function(activatedInfo) {
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> Does this logic work in web content that are not tabs such as webui, login
> screen, dialogs etc?

Nope; but it should still fall back to classic ChromeVox.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:58:
chrome.commands.onCommand.addListener(this.onGotCommand.bind(this));
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> nit: indent

Done.

https://codereview.chromium.org/536233002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.js:100:
* @return {boolean} Whether the given |url| is whitelisted.
On 2014/09/05 07:21:54, Peter Lundblad wrote:
> nit: indent

Done.

[Peter Lundblad, 2014-09-09 12:03:54]

lgtm

https://codereview.chromium.org/536233002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2 (right):

https://codereview.chromium.org/536233002/diff/80001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:24:
"commands.accessibility",
nit: indent

[David Tseng, 2014-09-16 00:55:48]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-09-16 01:11:29]

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

[commit-bot: I haz the power, 2014-09-16 02:28:05]

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

[commit-bot: I haz the power, 2014-09-16 02:28:06]

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

[David Tseng, 2014-09-16 04:21:54]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-09-16 04:23:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/536233002/120001

[commit-bot: I haz the power, 2014-09-16 05:10:42]

Committed patchset #7 (id:120001) as 4fa338c228574dfb6dd58109880ad48a4e0f2830

[commit-bot: I haz the power, 2014-09-16 05:12:01]

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