Adjust ChromeVox (spoken feedback) strings and make it possible for announcemen

Adjust ChromeVox (spoken feedback) strings and make it possible for announcemen
ts to occur directly in the extension.

BUG=261451
TEST=On Aura, rapidly turn ChromeVox on and off (on login/locked screen, user s
creens). Verify behaviors in above bug are true. Depends on a ChromeVox change.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232886

[David Tseng, 2013-10-25 02:15:55]

Either plundblad or dmazzoni feel free to review.
aboxhall FYI :).

[dmazzoni, 2013-10-25 23:04:02]

+kalman for chrome/common/extensions OWNERS review

[dmazzoni, 2013-10-25 23:09:15]

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibil...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibil...
chrome/browser/accessibility/accessibility_extension_api.cc:218:
system->lazy_background_task_queue()->AddPendingTask(
nit: maybe stick system->lazy_background_task_queue() in a local variable for
less line wrapping

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibil...
File chrome/browser/accessibility/accessibility_extension_api.h (right):

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibil...
chrome/browser/accessibility/accessibility_extension_api.h:19: struct Event;
unused?

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibil...
chrome/browser/accessibility/accessibility_extension_api.h:65: static void
DispatchEventToChromeVox(Profile* profile,
nit: when doing one argument per line, line them up - in this case, put a
newline first and then Profile* profile, indented 4.

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:414: // Give
ChromeVox a chance to speak/braille outgoing messages.
How about "announce" instead of speak/braille

https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:420:
base::TimeDelta::FromSeconds(1));
I know it's trivial, but let's make 1 a named constant.

I'm a little wary of any timeout other than zero. Is this really best?

[not at google - send to devlin, 2013-10-28 16:59:25]

lgtm

side note: BUG pending (or maybe I just can't find it) but can you/we look at
moving these functions into the "accessibility" namespace? If they're not ready
to release then use _api_features to guard behind dev-channel only (or similar)
but whitelist for ChromeVox.

An OKR for this quarter is to remove all code that deals with experimental APIs.

[David Tseng, 2013-10-28 20:55:33]

On Fri, Oct 25, 2013 at 4:09 PM, <dmazzoni@chromium.org> wrote:

>
> https://codereview.chromium.**org/42743003/diff/30001/**
>
chrome/browser/accessibility/**accessibility_extension_api.cc<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.cc>
> File chrome/browser/accessibility/**accessibility_extension_api.cc
> (right):
>
> https://codereview.chromium.**org/42743003/diff/30001/**
>
chrome/browser/accessibility/**accessibility_extension_api.**cc#newcode218<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.cc#newcode218>
> chrome/browser/accessibility/**accessibility_extension_api.**cc:218:
> system->lazy_background_task_**queue()->AddPendingTask(
> nit: maybe stick system->lazy_background_task_**queue() in a local
> variable for less line wrapping
> Done; much more verbose, but probably more clear.
> https://codereview.chromium.**org/42743003/diff/30001/**
>
chrome/browser/accessibility/**accessibility_extension_api.h<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.h>
> File chrome/browser/accessibility/**accessibility_extension_api.h (right):
>
> https://codereview.chromium.**org/42743003/diff/30001/**
>
chrome/browser/accessibility/**accessibility_extension_api.h#**newcode19<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.h#newcode19>
> chrome/browser/accessibility/**accessibility_extension_api.h:**19: struct
> Event;
> unused?
> Done.
> https://codereview.chromium.**org/42743003/diff/30001/**
>
chrome/browser/accessibility/**accessibility_extension_api.h#**newcode65<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.h#newcode65>
> chrome/browser/accessibility/**accessibility_extension_api.h:**65: static
> void DispatchEventToChromeVox(**Profile* profile,
> nit: when doing one argument per line, line them up - in this case, put
> a newline first and then Profile* profile, indented 4.
> Done.
> https://codereview.chromium.**org/42743003/diff/30001/**
>
chrome/browser/chromeos/**accessibility/accessibility_**manager.cc<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc>
> File chrome/browser/chromeos/**accessibility/accessibility_**manager.cc
> (right):
>
> https://codereview.chromium.**org/42743003/diff/30001/**
> chrome/browser/chromeos/**accessibility/accessibility_**
>
manager.cc#newcode414<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc#newcode414>
> chrome/browser/chromeos/**accessibility/accessibility_**manager.cc:414: //
> Give ChromeVox a chance to speak/braille outgoing messages.
> How about "announce" instead of speak/braille
> Done.
> https://codereview.chromium.**org/42743003/diff/30001/**
> chrome/browser/chromeos/**accessibility/accessibility_**
>
manager.cc#newcode420<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc#newcode420>
> chrome/browser/chromeos/**accessibility/accessibility_**manager.cc:420:
> base::TimeDelta::FromSeconds(**1));
> I know it's trivial, but let's make 1 a named constant.
>
> I'm a little wary of any timeout other than zero. Is this really best?
>
> We need to give ChromeVox the chance to relay any remaining messages (to
tts and earcons). Not sure how to accomplish this otherwise without adding
an "ack" type of api for ChromeVox.

https://codereview.chromium.**org/42743003/<https://codereview.chromium.org/4...
>

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

[David Tseng, 2013-10-29 18:55:56]

PTAL?



On Mon, Oct 28, 2013 at 1:55 PM, David Tseng <dtseng@chromium.org> wrote:

>
>
>
> On Fri, Oct 25, 2013 at 4:09 PM, <dmazzoni@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>>
chrome/browser/accessibility/**accessibility_extension_api.cc<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.cc>
>> File chrome/browser/accessibility/**accessibility_extension_api.cc
>> (right):
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>> chrome/browser/accessibility/**accessibility_extension_api.**
>>
cc#newcode218<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.cc#newcode218>
>> chrome/browser/accessibility/**accessibility_extension_api.**cc:218:
>> system->lazy_background_task_**queue()->AddPendingTask(
>> nit: maybe stick system->lazy_background_task_**queue() in a local
>> variable for less line wrapping
>> Done; much more verbose, but probably more clear.
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>>
chrome/browser/accessibility/**accessibility_extension_api.h<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.h>
>> File chrome/browser/accessibility/**accessibility_extension_api.h
>> (right):
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>>
chrome/browser/accessibility/**accessibility_extension_api.h#**newcode19<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.h#newcode19>
>> chrome/browser/accessibility/**accessibility_extension_api.h:**19: struct
>> Event;
>> unused?
>> Done.
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>>
chrome/browser/accessibility/**accessibility_extension_api.h#**newcode65<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/accessibility/accessibility_extension_api.h#newcode65>
>> chrome/browser/accessibility/**accessibility_extension_api.h:**65: static
>> void DispatchEventToChromeVox(**Profile* profile,
>> nit: when doing one argument per line, line them up - in this case, put
>> a newline first and then Profile* profile, indented 4.
>> Done.
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>>
chrome/browser/chromeos/**accessibility/accessibility_**manager.cc<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc>
>> File chrome/browser/chromeos/**accessibility/accessibility_**manager.cc
>> (right):
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>> chrome/browser/chromeos/**accessibility/accessibility_**
>>
manager.cc#newcode414<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc#newcode414>
>> chrome/browser/chromeos/**accessibility/accessibility_**manager.cc:414:
>> //
>> Give ChromeVox a chance to speak/braille outgoing messages.
>> How about "announce" instead of speak/braille
>> Done.
>>
>> https://codereview.chromium.**org/42743003/diff/30001/**
>> chrome/browser/chromeos/**accessibility/accessibility_**
>>
manager.cc#newcode420<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc#newcode420>
>> chrome/browser/chromeos/**accessibility/accessibility_**manager.cc:420:
>> base::TimeDelta::FromSeconds(**1));
>> I know it's trivial, but let's make 1 a named constant.
>>
>> I'm a little wary of any timeout other than zero. Is this really best?
>>
>> We need to give ChromeVox the chance to relay any remaining messages (to
> tts and earcons). Not sure how to accomplish this otherwise without adding
> an "ack" type of api for ChromeVox.
>
>
https://codereview.chromium.**org/42743003/<https://codereview.chromium.org/4...
>>
>
>

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

[dmazzoni, 2013-10-29 19:03:01]

On 2013/10/29 18:55:56, David Tseng wrote:
>
manager.cc#newcode420<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc#newcode420>
> >> chrome/browser/chromeos/**accessibility/accessibility_**manager.cc:420:
> >> base::TimeDelta::FromSeconds(**1));
> >> I know it's trivial, but let's make 1 a named constant.
> >>
> >> I'm a little wary of any timeout other than zero. Is this really best?
> >>
> >> We need to give ChromeVox the chance to relay any remaining messages (to
> > tts and earcons). Not sure how to accomplish this otherwise without adding
> > an "ack" type of api for ChromeVox.

It still seems possible that if the system is really sluggish, we could still
drop those messages. I can't think of an easy solution that's totally robust,
though. A TODO indicating this is not ideal would make me happier.

[dmazzoni, 2013-10-29 19:08:43]

lgtm with one nit

https://codereview.chromium.org/42743003/diff/170001/chrome/browser/accessibi...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

https://codereview.chromium.org/42743003/diff/170001/chrome/browser/accessibi...
chrome/browser/accessibility/accessibility_extension_api.cc:218:
extensions::LazyBackgroundTaskQueue* queue =
Move this to before the "if" because the conditional clause also references
system->lazy_background_task_queue().

Also, a few using declarations at the top of the file would make this more
concise. How about:

using extensions::LazyBackgroundTaskQueue
using extensions::BackgroundInfo

[David Tseng, 2013-10-29 20:32:44]

On Tue, Oct 29, 2013 at 12:03 PM, <dmazzoni@chromium.org> wrote:

> On 2013/10/29 18:55:56, David Tseng wrote:
>
> manager.cc#newcode420<https://**codereview.chromium.org/**
> 42743003/diff/30001/chrome/**browser/chromeos/**
>
accessibility/accessibility_**manager.cc#newcode420<https://codereview.chromium.org/42743003/diff/30001/chrome/browser/chromeos/accessibility/accessibility_manager.cc#newcode420>
> >
>
>> >> chrome/browser/chromeos/****accessibility/accessibility_****
>> manager.cc:420:
>> >> base::TimeDelta::FromSeconds(****1));
>>
>> >> I know it's trivial, but let's make 1 a named constant.
>> >>
>> >> I'm a little wary of any timeout other than zero. Is this really best?
>> >>
>> >> We need to give ChromeVox the chance to relay any remaining messages
>> (to
>> > tts and earcons). Not sure how to accomplish this otherwise without
>> adding
>> > an "ack" type of api for ChromeVox.
>>
>
> It still seems possible that if the system is really sluggish, we could
> still
> drop those messages. I can't think of an easy solution that's totally
> robust,
> though. A TODO indicating this is not ideal would make me happier.
>
> Ok; added TODO.
>
https://codereview.chromium.**org/42743003/<https://codereview.chromium.org/4...
>

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

[dmazzoni, 2013-11-01 14:49:54]

lgtm

[David Tseng, 2013-11-02 03:00:32]

PTAL.

I went ahead and reworked the logic to get rid of the delayed post task.

Now, it is assumed that ChromeVox will remove it's listener for the
onChromeVoxLoadStateChanged API once it receives loading=false (unloading).
It can cleanup and do whatever it needs to, then remove the actual
listener; this resultsin the actual unload on the C++ side.




On Fri, Nov 1, 2013 at 7:49 AM, <dmazzoni@chromium.org> wrote:

> lgtm
>
>
>
>
https://codereview.chromium.**org/42743003/<https://codereview.chromium.org/4...
>

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

[dmazzoni, 2013-11-04 16:04:41]

still lgtm - great!

[commit-bot: I haz the power, 2013-11-04 18:10:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/42743003/360001

[commit-bot: I haz the power, 2013-11-05 00:09:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/42743003/360001

[commit-bot: I haz the power, 2013-11-05 00:43:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/42743003/360001

[commit-bot: I haz the power, 2013-11-05 02:33:38]

Change committed as 232886