Proposed alternative for supporting ChromeVox keyboard commands.

Proposed alternative for supporting ChromeVox keyboard commands.

ChromeVox, a component extension, has some special use cases not currently supported by the chrome.commands API. Over the past year or so, it has become more clear that these special use cases mean non-trivial changes to the commands API. These changes have also uncovered some bugs due to (a) component extension events and (b) prefs.

This is a proposed simple alternative which still utilizes the same manifest format as the commands API and the same extension listener, but performs its own dispatch. It also need not register accelerators in advance as it hooks into keyboard events via EventRewriter.

This solution has some advantages. It means that we can swap the format and use keys not allowed by the commands API. Also, we can implement more features such as sequencing, or sticky key natively.

Some disadvantages are it will interfere with Chrome OS sticky keys and is Chrome OS specific.

TEST=ensure chrome.commands.onCommand gets called with appropriate command name when keys are struck on OOBE, locked screen, login screen, and logged in.

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

[David Tseng, 2015-06-15 19:24:50]

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

[David Tseng, 2015-06-15 21:33:27]

Patchset #3 (id:40001) has been deleted

[David Tseng, 2015-06-15 21:33:35]

Patchset #2 (id:20001) has been deleted

[David Tseng, 2015-06-15 21:33:41]

Patchset #1 (id:1) has been deleted

[Finnur, 2015-06-16 11:29:49]

Interesting approach. I have on occasion wondered if the Commands API was in
fact flexible enough to support this weird spanning-of profiles that you've
described. Perhaps this way is better for everyone. Unfortunately, I can't say
for sure because I don't feel qualified to review this, so you might be better
served with someone else taking a look. I only have a couple of comments on what
relates to the extension side of things.

https://codereview.chromium.org/1185753008/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:38:
extension_misc::kChromeVoxExtensionId);
if (!extension)
  return ui::EVENT_REWRITE_CONTINUE;

GetNamedCommands does not support nullptr being passed in.

https://codereview.chromium.org/1185753008/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:63:
extension_misc::kChromeVoxExtensionId, extension_event.Pass());
The ExtensionKeybindingRegistry uses:

  event->restrict_to_browser_context = browser_context_;
  event->user_gesture = EventRouter::USER_GESTURE_ENABLED;

Wondering if you'd benefit from one or both of those? I know restricting to a
browser context was a problem before (because the context didn't match what you
needed it to). But now it should, right?

I mention the user-gesture one just so you are aware of it -- not as an action
item to add it even if you don't need it. If things work for you as is then you
are probably better off not using it. But if you find things breaking then it is
at least fresh in your head.

[David Tseng, 2015-06-16 15:31:36]

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

[David Tseng, 2015-06-16 15:31:37]

+ oshima for OWNERS and any other commentary.
dmazzoni for accessibility/ChromeVox bits.

Thanks finnur for your comments. Appreciate all of the help on commands.

https://codereview.chromium.org/1185753008/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:38:
extension_misc::kChromeVoxExtensionId);
On 2015/06/16 11:29:49, Finnur wrote:
> if (!extension)
>   return ui::EVENT_REWRITE_CONTINUE;
> 
> GetNamedCommands does not support nullptr being passed in.

Done.

https://codereview.chromium.org/1185753008/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:63:
extension_misc::kChromeVoxExtensionId, extension_event.Pass());
On 2015/06/16 11:29:48, Finnur wrote:
> The ExtensionKeybindingRegistry uses:
> 
>   event->restrict_to_browser_context = browser_context_;
>   event->user_gesture = EventRouter::USER_GESTURE_ENABLED;
> 
> Wondering if you'd benefit from one or both of those? I know restricting to a
> browser context was a problem before (because the context didn't match what
you
> needed it to). But now it should, right?

Added. Yes; with the last profile/context logic this is working in all the
places I need.
> 
> I mention the user-gesture one just so you are aware of it -- not as an action
> item to add it even if you don't need it. If things work for you as is then
you
> are probably better off not using it. But if you find things breaking then it
is
> at least fresh in your head.


Great; I'll keep that in mind.

[dmazzoni, 2015-06-16 16:30:56]

I agree that we should add some custom logic for routing key events to
ChromeVox. ChromeVox's needs are unfortunately just going beyond what the
commands API was really intended for and trying to fix all of those interactions
doesn't necessarily make sense.

Why will sticky keys break? Isn't sticky keys just another event rewriter, and
can't it just run in sequence before this one?

One question: rather than declaring all of the commands in ChromeVox's manifest,
should we consider just passing through all keystrokes involving Search and none
that don't?

Another thing to consider would be to pass all key events to ChromeVox anyway -
distinguish between Search+keys that are *intercepted* and other keys that would
just be *monitored* - that would allow us to announce all keys as they're typed,
which is a common feature of other screen readers.

https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:29:
event.type() != ui::ET_KEY_PRESSED ||
I think you need to handle ET_KEY_RELEASED too. You may need to handle sequences
of events that start out as non-cvox keys but later the search key gets added so
the sequence all becomes a cvox event. We should have tests for those sequences.

Check out touch_exploration_controller_unittest.cc for some examples of writing
event rewriter unit tests.

[David Tseng, 2015-06-16 16:52:36]

On Tue, Jun 16, 2015 at 9:30 AM, <dmazzoni@chromium.org> wrote:

Why will sticky keys break? Isn't sticky keys just another event rewriter,
and
can't it just run in sequence before this one?

It could, but then our commands lookup logic would break or we would need
to be aware of sticky keys being on.

One question: rather than declaring all of the commands in ChromeVox's
manifest,
should we consider just passing through all keystrokes involving Search and
none
that don't?

I still would like the option of using the Commands API officially whenever
we're not on Chrome OS.

The issue with trapping all search-modified keys is I don't know how you
would then re-inject the keys if ChromeVox doesn't want to process them. We
do need to discard the events that we need to process.

Another thing to consider would be to pass all key events to ChromeVox
anyway -
distinguish between Search+keys that are *intercepted* and other keys that
would
just be *monitored* - that would allow us to announce all keys as they're
typed,
which is a common feature of other screen readers.

Perhaps for a future cl?


https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:29:
event.type() != ui::ET_KEY_PRESSED ||
I think you need to handle ET_KEY_RELEASED too. You may need to handle
sequences of events that start out as non-cvox keys but later the search
key gets added so the sequence all becomes a cvox event. We should have
tests for those sequences.

Check out touch_exploration_controller_unittest.cc for some examples of
writing event rewriter unit tests.


https://codereview.chromium.org/1185753008/

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

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

[David Tseng, 2015-06-16 16:55:37]

On Tue, Jun 16, 2015 at 9:52 AM, David Tseng <dtseng@chromium.org> wrote:

>
>
> On Tue, Jun 16, 2015 at 9:30 AM, <dmazzoni@chromium.org> wrote:
>
> Why will sticky keys break? Isn't sticky keys just another event rewriter,
> and
> can't it just run in sequence before this one?
>
> It could, but then our commands lookup logic would break or we would need
> to be aware of sticky keys being on.
>
>
Actually, you're right. It should just work.

One question: rather than declaring all of the commands in ChromeVox's
> manifest,
> should we consider just passing through all keystrokes involving Search
> and none
> that don't?
>
> I still would like the option of using the Commands API officially
> whenever we're not on Chrome OS.
>
> The issue with trapping all search-modified keys is I don't know how you
> would then re-inject the keys if ChromeVox doesn't want to process them. We
> do need to discard the events that we need to process.
>
> Another thing to consider would be to pass all key events to ChromeVox
> anyway -
> distinguish between Search+keys that are *intercepted* and other keys that
> would
> just be *monitored* - that would allow us to announce all keys as they're
> typed,
> which is a common feature of other screen readers.
>
> Perhaps for a future cl?
>
>
>
>
https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
> File
> chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
> (right):
>
>
>
https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
> chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:29:
> event.type() != ui::ET_KEY_PRESSED ||
> I think you need to handle ET_KEY_RELEASED too. You may need to handle
> sequences of events that start out as non-cvox keys but later the search
> key gets added so the sequence all becomes a cvox event. We should have
> tests for those sequences.
>
> Check out touch_exploration_controller_unittest.cc for some examples of
> writing event rewriter unit tests.
>
>
> https://codereview.chromium.org/1185753008/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>
>

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

[David Tseng, 2015-06-16 17:00:01]

On Tue, Jun 16, 2015 at 9:55 AM, David Tseng <dtseng@chromium.org> wrote:


On Tue, Jun 16, 2015 at 9:52 AM, David Tseng <dtseng@chromium.org> wrote:


On Tue, Jun 16, 2015 at 9:30 AM, <dmazzoni@chromium.org> wrote:

Why will sticky keys break? Isn't sticky keys just another event rewriter,
and
can't it just run in sequence before this one?

It could, but then our commands lookup logic would break or we would need
to be aware of sticky keys being on.


Actually, you're right. It should just work.

One more note:
The reason why I left the SpokenFeedbackEventRewriter before the Sticky
key/EventRewriter is because the latter does the search+left/right =
home/end mapping, which totally breaks a readable accelerator map unless we
special case it in EventRewriter. I can change things in a future cl.

One question: rather than declaring all of the commands in ChromeVox's
manifest,
should we consider just passing through all keystrokes involving Search and
none
that don't?

I still would like the option of using the Commands API officially whenever
we're not on Chrome OS.

The issue with trapping all search-modified keys is I don't know how you
would then re-inject the keys if ChromeVox doesn't want to process them. We
do need to discard the events that we need to process.

Another thing to consider would be to pass all key events to ChromeVox
anyway -
distinguish between Search+keys that are *intercepted* and other keys that
would
just be *monitored* - that would allow us to announce all keys as they're
typed,
which is a common feature of other screen readers.

Perhaps for a future cl?


https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:29:
event.type() != ui::ET_KEY_PRESSED ||
I think you need to handle ET_KEY_RELEASED too. You may need to handle
sequences of events that start out as non-cvox keys but later the search
key gets added so the sequence all becomes a cvox event. We should have
tests for those sequences.

Check out touch_exploration_controller_unittest.cc for some examples of
writing event rewriter unit tests.


https://codereview.chromium.org/1185753008/

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

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

[David Tseng, 2015-06-17 22:22:11]

https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:29:
event.type() != ui::ET_KEY_PRESSED ||
On 2015/06/16 16:30:56, dmazzoni wrote:
> I think you need to handle ET_KEY_RELEASED too.
If you discard a press event, I think the release gets discarded.

 You may need to handle sequences
> of events that start out as non-cvox keys but later the search key gets added
so
> the sequence all becomes a cvox event. We should have tests for those
sequences.

Example? Do you mean Search+Shift+n, b (where b is unmodified)? This does appear
to break with this change.


> 
> Check out touch_exploration_controller_unittest.cc for some examples of
writing
> event rewriter unit tests.


I was thinking about just constructing a SpokenFeedbackEventTest directly and
calling |RewriteEvent|. Would hat suffice and asserting the continue/discard
return?

[dmazzoni, 2015-06-18 16:44:11]

>
> If you discard a press event, I think the release gets discarded.
>

Are you sure that's always true? Either way it needs a test.

  You may need to handle sequences
> > of events that start out as non-cvox keys but later the search key
> gets added so
> > the sequence all becomes a cvox event. We should have tests for those
> sequences.
>
> Example? Do you mean Search+Shift+n, b (where b is unmodified)? This
> does appear to break with this change.
>

How about Shift-down, Search-down, Space-down, Space-up, Shift-up,
Search-up.

My worry is that the initial Shift-down is passed through as-is, but the
Shift-up could get suppressed. Or alternatively, that Search-up gets passed
through even though the Search-down was not - that could also cause
problems.

> Check out touch_exploration_controller_unittest.cc for some examples
> of writing
> > event rewriter unit tests.
>
> I was thinking about just constructing a SpokenFeedbackEventTest
> directly and calling |RewriteEvent|. Would hat suffice and asserting the
> continue/discard return?
>

No, I think the test should actually inject real key events into Chrome OS
at a low level and assert the full sequence of events triggered.

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

[David Tseng, 2015-06-22 19:04:42]

PTAL.

I now track modifiers that are held at the time of a discarded event and discard
any released modifiers based on that.

In terms of testing, we need to have ChromeVox enabled to fully test as you
suggest (i.e. sending keys, triggering commands lookup, etc). I think it's
easiest to write SpokenFeedbackTests for this purpose. I tried writing touch
exploration like event generator tests as well as event rewriter tests based
unit tests, but needing a full ash desktop and ChromeVox made those options more
complicated.

On 2015/06/18 16:44:11, dmazzoni wrote:
> >
> > If you discard a press event, I think the release gets discarded.
> >
> 
> Are you sure that's always true? Either way it needs a test.
> 
>   You may need to handle sequences
> > > of events that start out as non-cvox keys but later the search key
> > gets added so
> > > the sequence all becomes a cvox event. We should have tests for those
> > sequences.
> >
> > Example? Do you mean Search+Shift+n, b (where b is unmodified)? This
> > does appear to break with this change.
> >
> 
> How about Shift-down, Search-down, Space-down, Space-up, Shift-up,
> Search-up.
> 
> My worry is that the initial Shift-down is passed through as-is, but the
> Shift-up could get suppressed. Or alternatively, that Search-up gets passed
> through even though the Search-down was not - that could also cause
> problems.
> 
> > Check out touch_exploration_controller_unittest.cc for some examples
> > of writing
> > > event rewriter unit tests.
> >
> > I was thinking about just constructing a SpokenFeedbackEventTest
> > directly and calling |RewriteEvent|. Would hat suffice and asserting the
> > continue/discard return?
> >
> 
> No, I think the test should actually inject real key events into Chrome OS
> at a low level and assert the full sequence of events triggered.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-06-22 20:35:38]

Overall looks good, just a thought about testing.

On 2015/06/22 19:04:42, David Tseng wrote:
> In terms of testing, we need to have ChromeVox enabled to fully test as you
> suggest (i.e. sending keys, triggering commands lookup, etc). I think it's
> easiest to write SpokenFeedbackTests for this purpose. I tried writing touch
> exploration like event generator tests as well as event rewriter tests based
> unit tests, but needing a full ash desktop and ChromeVox made those options
more
> complicated.

I think some mocking or dependency injection to make it unit-testable would be
good as part of this change. A spoken feedback test also sounds great, but
there's enough here to unit test, maybe using EventGenerator.

Replace chromeos::AccessibilityManager::Get()->IsSpokenFeedbackEnabled() with a
helper function IsSpokenFeedbackEnabled(), and then either provide a test API
like SetSpokenFeedbackEnabledForTesting(), or make it virtual and override it in
a test-only subclass.

Same for GetNamedCommands and DispatchEventToExtension - if you move those to
helper functions, you can mock them in your test and still test the rest of the
logic.

[David Tseng, 2015-06-23 17:02:55]

PTAL. Tests added.

On Mon, Jun 22, 2015 at 1:35 PM, <dmazzoni@chromium.org> wrote:

> Overall looks good, just a thought about testing.
>
> On 2015/06/22 19:04:42, David Tseng wrote:
>
>> In terms of testing, we need to have ChromeVox enabled to fully test as
>> you
>> suggest (i.e. sending keys, triggering commands lookup, etc). I think it's
>> easiest to write SpokenFeedbackTests for this purpose. I tried writing
>> touch
>> exploration like event generator tests as well as event rewriter tests
>> based
>> unit tests, but needing a full ash desktop and ChromeVox made those
>> options
>>
> more
>
>> complicated.
>>
>
> I think some mocking or dependency injection to make it unit-testable
> would be
> good as part of this change. A spoken feedback test also sounds great, but
> there's enough here to unit test, maybe using EventGenerator.
>
> Replace chromeos::AccessibilityManager::Get()->IsSpokenFeedbackEnabled()
> with a
> helper function IsSpokenFeedbackEnabled(), and then either provide a test
> API
> like SetSpokenFeedbackEnabledForTesting(), or make it virtual and override
> it in
> a test-only subclass.
>
> Same for GetNamedCommands and DispatchEventToExtension - if you move those
> to
> helper functions, you can mock them in your test and still test the rest
> of the
> logic.
>
>
> https://codereview.chromium.org/1185753008/
>

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

[dmazzoni, 2015-06-23 17:11:18]

The delegate looks great. Just one more suggestion there - I probably would have
provided a default implementation for the delegate rather than if/else
statements - but alternatively just explicitly call it a TestingDelegate and
make the variable name testing_delegate_ - that makes it more clear reading the
code when the delegate would be used.

My only last concern is about symmetrically either eating the Search+Shift key
down events or not eating the release events.

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
File
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc
(right):

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:88:
// Send Search+Shift+Right.
Nit: this comment applies to the following block of code and also the block
after it, which is a little confusing. How about "Press Search and Shift keys"
here, then "Press Right key and assert that the correct command was triggered."
or something like that.

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:97:
ASSERT_EQ(2U, event_capturer_.captured_events().size());
To clarify, the Search and Shift keys did generate events, but the Right key did
not?

Shouldn't we eat the Search and Shift key down events too? Or if we don't,
shouldn't we not eat the releases for those?

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:105:
generator_->ReleaseKey(ui::VKEY_LWIN, 128);
nit: use constants instead of magic values like 128 here

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:114:
TEST_F(SpokenFeedbackEventRewriterTest, KeysNotEatenWithChromeVoxDisabled) {
nit: blank line before this new test

[David Tseng, 2015-06-23 17:51:29]

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
File
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc
(right):

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:88:
// Send Search+Shift+Right.
On 2015/06/23 17:11:17, dmazzoni wrote:
> Nit: this comment applies to the following block of code and also the block
> after it, which is a little confusing. How about "Press Search and Shift keys"
> here, then "Press Right key and assert that the correct command was
triggered."
> or something like that.

Done.

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:97:
ASSERT_EQ(2U, event_capturer_.captured_events().size());
On 2015/06/23 17:11:17, dmazzoni wrote:
> To clarify, the Search and Shift keys did generate events, but the Right key
did
> not?

Yes.
> 
> Shouldn't we eat the Search and Shift key down events too? Or if we don't,
> shouldn't we not eat the releases for those?
> 


Went with the latter.

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:105:
generator_->ReleaseKey(ui::VKEY_LWIN, 128);
On 2015/06/23 17:11:17, dmazzoni wrote:
> nit: use constants instead of magic values like 128 here

Done.

https://codereview.chromium.org/1185753008/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:114:
TEST_F(SpokenFeedbackEventRewriterTest, KeysNotEatenWithChromeVoxDisabled) {
On 2015/06/23 17:11:17, dmazzoni wrote:
> nit: blank line before this new test

Done.

[dmazzoni, 2015-06-23 18:25:11]

lgtm

[David Tseng, 2015-06-23 19:51:18]

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

[David Tseng, 2015-06-23 19:51:20]

+ sadrul for ui/events
and oshima for chrome/browser/chromeos/chrome_browser_main_chromeos.cc

[sadrul, 2015-06-24 18:31:02]

https://codereview.chromium.org/1185753008/diff/180001/chrome/browser/chromeo...
File
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc
(right):

https://codereview.chromium.org/1185753008/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:48:
generator_->AddEventRewriter(spoken_feedback_event_rewriter_.get());
Can this just do CurrentContext()->GetHost()->GetEventSource() to get the
EventSource, and then add the rewriter there?

https://codereview.chromium.org/1185753008/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:51:
void TearDown() override {
I think you should remove the rewriter here too?

https://codereview.chromium.org/1185753008/diff/180001/ui/events/event_consta...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/1185753008/diff/180001/ui/events/event_consta...
ui/events/event_constants.h:98: EF_SHIFT_DOWN | EF_CONTROL_DOWN | EF_ALT_DOWN |
EF_COMMAND_DOWN,
Can you define there where you need it? (see for example
Textfield::InsertChar(), DesktopDragDropClientAuraX11::OnMouseMovement() etc.)

https://codereview.chromium.org/1185753008/diff/180001/ui/events/test/event_g...
File ui/events/test/event_generator.h (right):

https://codereview.chromium.org/1185753008/diff/180001/ui/events/test/event_g...
ui/events/test/event_generator.h:353: // Add an EventRewriter to the
currentEventSource.
What is currentEventSource?

[David Tseng, 2015-06-24 19:09:54]

Thanks sadrul; no longer requires your approval.

https://codereview.chromium.org/1185753008/diff/180001/chrome/browser/chromeo...
File
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc
(right):

https://codereview.chromium.org/1185753008/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:48:
generator_->AddEventRewriter(spoken_feedback_event_rewriter_.get());
On 2015/06/24 18:31:02, sadrul wrote:
> Can this just do CurrentContext()->GetHost()->GetEventSource() to get the
> EventSource, and then add the rewriter there?

Done.

https://codereview.chromium.org/1185753008/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter_unittest.cc:51:
void TearDown() override {
On 2015/06/24 18:31:02, sadrul wrote:
> I think you should remove the rewriter here too?

Done.

https://codereview.chromium.org/1185753008/diff/180001/ui/events/event_consta...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/1185753008/diff/180001/ui/events/event_consta...
ui/events/event_constants.h:98: EF_SHIFT_DOWN | EF_CONTROL_DOWN | EF_ALT_DOWN |
EF_COMMAND_DOWN,
On 2015/06/24 18:31:02, sadrul wrote:
> Can you define there where you need it? (see for example
> Textfield::InsertChar(), DesktopDragDropClientAuraX11::OnMouseMovement() etc.)

Done.

https://codereview.chromium.org/1185753008/diff/180001/ui/events/test/event_g...
File ui/events/test/event_generator.h (right):

https://codereview.chromium.org/1185753008/diff/180001/ui/events/test/event_g...
ui/events/test/event_generator.h:353: // Add an EventRewriter to the
currentEventSource.
On 2015/06/24 18:31:02, sadrul wrote:
> What is currentEventSource?

Obsolete.

[oshima, 2015-06-24 19:37:40]

lgtm

[David Tseng, 2015-06-24 20:44:26]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2015-06-24 20:44:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1185753008/#ps200001
(title: "Remove ui/events/* changes.")

[commit-bot: I haz the power, 2015-06-24 20:45:47]

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

[commit-bot: I haz the power, 2015-06-24 20:51:24]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2015-06-24 20:53:13]

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

[stevenjb, 2015-06-25 16:10:13]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2015-06-25 16:10:14]

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:109: if
(delegate_->DispatchKeyToChromeVox(key_event)) {
We are seeing a crash in DispatchKeyToChromeVox when it calls
ProfileManager::GetLastUsedProfile when called from here. Here is a crash:

https://00e9e64bac9e55b1ad3bc989f7fe6755e50c3d830bde6537f1-apidata.googleuser...

I am going to file an issue for this, and will try to use cbuildbot to confirm,
but if you can think how this might happen after the Profile has been destroyed,
we should revert this.

[stevenjb, 2015-06-25 16:31:44]

A revert of this CL (patchset #8 id:200001) has been created in
https://codereview.chromium.org/1211003002/ by stevenjb@chromium.org.

The reason for reverting is: Appears to be causing crashes. See
https://crbug.com/504400.
.

[David Tseng, 2015-06-25 16:44:06]

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:109: if
(delegate_->DispatchKeyToChromeVox(key_event)) {
On 2015/06/25 16:10:14, stevenjb wrote:
> We are seeing a crash in DispatchKeyToChromeVox when it calls
> ProfileManager::GetLastUsedProfile when called from here. Here is a crash:
> 
>
https://00e9e64bac9e55b1ad3bc989f7fe6755e50c3d830bde6537f1-apidata.googleuser...
> 
> I am going to file an issue for this, and will try to use cbuildbot to
confirm,
> but if you can think how this might happen after the Profile has been
destroyed,
> we should revert this.

According to the stack, it looks like the call to
ProfileManager::GetLastUsedProfile eventually calls
ProfileManager::CreateAndInitializeProfile which causes an out of memory error.
Oshima, any comments on this particular theory?

[oshima, 2015-06-25 17:02:37]

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:109: if
(delegate_->DispatchKeyToChromeVox(key_event)) {
On 2015/06/25 16:44:06, David Tseng wrote:
> On 2015/06/25 16:10:14, stevenjb wrote:
> > We are seeing a crash in DispatchKeyToChromeVox when it calls
> > ProfileManager::GetLastUsedProfile when called from here. Here is a crash:
> > 
> >
>
https://00e9e64bac9e55b1ad3bc989f7fe6755e50c3d830bde6537f1-apidata.googleuser...
> > 
> > I am going to file an issue for this, and will try to use cbuildbot to
> confirm,
> > but if you can think how this might happen after the Profile has been
> destroyed,
> > we should revert this.
> 
> According to the stack, it looks like the call to
> ProfileManager::GetLastUsedProfile eventually calls
> ProfileManager::CreateAndInitializeProfile which causes an out of memory
error.
> Oshima, any comments on this particular theory?

I couldn't access the stack info. Can you post it here?

[stevenjb, 2015-06-25 17:09:25]

See http://crbug.com/504400
(And let's continue any discussion there)


On Thu, Jun 25, 2015 at 11:02 AM, <oshima@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
> File
> chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
> (right):
>
>
>
https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
>
> chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:109:
> if (delegate_->DispatchKeyToChromeVox(key_event)) {
> On 2015/06/25 16:44:06, David Tseng wrote:
>
>> On 2015/06/25 16:10:14, stevenjb wrote:
>> > We are seeing a crash in DispatchKeyToChromeVox when it calls
>> > ProfileManager::GetLastUsedProfile when called from here. Here is a
>>
> crash:
>
>> >
>> >
>>
>
>
>
https://00e9e64bac9e55b1ad3bc989f7fe6755e50c3d830bde6537f1-apidata.googleuser...
>
>> >
>> > I am going to file an issue for this, and will try to use cbuildbot
>>
> to
>
>> confirm,
>> > but if you can think how this might happen after the Profile has
>>
> been
>
>> destroyed,
>> > we should revert this.
>>
>
>  According to the stack, it looks like the call to
>> ProfileManager::GetLastUsedProfile eventually calls
>> ProfileManager::CreateAndInitializeProfile which causes an out of
>>
> memory error.
>
>> Oshima, any comments on this particular theory?
>>
>
> I couldn't access the stack info. Can you post it here?
>
> https://codereview.chromium.org/1185753008/
>

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

[oshima, 2015-06-25 17:13:37]

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1185753008/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:109: if
(delegate_->DispatchKeyToChromeVox(key_event)) {
On 2015/06/25 17:02:37, oshima wrote:
> On 2015/06/25 16:44:06, David Tseng wrote:
> > On 2015/06/25 16:10:14, stevenjb wrote:
> > > We are seeing a crash in DispatchKeyToChromeVox when it calls
> > > ProfileManager::GetLastUsedProfile when called from here. Here is a crash:
> > > 
> > >
> >
>
https://00e9e64bac9e55b1ad3bc989f7fe6755e50c3d830bde6537f1-apidata.googleuser...
> > > 
> > > I am going to file an issue for this, and will try to use cbuildbot to
> > confirm,
> > > but if you can think how this might happen after the Profile has been
> > destroyed,
> > > we should revert this.
> > 
> > According to the stack, it looks like the call to
> > ProfileManager::GetLastUsedProfile eventually calls
> > ProfileManager::CreateAndInitializeProfile which causes an out of memory
> error.
> > Oshima, any comments on this particular theory?
> 
> I couldn't access the stack info. Can you post it here?

My guess is that this is called before profiles are fully loaded.

I don't know why CreateAndInitializeProfile crashes, but I think you shouldn't
call this unless
profiles are loaded anyway. There is NOTIFICATION_PROFILE_ADDED notification, so
you can try that.
(I'm not sure if this is called for login profile. you may need different way to
wire this up if not)