Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814213002:
Refactor Select-to-speak so that mouse events are forwarded to the extension. (Closed)
|
Created:
3 years, 8 months ago by dmazzoni Modified:
3 years, 7 months ago Reviewers:
David Tseng CC:
chromium-reviews, mlamouri+watch-content_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, jam, nektar+watch_chromium.org, yuzo+watch_chromium.org, dougt+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, je_julie, dtapuska+chromiumwatch_chromium.org, davemoore+watch_chromium.org, arv+watch_chromium.org, dtapuska Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor Select-to-speak so that mouse events are forwarded to the extension.
Previously the Select-to-speak event handler captured mouse events and
turned them into accessibility events that could be caught by
select-to-speak.
Instead, the event handler can mouse and keyboard events to the extension
background page, and the extension can use the automation API to do a hit test.
This is mostly a refactoring change, but it gives us some more flexibility
and makes the event handler simpler, possibly something that could be
general-purpose, while putting more of the logic into the extension code.
BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2814213002
Cr-Commit-Position: refs/heads/master@{#472339}
Committed: https://chromium.googlesource.com/chromium/src/+/ccb40f537fbbc17772e7255a7b575661ce4353cd
Patch Set 1 #Patch Set 2 : Rebase, now no changes to content/renderer are needed #Patch Set 3 : Tests updated, ready for review #
Total comments: 26
Patch Set 4 : Forward key events to extension #Patch Set 5 : Fix closure compiler warnings #
Total comments: 13
Patch Set 6 : Address feedback #Patch Set 7 : closure #
Dependent Patchsets: Messages
Total messages: 53 (30 generated)
Description was changed from ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can just forward the mouse event to the extension background page, and the extension can use the automation API to do a hit test. This is just a pure refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. This requires a fix to RAF-aligned mouse events, since the background page never generates any compositing frames. BUG=699617 ========== to ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can just forward the mouse event to the extension background page, and the extension can use the automation API to do a hit test. This is just a pure refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. This requires a fix to RAF-aligned mouse events, since the background page never generates any compositing frames. BUG=699617 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dmazzoni@chromium.org changed reviewers: + dtapuska@chromium.org, dtseng@chromium.org
On 2017/04/12 19:47:23, dmazzoni wrote: Are you able to wait on this change for a little bit? Change https://codereview.chromium.org/2813683002/ will simplify this greatly... The MainThreadEventQueue gets created inside the the RenderWidget in that change.
Sure, sounds good.
On 2017/04/12 20:48:33, dmazzoni wrote: > Sure, sounds good. That change landed today.. and I've put up a modified fix here: https://codereview.chromium.org/2835673002/
Thanks! I'll give it a try On Fri, Apr 21, 2017 at 10:25 AM <dtapuska@chromium.org> wrote: > On 2017/04/12 20:48:33, dmazzoni wrote: > > Sure, sounds good. > > That change landed today.. and I've put up a modified fix here: > https://codereview.chromium.org/2835673002/ > > > https://codereview.chromium.org/2814213002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/21 17:33:12, dmazzoni wrote: > Thanks! I'll give it a try > > On Fri, Apr 21, 2017 at 10:25 AM <mailto:dtapuska@chromium.org> wrote: > > > On 2017/04/12 20:48:33, dmazzoni wrote: > > > Sure, sounds good. > > > > That change landed today.. and I've put up a modified fix here: > > https://codereview.chromium.org/2835673002/ > > > > > > https://codereview.chromium.org/2814213002/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. raf alignment is now disabled for background pages (that change just landed). So you should be able to give this ago with just your accessibility changes.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
RAF-aligned mouse events are no longer causing a problem, thanks! Not quite ready to land, I need to fix a couple of tests.
Description was changed from ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can just forward the mouse event to the extension background page, and the extension can use the automation API to do a hit test. This is just a pure refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. This requires a fix to RAF-aligned mouse events, since the background page never generates any compositing frames. BUG=699617 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can just forward the mouse event to the extension background page, and the extension can use the automation API to do a hit test. This is mostly a refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. BUG=699617 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dmazzoni@chromium.org changed reviewers: - dtapuska@chromium.org
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Finished, tests updated, ready for review. dtapuska to cc since this no longer requires any changes to RAF-aligned events.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc (right): https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:31: static_cast<aura::Window*>(event.target())->GetRootWindow(); Are all of these (|root|, |event.target()| always non-null? https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (left): https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:128: * @param {!AutomationEvent} evt This is a DOM event listener, not an automation event listener https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:129: */ @return https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:136: // Walk up to the nearest window, web area, or dialog that the Might as well include toolbar as well. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (right): https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:75: // A MOUSE_CANCELED accessibility event is fired if the user releases Would be great to also handle key events in this extension; it's still looking to be split in ways that a new reader would find confusing. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:108: * @param {!Event} evt The DOM event Dom event listeners have a return value (i.e. add @return). https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:126: * @param {!Event} evt The DOM event Ditto, @return https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:159: // onMouseReleasedHitTest_. Just wondering, but it seems confusing to have a mouse released event in the accessibility enums. Can we use some other event here and remove the mouse related events from accessibility? https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:164: * Called in response to our hit test afterthe mouse is released, nit: after the https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:170: onMouseReleasedHitTest_: function(evt) { nit: Maybe rename the listeners like onAutomationFooEvent and onMouseFooEvent https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:196: document.addEventListener('mousedown', this.onMouseDown_.bind(this)); Do these listeners need to be removed at some point or does the reader need to know that mouse events don't get sent when some things occur in the native event handler? https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:198: document.addEventListener('mousemove', this.onMouseMove_.bind(this)); nit: sort
One more note. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (left): https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:99: this.down_ = true; Now that you have the mouse event, I don't think you need to track down state. Just use |evt.which|. Also, evt.metaKey, evt.ctrlKey, ... might clean things up dramatically as you can just check them in all of the handlers and remove state from the native handler (and possibly here).
Now forwards key events to the extension, PTAL. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc (right): https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:31: static_cast<aura::Window*>(event.target())->GetRootWindow(); On 2017/05/07 06:10:27, David Tseng wrote: > Are all of these (|root|, |event.target()| always non-null? I checked a few other places and it seems like these should always be non-null. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (left): https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:99: this.down_ = true; On 2017/05/07 06:59:40, David Tseng wrote: > Now that you have the mouse event, I don't think you need to track down state. > Just use |evt.which|. Changed to this.trackingMouse_ - it's not whether or not the button is down, it's whether or not we're currently tracking it. > Also, evt.metaKey, evt.ctrlKey, ... > might clean things up dramatically as you can just check them in all of the > handlers and remove state from the native handler (and possibly here). There's some state changes that need to happen on up and down, so I'm not sure evt.metaKey or evt.ctrlKey would help us much. The logic is basically all in the extension now, though, so see what you think. Some examples of corner cases that might fail if we just tried to check evt.metaKey and not keep track of more state: * The mouse is down when the extension starts so the first event we get is move or up * The user clicks and starts dragging and then presses Search https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:128: * @param {!AutomationEvent} evt On 2017/05/07 06:10:27, David Tseng wrote: > This is a DOM event listener, not an automation event listener Done. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:129: */ On 2017/05/07 06:10:27, David Tseng wrote: > @return Done. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:136: // Walk up to the nearest window, web area, or dialog that the On 2017/05/07 06:10:28, David Tseng wrote: > Might as well include toolbar as well. Done. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (right): https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:75: // A MOUSE_CANCELED accessibility event is fired if the user releases On 2017/05/07 06:10:27, David Tseng wrote: > Would be great to also handle key events in this extension; it's still looking > to be split in ways that a new reader would find confusing. OK, done. The event handler now forwards all keyboard events too, and the extension now handles canceling via releasing the Search key after clicking, or tapping and releasing Search or Control. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:108: * @param {!Event} evt The DOM event On 2017/05/07 06:10:27, David Tseng wrote: > Dom event listeners have a return value (i.e. add @return). Done. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:126: * @param {!Event} evt The DOM event On 2017/05/07 06:10:27, David Tseng wrote: > Ditto, @return Done. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:159: // onMouseReleasedHitTest_. On 2017/05/07 06:10:27, David Tseng wrote: > Just wondering, but it seems confusing to have a mouse released event in the > accessibility enums. Can we use some other event here and remove the mouse > related events from accessibility? I agree, this is confusing. I think we should get rid of the mouse events from accessibility, but there's no other obvious event to return. How about as a follow-up I delete all of the mouse events and replace them with a HIT_TEST_RESPONSE event? https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:164: * Called in response to our hit test afterthe mouse is released, On 2017/05/07 06:10:27, David Tseng wrote: > nit: after the Done. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:170: onMouseReleasedHitTest_: function(evt) { On 2017/05/07 06:10:27, David Tseng wrote: > nit: Maybe rename the listeners like > onAutomationFooEvent and > onMouseFooEvent Done. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:196: document.addEventListener('mousedown', this.onMouseDown_.bind(this)); On 2017/05/07 06:10:27, David Tseng wrote: > Do these listeners need to be removed at some point or does the reader need to > know that mouse events don't get sent when some things occur in the native event > handler? We're tracking keys in the extension now, as suggested, so it doesn't matter whether the event handler sends all keys or not. But either way I don't see any reason to remove the event listeners. It's simpler to just return if we don't care about some events, rather than constantly adding and removing listeners. https://codereview.chromium.org/2814213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:198: document.addEventListener('mousemove', this.onMouseMove_.bind(this)); On 2017/05/07 06:10:27, David Tseng wrote: > nit: sort Done.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Description was changed from ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can just forward the mouse event to the extension background page, and the extension can use the automation API to do a hit test. This is mostly a refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. BUG=699617 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can mouse and keyboard events to the extension background page, and the extension can use the automation API to do a hit test. This is mostly a refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. BUG=699617 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping - I know you're not feeling well so no rush, but whenever you do have time this is ready for a look.
lgtm https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc (left): https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:75: state_ = INACTIVE; Almost there. Can we move all of this state logic into the extension as well? We would always cancel events by default and re-inject if needed. https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (left): https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:288: uiLocale = uiLocale.replace('_', '-').toLowerCase(); For future cl. An extjs test would be appropriate for select to speak to get some coverage for specific html snippets similar to the mock ffedback style of tests in ChromeVox. https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (right): https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:62: this.searchKeyDown_ = false; nit: isSearchKeyDown_ https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:64: /** @private { Set } */ nit: !Set<number> https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:125: return false; return true https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:149: return false; return true https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:266: * Set up event listeners for mouse ane keyboard events. These are s/ane/and
https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc (left): https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:75: state_ = INACTIVE; On 2017/05/16 15:54:39, David Tseng wrote: > Almost there. Can we move all of this state logic into the extension as well? We > would always cancel events by default and re-inject if needed. As discussed, I'd like to avoid re-injecting due to problems we experienced doing that with ChromeVox, and also due to the added latency, since I'd like S2S to not add any overhead when not explicitly used. https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (left): https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:288: uiLocale = uiLocale.replace('_', '-').toLowerCase(); On 2017/05/16 15:54:40, David Tseng wrote: > For future cl. An extjs test would be appropriate for select to speak to get > some coverage for specific html snippets similar to the mock ffedback style of > tests in ChromeVox. Sounds good https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js (right): https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:62: this.searchKeyDown_ = false; On 2017/05/16 15:54:39, David Tseng wrote: > nit: isSearchKeyDown_ Done. https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:64: /** @private { Set } */ On 2017/05/16 15:54:42, David Tseng wrote: > nit: !Set<number> Done. https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:125: return false; On 2017/05/16 15:54:43, David Tseng wrote: > return true Leaving as false since we're not reinjecting https://codereview.chromium.org/2814213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js:266: * Set up event listeners for mouse ane keyboard events. These are On 2017/05/16 15:54:41, David Tseng wrote: > s/ane/and Done.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org Link to the patchset: https://codereview.chromium.org/2814213002/#ps100001 (title: "Address feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org Link to the patchset: https://codereview.chromium.org/2814213002/#ps120001 (title: "closure")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494995496524180,
"parent_rev": "8e3651c7bd1d02a6954068dcf6c9d09313e7eeb6", "commit_rev":
"ccb40f537fbbc17772e7255a7b575661ce4353cd"}
Message was sent while issue was closed.
Description was changed from ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can mouse and keyboard events to the extension background page, and the extension can use the automation API to do a hit test. This is mostly a refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. BUG=699617 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refactor Select-to-speak so that mouse events are forwarded to the extension. Previously the Select-to-speak event handler captured mouse events and turned them into accessibility events that could be caught by select-to-speak. Instead, the event handler can mouse and keyboard events to the extension background page, and the extension can use the automation API to do a hit test. This is mostly a refactoring change, but it gives us some more flexibility and makes the event handler simpler, possibly something that could be general-purpose, while putting more of the logic into the extension code. BUG=699617 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2814213002 Cr-Commit-Position: refs/heads/master@{#472339} Committed: https://chromium.googlesource.com/chromium/src/+/ccb40f537fbbc17772e7255a7b57... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ccb40f537fbbc17772e7255a7b57...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 472339 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2883283003/ by kolos@chromium.org. The reason for reverting is: FindIt detected this CL as culprit for test failures. https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.c....
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2892463003/ by kolos@chromium.org. The reason for reverting is: FindIT detected it as culprit of test failures. https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.c.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
