Initial implementation of continuous read for ChromeVox Next.

Initial implementation of continuous read for ChromeVox Next.

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

[David Tseng, 2015-01-16 00:45:39]

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

[David Tseng, 2015-01-16 00:45:40]

Ready for an initial look. Tests pending.

[dmazzoni, 2015-01-16 16:42:11]

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

https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:219:
var continueReading = function(prevRange) {
Could you give either the function or the global bool flag different names here?

https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:226:
else if (evt.type == 'interrupted')
Should we also break on the 'cancelled' event or does that never happen?

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

https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:48: *
@param {Object=} opt_props Speech properties.
Rather than add yet another argument to Output - I already can't remember them
all - what would you think about using chaining instead? It'd be nice like this:

new Output()
    .withBraille()
    .withSpeech()
    .setPreviousRange(range)
    .setRange(range)
    .setEventType(type)
    .onSpeechEnded(function())
    .onSpeechCanceled(function())
    .go();

[David Tseng, 2015-01-16 19:15:20]

PTAL; test also added.

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

https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:219:
var continueReading = function(prevRange) {
On 2015/01/16 16:42:11, dmazzoni wrote:
> Could you give either the function or the global bool flag different names
here?

Done.

https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:226:
else if (evt.type == 'interrupted')
On 2015/01/16 16:42:11, dmazzoni wrote:
> Should we also break on the 'cancelled' event or does that never happen?

Didn't seem to happen for me.

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

https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:48: *
@param {Object=} opt_props Speech properties.
On 2015/01/16 16:42:11, dmazzoni wrote:
> Rather than add yet another argument to Output - I already can't remember them
> all - what would you think about using chaining instead? It'd be nice like
this:
> 
> new Output()
>     .withBraille()
>     .withSpeech()
>     .setPreviousRange(range)
>     .setRange(range)
>     .setEventType(type)
>     .onSpeechEnded(function())
>     .onSpeechCanceled(function())
>     .go();

Done.

[David Tseng, 2015-01-23 17:53:15]

Friendly ping.
dtseng@chromium.org writes:

> PTAL; test also added.
>
>
>
https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
> (right):
>
>
https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:219:
> var continueReading = function(prevRange) {
> On 2015/01/16 16:42:11, dmazzoni wrote:
>> Could you give either the function or the global bool flag different
> names here?
>
> Done.
>
>
https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:226:
> else if (evt.type == 'interrupted')
> On 2015/01/16 16:42:11, dmazzoni wrote:
>> Should we also break on the 'cancelled' event or does that never
> happen?
>
> Didn't seem to happen for me.
>
>
https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> (right):
>
>
https://codereview.chromium.org/845693004/diff/1/chrome/browser/resources/chr...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:48:
> * @param {Object=} opt_props Speech properties.
> On 2015/01/16 16:42:11, dmazzoni wrote:
>> Rather than add yet another argument to Output - I already can't
> remember them
>> all - what would you think about using chaining instead? It'd be nice
> like this:
>
>> new Output()
>>      .withBraille()
>>      .withSpeech()
>>      .setPreviousRange(range)
>>      .setRange(range)
>>      .setEventType(type)
>>      .onSpeechEnded(function())
>>      .onSpeechCanceled(function())
>>      .go();
>
> Done.
>
> https://codereview.chromium.org/845693004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.


-- 
Foo X. Bar
http://www.example.com

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-01-23 18:01:14]

The chaining looks great, it made most of the output statements much more
readable.

Was there going to be a high-level test for continuous reading, i.e. at least a
test that starts reading on a particular element and reads to the end?

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

https://codereview.chromium.org/845693004/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:227:
function() {global.isReadingContinuously = false;})
nit: spaces around the { }

[David Tseng, 2015-01-23 22:28:18]

Added e2e test. PTAL.

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

https://codereview.chromium.org/845693004/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:227:
function() {global.isReadingContinuously = false;})
On 2015/01/23 18:01:13, dmazzoni wrote:
> nit: spaces around the { }

Done.

[dmazzoni, 2015-01-23 23:39:36]

lgtm

[David Tseng, 2015-01-26 19:16:04]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-01-26 19:16:38]

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

[commit-bot: I haz the power, 2015-01-26 20:18:55]

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

[commit-bot: I haz the power, 2015-01-26 20:18:57]

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

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

[David Tseng, 2015-01-26 21:12:49]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-01-26 21:13:14]

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

[commit-bot: I haz the power, 2015-01-26 22:04:56]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-01-26 22:05:50]

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