Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2387353004:
Delay Input.dispatchKeyEvent response until after key event ack. (Closed)
|
Created:
4 years, 2 months ago by samuong Modified:
4 years ago CC:
chromium-reviews, caseq+blink_chromium.org, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, darin-cc_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org, Alexei Svitkine (slow), Charlie Harrison Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelay Input.dispatchKeyEvent response until after key event ack.
BUG=chromedriver:1506
Committed: https://crrev.com/235dd76ed8ea15f62aa1f726d5d451bfe0e96fc4
Cr-Commit-Position: refs/heads/master@{#437706}
Patch Set 1 #Patch Set 2 : add flag to distinguish synthetic key events from user events, and add InputEventObserver::OnKeyboa… #Patch Set 3 : OnKeyboardEventAck -> OnInputEventAck #Patch Set 4 : rebase, make sure that tests build and run #
Total comments: 6
Patch Set 5 : fix some review comments, fix a bug #
Total comments: 6
Patch Set 6 : add test, address review comments about docs #
Total comments: 1
Patch Set 7 : remove |is_synthetic|, fixed bug for mouse acks, add test for mouse events #
Total comments: 4
Patch Set 8 : fix nits #
Total comments: 8
Patch Set 9 : address some review comments #Patch Set 10 : add call to InputHandler::Detached #Patch Set 11 : fix test #
Total comments: 8
Patch Set 12 : fix nits #Patch Set 13 : provide default implementation for InputEventObserver methods #Patch Set 14 : remove another unneccesary OnInputEventAck override #Patch Set 15 : rebase, make InputHandler::host_ a weak pointer instead of a raw pointer #Patch Set 16 : undo weakptr change, rebase to pull in dgozman's fix (crrev.com/437658) #Messages
Total messages: 52 (17 generated)
samuong@chromium.org changed reviewers: + dtapuska@chromium.org, tdresser@chromium.org
As discussed, I've added a bool NativeWebKeyboardEvent::is_synthetic and InputEventObserver::OnInputEventAck, and make InputHandler an InputEventObserver. PTAL?
Can we add a test for this? https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:141: bool was_synthetic) { We appear to switch between is_synthetic and was_synthetic. Let's use is_synthetic throughout? https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:143: DCHECK(!pending_key_event_ids_.empty()); A compromised renderer could send bogus acks, and they shouldn't make us crash. We should probably have an early return here if there are no pending key event ids. https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.h (right): https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... content/browser/devtools/protocol/input_handler.h:137: std::queue<DevToolsCommandId> pending_key_event_ids_; Can you add a comment here explaining what we're doing?
While retesting this in Forge, I did notice that I introduced a regression in ps2 (which is fixed in ps5). For testing, SyntheticKeyEventTest.KeyEventSynthesizeKey should test that Input.dispatchKeyEvent works. But if we want to write a test to delay an input ack, and assert that this also delays the command response, what is the best way to delay the input ack in a browser test? Also, I'm not sure about the lifetime of the InputHandler in relation to the RWHI. If it's possible for the InputHandler to be destroyed while we're waiting for an ack, then RWHI could dereference a bad pointer to InputHandler and crash, so we really should be passing a WeakPtr like we do when queuing synthetic gestures. Perhaps we should change RWHI::AddInputEventObserver to accept a WeakPtr rather than a raw pointer? https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:141: bool was_synthetic) { On 2016/10/12 19:49:36, tdresser wrote: > We appear to switch between is_synthetic and was_synthetic. > > Let's use is_synthetic throughout? Done. https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:143: DCHECK(!pending_key_event_ids_.empty()); On 2016/10/12 19:49:36, tdresser wrote: > A compromised renderer could send bogus acks, and they shouldn't make us crash. > We should probably have an early return here if there are no pending key event > ids. Done. That's a good point, I've added this to the if condition so we won't do anything if we receive an ack while the queue is empty. https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.h (right): https://codereview.chromium.org/2387353004/diff/60001/content/browser/devtool... content/browser/devtools/protocol/input_handler.h:137: std::queue<DevToolsCommandId> pending_key_event_ids_; On 2016/10/12 19:49:36, tdresser wrote: > Can you add a comment here explaining what we're doing? Done.
https://codereview.chromium.org/2387353004/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/80001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:228: host_->AddInputEventObserver(this); So is it possible to call this API twice in a row? and then AddInputEventObserver will get called twice and probably will DCHECK no? Should this remove itself as an observer if it is getting destroyed? https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... File content/public/browser/native_web_keyboard_event.h (right): https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... content/public/browser/native_web_keyboard_event.h:66: bool is_synthetic; Docs? https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... content/public/browser/render_widget_host.h:249: virtual void OnInputEventAck(const blink::WebInputEvent&, bool) = 0; I think you need to document what this boolean represents. (synthetic can also mean a lot of things).
On 2016/10/14 19:06:29, dtapuska wrote: > https://codereview.chromium.org/2387353004/diff/80001/content/browser/devtool... > File content/browser/devtools/protocol/input_handler.cc (right): > > https://codereview.chromium.org/2387353004/diff/80001/content/browser/devtool... > content/browser/devtools/protocol/input_handler.cc:228: > host_->AddInputEventObserver(this); > So is it possible to call this API twice in a row? and then > AddInputEventObserver will get called twice and probably will DCHECK no? > > Should this remove itself as an observer if it is getting destroyed? > > https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... > File content/public/browser/native_web_keyboard_event.h (right): > > https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... > content/public/browser/native_web_keyboard_event.h:66: bool is_synthetic; > Docs? > > https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... > File content/public/browser/render_widget_host.h (right): > > https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... > content/public/browser/render_widget_host.h:249: virtual void > OnInputEventAck(const blink::WebInputEvent&, bool) = 0; > I think you need to document what this boolean represents. (synthetic can also > mean a lot of things). To delay an input ack. You can just spin inside an js event handler. The ack won't get sent until it is done the JS. It appears to me that the InputHandler is built off of the RFHI. So being that RFHI outlives RWHI then the lifetime of a RFHI. You probably should clear any state when a RWHI swaps out via the InputHandler callback SetRenderWidgetHost.
Sorry I got sidetracked with some other high-priority bugs, and only got back to this today. I've added a test for key events that delays the ack by spinning in the event handler. I don't really like the way it has to wait 5 seconds, but I don't see a better way to do it. One thing I tried was putting up a JS dialog, in the hopes that it would block the renderer, but it seems that the window.alert function isn't available in content_shell :( And you're right about the InputHandler ownership, I've changed the way we add and remove the InputEventObserver to reflect this. https://codereview.chromium.org/2387353004/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/80001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:228: host_->AddInputEventObserver(this); On 2016/10/14 19:06:29, dtapuska wrote: > So is it possible to call this API twice in a row? and then > AddInputEventObserver will get called twice and probably will DCHECK no? > > Should this remove itself as an observer if it is getting destroyed? The RWHI will check if the observer has already been added, and won't add the same observer twice. It's not a DCHECK so it won't crash. Anyway this is a moot point, since I'm now adding this in InputHandler::SetRenderWidgetHost as you suggested elsewhere. https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... File content/public/browser/native_web_keyboard_event.h (right): https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... content/public/browser/native_web_keyboard_event.h:66: bool is_synthetic; On 2016/10/14 19:06:29, dtapuska wrote: > Docs? Done. https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/2387353004/diff/80001/content/public/browser/... content/public/browser/render_widget_host.h:249: virtual void OnInputEventAck(const blink::WebInputEvent&, bool) = 0; On 2016/10/14 19:06:29, dtapuska wrote: > I think you need to document what this boolean represents. (synthetic can also > mean a lot of things). Done. https://codereview.chromium.org/2387353004/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2387353004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1981: OnInputEventAck(mouse_event.event, true)); Right now I'm setting is_synthetic to true for *all* mouse events. Unlike key events, there is no content-level NativeWebMouseEvent, only the blink-level WebMouseEvent. So there's a few options here: 1. add blink::WebMouseEvent::is_synthetic 2. create content::NativeWebMouseEvent and add an is_synthetic property 3. forget about this, since we don't really support the use case where we mix synthetic and user inputs anyway Can you let me know what you think here?
I've removed the is_synthetic property, since I don't have a good way to do this for mouse events. I've also added a test for dispatching mouse events. PTAL?
lgtm % nits https://codereview.chromium.org/2387353004/diff/120001/content/browser/devtoo... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/input_handler.cc:169: host->AddInputEventObserver(this); I'd expect host_ = host before the observer was added. In case for some reason AddInputEventObserver calls back into this object (I know it doesn't exist today) host_ would not be setup properly. https://codereview.chromium.org/2387353004/diff/120001/content/public/test/br... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/2387353004/diff/120001/content/public/test/br... content/public/test/browser_test_utils.h:554: bool HasReceivedAck(); should be const
samuong@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman@ for devtools changes https://codereview.chromium.org/2387353004/diff/120001/content/browser/devtoo... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/input_handler.cc:169: host->AddInputEventObserver(this); On 2016/11/21 14:49:44, dtapuska wrote: > I'd expect host_ = host before the observer was added. In case for some reason > AddInputEventObserver calls back into this object (I know it doesn't exist > today) host_ would not be setup properly. Done. https://codereview.chromium.org/2387353004/diff/120001/content/public/test/br... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/2387353004/diff/120001/content/public/test/br... content/public/test/browser_test_utils.h:554: bool HasReceivedAck(); On 2016/11/21 14:49:45, dtapuska wrote: > should be const Done.
Looks very good! https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/input_handler.cc:158: for (const DevToolsCommandId& command_id : pending_key_command_ids_) You'll need to do the same on detach. See Detached() method in other domain handlers. https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/input_handler.h (right): https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/input_handler.h:35: void OnInputEvent(const blink::WebInputEvent& event) override; We can probably move these to private. https://codereview.chromium.org/2387353004/diff/140001/content/test/data/devt... File content/test/data/devtools/input_ack_tests.html (right): https://codereview.chromium.org/2387353004/diff/140001/content/test/data/devt... content/test/data/devtools/input_ack_tests.html:12: // spin for 5 seconds to block the key event ack This will be flaky for sure :) Do we really need it? Let's just verify that we don't get result synchronously, then wait and get the result?
https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/input_handler.cc:158: for (const DevToolsCommandId& command_id : pending_key_command_ids_) On 2016/11/21 19:14:39, dgozman wrote: > You'll need to do the same on detach. See Detached() method in other domain > handlers. I've added an InputHandler::Detached method, but there's no superclass Detached method that I'm overriding, so I'm not really clear on how this gets called. Does the call come from generated code somewhere? https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/input_handler.h (right): https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/input_handler.h:35: void OnInputEvent(const blink::WebInputEvent& event) override; On 2016/11/21 19:14:39, dgozman wrote: > We can probably move these to private. Done. https://codereview.chromium.org/2387353004/diff/140001/content/test/data/devt... File content/test/data/devtools/input_ack_tests.html (right): https://codereview.chromium.org/2387353004/diff/140001/content/test/data/devt... content/test/data/devtools/input_ack_tests.html:12: // spin for 5 seconds to block the key event ack On 2016/11/21 19:14:39, dgozman wrote: > This will be flaky for sure :) Do we really need it? Let's just verify that we > don't get result synchronously, then wait and get the result? What's the best way to verify that we don't get the result synchronously? In the KeyboardEventAck, I could do something like: SendKeyEvent("rawKeyDown", 0, 13, 13, "Enter", false); ASSERT_FALSE(filter->HasReceivedAck()); But I'm not really sure the assertion does anything useful - we never ran the run loop, so we won't see the ack even if it did come in early? I could remove the assertion, but then the test starts to look a lot like the existing KeyEventSynthesizeKey test. Ideally there'd be a way to cause the JS event handler to block. I wanted to use alert() to do this, but it looks like this isn't implemented in content shell. Are there any other ways to do this?
> I've added an InputHandler::Detached method, but there's no superclass
Detached
> method that I'm overriding, so I'm not really clear on how this gets called.
> Does the call come from generated code somewhere?
Look at RenderFrameDevToolsAgentHost::OnClientDetached.
> > This will be flaky for sure :) Do we really need it? Let's just verify that
we
> > don't get result synchronously, then wait and get the result?
>
> What's the best way to verify that we don't get the result synchronously? In
the
> KeyboardEventAck, I could do something like:
>
> SendKeyEvent("rawKeyDown", 0, 13, 13, "Enter", false);
> ASSERT_FALSE(filter->HasReceivedAck());
>
> But I'm not really sure the assertion does anything useful - we never ran the
> run loop, so we won't see the ack even if it did come in early?
>
> I could remove the assertion, but then the test starts to look a lot like the
> existing KeyEventSynthesizeKey test.
>
> Ideally there'd be a way to cause the JS event handler to block. I wanted to
use
> alert() to do this, but it looks like this isn't implemented in content shell.
> Are there any other ways to do this?
The easiest way would be a layout test which sends an event, does console.log
inside the handler and verifies that console.log notification comes before the
event response. Does this work for you?
https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/2387353004/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/input_handler.cc:158: for (const DevToolsCommandId& command_id : pending_key_command_ids_) On 2016/11/21 22:28:43, samuong wrote: > On 2016/11/21 19:14:39, dgozman wrote: > > You'll need to do the same on detach. See Detached() method in other domain > > handlers. > > I've added an InputHandler::Detached method, but there's no superclass Detached > method that I'm overriding, so I'm not really clear on how this gets called. > Does the call come from generated code somewhere? Done - added a call from RFDTAH::OnClientDetached https://codereview.chromium.org/2387353004/diff/140001/content/test/data/devt... File content/test/data/devtools/input_ack_tests.html (right): https://codereview.chromium.org/2387353004/diff/140001/content/test/data/devt... content/test/data/devtools/input_ack_tests.html:12: // spin for 5 seconds to block the key event ack On 2016/11/21 22:28:43, samuong wrote: > On 2016/11/21 19:14:39, dgozman wrote: > > This will be flaky for sure :) Do we really need it? Let's just verify that we > > don't get result synchronously, then wait and get the result? > > What's the best way to verify that we don't get the result synchronously? In the > KeyboardEventAck, I could do something like: > > SendKeyEvent("rawKeyDown", 0, 13, 13, "Enter", false); > ASSERT_FALSE(filter->HasReceivedAck()); > > But I'm not really sure the assertion does anything useful - we never ran the > run loop, so we won't see the ack even if it did come in early? > > I could remove the assertion, but then the test starts to look a lot like the > existing KeyEventSynthesizeKey test. > > Ideally there'd be a way to cause the JS event handler to block. I wanted to use > alert() to do this, but it looks like this isn't implemented in content shell. > Are there any other ways to do this? Since I already had the browser test set up, I've modified it to verify that the console.log notification comes before the ack and the event response, as you suggested. If you want me to convert this to a layout test, let me know.
lgtm https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:583: if (input_handler_) I think it's never null (created in constructor).
LGTM https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:488: // event ack, adn the subsequent command response for Input.dispatchKeyEvent. adn -> and https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:514: // event ack, adn the subsequent command response for Input.dispatchKeyEvent. adn -> and https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:583: if (input_handler_) On 2016/11/23 02:03:53, dgozman wrote: > I think it's never null (created in constructor). Looks like you're right. I'm not sure why this is a unique_ptr, instead of being stack allocated.
samuong@chromium.org changed reviewers: + mpearson@chromium.org, pfeldman@chromium.org
+mpearson@ for: chrome/browser/metrics/desktop_session_duration/desktop_session_duration_observer.h +csharrison@ for: chrome/browser/page_load_metrics/metrics_web_contents_observer.h +pfeldman@ for: content/browser/site_per_process_browsertest.cc content/public/browser/render_widget_host.h content/public/test/browser_test_utils.cc content/public/test/browser_test_utils.h https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:488: // event ack, adn the subsequent command response for Input.dispatchKeyEvent. On 2016/11/23 13:38:13, tdresser wrote: > adn -> and Done. https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:514: // event ack, adn the subsequent command response for Input.dispatchKeyEvent. On 2016/11/23 13:38:13, tdresser wrote: > adn -> and Done. Also changed Key -> Mouse. https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:583: if (input_handler_) On 2016/11/23 13:38:13, tdresser wrote: > On 2016/11/23 02:03:53, dgozman wrote: > > I think it's never null (created in constructor). > > Looks like you're right. I'm not sure why this is a unique_ptr, instead of being > stack allocated. Done. I removed the "if (input_handler_)", but I noticed that it is checked unnecessarily in a few other places too. Let me know if you want me to remove these and I can submit it in a follow-up CL.
https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2387353004/diff/200001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:583: if (input_handler_) On 2016/11/23 18:40:48, samuong wrote: > On 2016/11/23 13:38:13, tdresser wrote: > > On 2016/11/23 02:03:53, dgozman wrote: > > > I think it's never null (created in constructor). > > > > Looks like you're right. I'm not sure why this is a unique_ptr, instead of > being > > stack allocated. > > Done. I removed the "if (input_handler_)", but I noticed that it is checked > unnecessarily in a few other places too. Let me know if you want me to remove > these and I can submit it in a follow-up CL. I'd be happy to see us stack allocated input_handler_ in a followup CL.
On 2016/11/23 18:40:48, samuong wrote: > +mpearson@ for: > chrome/browser/metrics/desktop_session_duration/desktop_session_duration_observer.h I am not familiar with this code. Please look through the file history and find a different metrics team owner. thanks, mark
samuong@chromium.org changed reviewers: + asvitkine@chromium.org, csharrison@chromium.org - mpearson@chromium.org
-mpearson@ +asvitkine@, are you able to do an owner review for desktop_session_duration_observer.h? I've added a new method to the InputEventObserver interface so I've had to create an empty method in DesktopSessionDurationObserver. There are no changes to the existing functionality. csharrison@, same goes for MetricsWebContentsObserver - there are no changes to the existing functionality for that class.
Why not have the base class provide the empty implementation so you don't need to change the current implementors?
+1 to asvitkine. FYI I will be OOO from about now to Monday, so you may need to choose another reviewer.
samuong@chromium.org changed reviewers: - asvitkine@chromium.org, csharrison@chromium.org
OK, I've added a default empty implementation for InputEventObserver::OnInputEventAck. For consistency, I've done the same for the existing OnInputEvent method, Dave and/or Tim are you still OK with this?
lgtm
The CQ bit was checked by samuong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, dgozman@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2387353004/#ps260001 (title: "remove another unneccesary OnInputEventAck override")
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Still LGTM.
The CQ bit was checked by samuong@chromium.org
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samuong@chromium.org
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The try job failures (which unfortunately don't reproduce on my local machine) happen because the test binary crashes when InputHandler::SetRenderWidgetHost calls |host_->RemoveInputEventObserver(this)|. Changing |host_| to a weak pointer allows us to avoid calling this function if the RWHI pointer becomes invalidated, but I'm not sure why this is happening in the first place (or why it only repros on the try bot). Dmitry or Pavel, is this a reasonable fix, or should we always expect host_ to be valid when SetRenderWidgetHost is called?
The CQ bit was checked by samuong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, dgozman@chromium.org, tdresser@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2387353004/#ps300001 (title: "rebase to pull in dgozman's fix (crrev.com/437658)")
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": 300001, "attempt_start_ts": 1481320661752720,
"parent_rev": "63273f431d335e1c6b59d99e70c1849f046f05f2", "commit_rev":
"5f509059d2c91904b80ac9475cff9225128238a5"}
Message was sent while issue was closed.
Description was changed from ========== Delay Input.dispatchKeyEvent response until after key event ack. BUG=chromedriver:1506 ========== to ========== Delay Input.dispatchKeyEvent response until after key event ack. BUG=chromedriver:1506 Review-Url: https://codereview.chromium.org/2387353004 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/2565313002/ by gab@chromium.org. The reason for reverting is: Tentative revert for: Failed steps failed chrome_public_test_apk on android failed content_browsertests on android https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... as of this CL (and 2 others which are mere renames + don't touch content so suspecting this one from the 3: https://chromium.googlesource.com/chromium/src/+log/b671a5234f41f1784bd261a7c...) No logs from failures on Android bot unfortunately :-(... Don't understand why it would have passed CQ and not waterfall but still can't see what else in range could be at fault, sorry...
Message was sent while issue was closed.
Description was changed from ========== Delay Input.dispatchKeyEvent response until after key event ack. BUG=chromedriver:1506 Review-Url: https://codereview.chromium.org/2387353004 ========== to ========== Delay Input.dispatchKeyEvent response until after key event ack. BUG=chromedriver:1506 Committed: https://crrev.com/235dd76ed8ea15f62aa1f726d5d451bfe0e96fc4 Cr-Commit-Position: refs/heads/master@{#437706} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/235dd76ed8ea15f62aa1f726d5d451bfe0e96fc4 Cr-Commit-Position: refs/heads/master@{#437706} |
