Split spoken feedback up on a per format rule basis.

Split spoken feedback up on a per format rule basis.

Committed: https://crrev.com/245c79e93e9c0ae0261e441537ecc35c78fcb134
Cr-Commit-Position: refs/heads/master@{#325301}

[David Tseng, 2015-03-26 17:20:10]

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

[Peter Lundblad, 2015-03-30 15:27:46]

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

https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:373:
splits = splits.filter(function(i) {
When are we going to have duplicates here?

https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:798: if
(needsPrefix) {
Indentation.

https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:654:
Dlg
Lowercase.

[David Tseng, 2015-03-30 16:56:20]

plundblad@chromium.org writes:

>
https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> (right):
>
>
https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:373:
> splits = splits.filter(function(i) {
> When are we going to have duplicates here?
>
One example: if the format rule results in an empty string.

>
https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:798:
> if (needsPrefix) {
> Indentation.

Done

>
https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
> (right):
>
>
https://codereview.chromium.org/1035983003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:654:
> Dlg
> Lowercase.
>
Done

> https://codereview.chromium.org/1035983003/


-- 

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

[David Tseng, 2015-04-07 19:33:10]

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

[David Tseng, 2015-04-07 19:33:11]

+ dmazzoni

Reworked this cl to represent output as an array of spannables. This seemed to
clean things up more nicely than trying to mark up the spannables as a whole.

[David Tseng, 2015-04-13 22:43:34]

Friendly Monday ping.

On 2015/04/07 19:33:11, David Tseng wrote:
> + dmazzoni
> 
> Reworked this cl to represent output as an array of spannables. This seemed to
> clean things up more nicely than trying to mark up the spannables as a whole.

[David Tseng, 2015-04-13 22:45:40]

Patchset #22 (id:360002) has been deleted

[David Tseng, 2015-04-13 22:45:56]

Patchset #21 (id:400001) has been deleted

[David Tseng, 2015-04-13 22:46:13]

Patchset #20 (id:380001) has been deleted

[David Tseng, 2015-04-13 22:46:25]

Patchset #19 (id:360001) has been deleted

[dmazzoni, 2015-04-13 22:56:52]

Idea sounds great, just one possible concern

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:133: *
Metadata about supported automation states.
To clarify, this will eventually include other states like selected, etc?

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:397:
this.speechProperties_['startCallback'] = this.speechStartCallback_;
It seems to me like this will execute the start and end callbacks once for each
buffer in the list, is that what you intended? I think it should execute the
start callback only once for the first buffer, and the end one only once for the
last buffer.

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs
(right):

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs:59:
o.toSpannable());
nit: indentation (3x in this file with the same line essentially)

[David Tseng, 2015-04-15 17:58:36]

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:133: *
Metadata about supported automation states.
On 2015/04/13 22:56:52, dmazzoni wrote:
> To clarify, this will eventually include other states like selected, etc?

Yes; that's right.

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs
(right):

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs:59:
o.toSpannable());
On 2015/04/13 22:56:52, dmazzoni wrote:
> nit: indentation (3x in this file with the same line essentially)

Done.

[David Tseng, 2015-04-15 18:00:51]



https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1035983003/diff/420001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:397:
this.speechProperties_['startCallback'] = this.speechStartCallback_;
On 2015/04/13 22:56:52, dmazzoni wrote:
> It seems to me like this will execute the start and end callbacks once for
each
> buffer in the list, is that what you intended? I think it should execute the
> start callback only once for the first buffer, and the end one only once for
the
> last buffer.

Done.

[dmazzoni, 2015-04-15 20:22:55]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-04-15 20:22:56]

lgtm

[commit-bot: I haz the power, 2015-04-15 20:24:11]

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

[commit-bot: I haz the power, 2015-04-15 20:55:01]

Committed patchset #20 (id:440001)

[commit-bot: I haz the power, 2015-04-15 20:55:57]

Patchset 20 (id:??) landed as
https://crrev.com/245c79e93e9c0ae0261e441537ecc35c78fcb134
Cr-Commit-Position: refs/heads/master@{#325301}

[maniscalco, 2015-04-15 23:09:34]

A revert of this CL (patchset #20 id:440001) has been created in
https://codereview.chromium.org/1060913004/ by maniscalco@chromium.org.

The reason for reverting is: Looks like this is causing test timeouts on linux
asan.  For details, see
https://code.google.com/p/chromium/issues/detail?id=477492
.