Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6630001:
Allow webdriver users to choose between sending the key events when... (Closed)
|
Created:
9 years, 9 months ago by timothe faudot Modified:
9 years, 3 months ago Reviewers:
timothe, Hironori Bono, dominicc1, kkania, commit-bot: I haz the power, Paweł Hajdan Jr., Jason Leyba CC:
chromium-reviews, Eran M. (Google) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAllow webdriver users to choose between sending the key events when
using send_keys to the window of the browser or to webkit directly as it was
done before.
BUG=74899
TEST=run chromedriver_tests.py using the new 2.0b3 version python drivers.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80000
Patch Set 1 #
Total comments: 12
Patch Set 2 : '' #
Total comments: 9
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 6
Patch Set 6 : '' #
Total comments: 4
Patch Set 7 : '' #
Total comments: 10
Patch Set 8 : '' #
Total comments: 22
Patch Set 9 : '' #
Total comments: 16
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 27
Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Total comments: 24
Patch Set 16 : '' #
Total comments: 4
Patch Set 17 : '' #Patch Set 18 : '' #Patch Set 19 : '' #
Total comments: 4
Patch Set 20 : '' #Patch Set 21 : '' #Messages
Total messages: 48 (0 generated)
Hello everyone, here's the patch that allows users to switch between native and webkit events when using the new chromedriver.
Greetings Tim, Thank you for your change. I have added some style nits. Regards, Hironori Bono http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4664: && use_native_events) { nit: This operator should be the end of the previous line. <http://dev.chromium.org/developers/coding-style> http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4671: if (browser_window == NULL) { nit: browser_window == NULL -> !browser_window http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4677: if (window == NULL) { nit: window == NULL -> !window http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4685: WebKit::WebInputEvent::ControlKey), nit: I prefer using a local variable as listed in the following snippet. bool control = !!(modifiers & automation::kControlKeyMask); bool shift = !!(modifiers & automation::kShiftKeyMask); bool alt = !!(modifiers & automation::kShiftKeyMask); bool meta = !!(modifiers & automation::kShiftKeyMask); if (!ui_controls::SendKeyPress(window, event.nativeKeyCode, control, shift, alt, meta)) { ... } http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4707: ForwardKeyboardEvent(event); nit: I prefer using an early exit as listed in the following snippet. if (!args->GetBoolean("useNativeEvents", &use_native_events) || !use_native_events) { new InputEventAckNotificationObserver(this, reply_message, event.type); browser->GetSelectedTabContents()->render_view_host()-> ForwardKeyboardEvent(event); return; } http://codereview.chromium.org/6630001/diff/1/chrome/test/webdriver/automation.h File chrome/test/webdriver/automation.h (right): http://codereview.chromium.org/6630001/diff/1/chrome/test/webdriver/automatio... chrome/test/webdriver/automation.h:73: bool use_native_events, bool* success); nit: all parameters should be aligned if possible. <http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...>
http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4664: && use_native_events) { On 2011/03/04 06:25:17, hbono wrote: > nit: This operator should be the end of the previous line. > <http://dev.chromium.org/developers/coding-style> Done. http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4671: if (browser_window == NULL) { On 2011/03/04 06:25:17, hbono wrote: > nit: browser_window == NULL -> !browser_window Done. http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4677: if (window == NULL) { On 2011/03/04 06:25:17, hbono wrote: > nit: window == NULL -> !window Done. http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4685: WebKit::WebInputEvent::ControlKey), On 2011/03/04 06:25:17, hbono wrote: > nit: I prefer using a local variable as listed in the following snippet. > bool control = !!(modifiers & automation::kControlKeyMask); > bool shift = !!(modifiers & automation::kShiftKeyMask); > bool alt = !!(modifiers & automation::kShiftKeyMask); > bool meta = !!(modifiers & automation::kShiftKeyMask); > if (!ui_controls::SendKeyPress(window, event.nativeKeyCode, control, > shift, alt, meta)) { > ... > } Done. http://codereview.chromium.org/6630001/diff/1/chrome/browser/automation/testi... chrome/browser/automation/testing_automation_provider.cc:4707: ForwardKeyboardEvent(event); On 2011/03/04 06:25:17, hbono wrote: > nit: I prefer using an early exit as listed in the following snippet. > if (!args->GetBoolean("useNativeEvents", &use_native_events) || > !use_native_events) { > new InputEventAckNotificationObserver(this, reply_message, event.type); > browser->GetSelectedTabContents()->render_view_host()-> > ForwardKeyboardEvent(event); > return; > } Down, also added an early return statement up. http://codereview.chromium.org/6630001/diff/1/chrome/test/webdriver/automation.h File chrome/test/webdriver/automation.h (right): http://codereview.chromium.org/6630001/diff/1/chrome/test/webdriver/automatio... chrome/test/webdriver/automation.h:73: bool use_native_events, bool* success); On 2011/03/04 06:25:17, hbono wrote: > nit: all parameters should be aligned if possible. > <http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...> Done.
I would prefer native events be enabled on a per-session basis. This way users could run both types of tests without having to restart the server. I'm also unclear on what ui_controls::SendKeyPress is actually doing. Is it generating an OS-level event that any window could respond to? The chromedriver supports concurrent sessions. What happens if you send key presses to one session, but another session's window is active? http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4660: // ui_controls::endKeyPress to the native window handle of the browser, ui_controls::SendKeyPress http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:122: driver = WebDriver(launcher.GetURL(), 'chrome', 'any') You're not verifying that the server has actually started with native events enabled. http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... File chrome/test/webdriver/session.cc (right): http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:167: scoped_ptr<ListValue> nullArgs(new ListValue); This would be much simpler if you did it all with one ExecuteScript call if (document.activeElement != arguments[0]) { if (document.activeElement) document.activeElement.blur(); arguments[0].focus(); }
Drive-by with automation-related comments. http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4659: // If useNativeEvents is true, this funciton will call I'd strongly prefer to standardize on one way we implement this, and not leave it as an option for the client. By the way, why are the native events needed at all? I have read the bug, and why not just send the Chinese/Japanese/Korean chars directly to WebKit? My understanding is that ChromeDriver was not intended to test IME software... And of course the native events are really going to be more flaky.
On 2011/03/05 11:55:51, Paweł Hajdan Jr. wrote: > Drive-by with automation-related comments. > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > File chrome/browser/automation/testing_automation_provider.cc (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > chrome/browser/automation/testing_automation_provider.cc:4659: // If > useNativeEvents is true, this funciton will call > I'd strongly prefer to standardize on one way we implement this, and not leave > it as an option for the client. > > By the way, why are the native events needed at all? I have read the bug, and > why not just send the Chinese/Japanese/Korean chars directly to WebKit? My > understanding is that ChromeDriver was not intended to test IME software... > > And of course the native events are really going to be more flaky. I updated the bug page to explain why we need this functionality. To answer more precisely here: We do not test IME software themselves, we test the behavior of our web pages when confronted with those software. No one has really soved this problem yet, and being able to test the behavior of our apps in a key-per-key basis will make testers life easier. Also for the standardization, if not an option to this method call as implemented now what could be the alternatives? a new method in the TestingAutomationProvider::SendNativeKeyEventToActiveTab? I'm okay with both personally. Note that as I made the flag false by default this will not influence the behavior of previously wrote scripts which will still use webkit events.
On 2011/03/04 19:25:12, Jason Leyba wrote: > I would prefer native events be enabled on a per-session basis. This way users > could run both types of tests without having to restart the server. > > I'm also unclear on what ui_controls::SendKeyPress is actually doing. Is it > generating an OS-level event that any window could respond to? The chromedriver > supports concurrent sessions. What happens if you send key presses to one > session, but another session's window is active? > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > File chrome/browser/automation/testing_automation_provider.cc (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > chrome/browser/automation/testing_automation_provider.cc:4660: // > ui_controls::endKeyPress to the native window handle of the browser, > ui_controls::SendKeyPress > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > File chrome/test/webdriver/chromedriver_tests.py (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > chrome/test/webdriver/chromedriver_tests.py:122: driver = > WebDriver(launcher.GetURL(), 'chrome', 'any') > You're not verifying that the server has actually started with native events > enabled. > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... > File chrome/test/webdriver/session.cc (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... > chrome/test/webdriver/session.cc:167: scoped_ptr<ListValue> nullArgs(new > ListValue); > This would be much simpler if you did it all with one ExecuteScript call > > if (document.activeElement != arguments[0]) { > if (document.activeElement) > document.activeElement.blur(); > arguments[0].focus(); > } IME software do not support concurrent editing as humans never edit two text boxes at once, if the focus of an input widget is lost, the IME will stop monitoring changes in this widget. That said, providing support for concurrent sessions while testing with native input is not a must-have in my opinion. Nonetheless this is something that the user should be aware of, we should put a note that concurrent text input concurrently is not supported somewhere in the docs or wiki.
On 2011/03/04 19:25:12, Jason Leyba wrote: > I would prefer native events be enabled on a per-session basis. This way users > could run both types of tests without having to restart the server. > > I'm also unclear on what ui_controls::SendKeyPress is actually doing. Is it > generating an OS-level event that any window could respond to? The chromedriver > supports concurrent sessions. What happens if you send key presses to one > session, but another session's window is active? > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > File chrome/browser/automation/testing_automation_provider.cc (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > chrome/browser/automation/testing_automation_provider.cc:4660: // > ui_controls::endKeyPress to the native window handle of the browser, > ui_controls::SendKeyPress > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > File chrome/test/webdriver/chromedriver_tests.py (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > chrome/test/webdriver/chromedriver_tests.py:122: driver = > WebDriver(launcher.GetURL(), 'chrome', 'any') > You're not verifying that the server has actually started with native events > enabled. > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... > File chrome/test/webdriver/session.cc (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... > chrome/test/webdriver/session.cc:167: scoped_ptr<ListValue> nullArgs(new > ListValue); > This would be much simpler if you did it all with one ExecuteScript call > > if (document.activeElement != arguments[0]) { > if (document.activeElement) > document.activeElement.blur(); > arguments[0].focus(); > } As for enabling native events on a per-session basis, if we see how firefox handles this for the moment it is a flag in a profile, that sounds reasonable to me but we don't have a ChromeProfile yet. Another option would be a parameter to the create_session command but that would change the wire so I am not particularly willing to do it. Do you see any other way of enabling it per session without having to restart the server?
http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4659: // If useNativeEvents is true, this funciton will call On 2011/03/05 11:55:51, Paweł Hajdan Jr. wrote: > I'd strongly prefer to standardize on one way we implement this, and not leave > it as an option for the client. > > By the way, why are the native events needed at all? I have read the bug, and > why not just send the Chinese/Japanese/Korean chars directly to WebKit? My > understanding is that ChromeDriver was not intended to test IME software... > > And of course the native events are really going to be more flaky. I added some justifications in the associated bug http://code.google.com/p/chromium/issues/detail?id=74899 http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4660: // ui_controls::endKeyPress to the native window handle of the browser, On 2011/03/04 19:25:12, Jason Leyba wrote: > ui_controls::SendKeyPress Done. http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:122: driver = WebDriver(launcher.GetURL(), 'chrome', 'any') On 2011/03/04 19:25:12, Jason Leyba wrote: > You're not verifying that the server has actually started with native events > enabled. This test only ensures that we can start the server with the flag --native-events set and that it is stored in the session manager. For the test that send key events it is in webdriver_remote_tests.py/RemoteWebdriverNativeEventsTest http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... File chrome/test/webdriver/session.cc (right): http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:167: scoped_ptr<ListValue> nullArgs(new ListValue); On 2011/03/04 19:25:12, Jason Leyba wrote: > This would be much simpler if you did it all with one ExecuteScript call > > if (document.activeElement != arguments[0]) { > if (document.activeElement) > document.activeElement.blur(); > arguments[0].focus(); > } Done.
On 2011/03/07 02:45:07, timothe faudot wrote: > On 2011/03/04 19:25:12, Jason Leyba wrote: > > I would prefer native events be enabled on a per-session basis. This way > users > > could run both types of tests without having to restart the server. > > > > I'm also unclear on what ui_controls::SendKeyPress is actually doing. Is it > > generating an OS-level event that any window could respond to? The > chromedriver > > supports concurrent sessions. What happens if you send key presses to one > > session, but another session's window is active? > > > > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > > File chrome/browser/automation/testing_automation_provider.cc (right): > > > > > http://codereview.chromium.org/6630001/diff/2002/chrome/browser/automation/te... > > chrome/browser/automation/testing_automation_provider.cc:4660: // > > ui_controls::endKeyPress to the native window handle of the browser, > > ui_controls::SendKeyPress > > > > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > > File chrome/test/webdriver/chromedriver_tests.py (right): > > > > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > > chrome/test/webdriver/chromedriver_tests.py:122: driver = > > WebDriver(launcher.GetURL(), 'chrome', 'any') > > You're not verifying that the server has actually started with native events > > enabled. > > > > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... > > File chrome/test/webdriver/session.cc (right): > > > > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/sessio... > > chrome/test/webdriver/session.cc:167: scoped_ptr<ListValue> nullArgs(new > > ListValue); > > This would be much simpler if you did it all with one ExecuteScript call > > > > if (document.activeElement != arguments[0]) { > > if (document.activeElement) > > document.activeElement.blur(); > > arguments[0].focus(); > > } > > > As for enabling native events on a per-session basis, if we see how firefox > handles this for the moment it is a flag in a profile, that sounds reasonable to > me but we don't have a ChromeProfile yet. > Another option would be a parameter to the create_session command but that would > change the wire so I am not particularly willing to do it. > Do you see any other way of enabling it per session without having to restart > the server? It can go in the requested session capabilities. Use a key like "chrome.nativeEvents".
If you're not going to support concurrent sessions when native events are enabled, you need to explicitly document that somewhere. Perhaps the server should even refuse to create more than one session if native events are enabled? Also, you didn't address my concerns about the ui_controls::SendKeyPress. Will the generated key presses go only to the chrome window, or could they go to another window (e.g. Firefox)? The latter would be inconsistent with how native key events work for the Firefox and IE drivers. http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:122: driver = WebDriver(launcher.GetURL(), 'chrome', 'any') On 2011/03/07 02:45:26, timothe faudot wrote: > On 2011/03/04 19:25:12, Jason Leyba wrote: > > You're not verifying that the server has actually started with native events > > enabled. > > This test only ensures that we can start the server with the flag > --native-events set and that it is stored in the session manager. That's my point - you're only testing that you can start the server with the --native-events flag and create a session. There's nothing about this test that verifies the server did anything with the --native-events flag. > For the test that send key events it is in > webdriver_remote_tests.py/RemoteWebdriverNativeEventsTest I don't see any changes to webdriver_remote_tests in the latest patch.
On 2011/03/07 03:32:56, Jason Leyba wrote: > If you're not going to support concurrent sessions when native events are > enabled, you need to explicitly document that somewhere. Perhaps the server > should even refuse to create more than one session if native events are enabled? > > Also, you didn't address my concerns about the ui_controls::SendKeyPress. Will > the generated key presses go only to the chrome window, or could they go to > another window (e.g. Firefox)? The latter would be inconsistent with how native > key events work for the Firefox and IE drivers. > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > File chrome/test/webdriver/chromedriver_tests.py (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > chrome/test/webdriver/chromedriver_tests.py:122: driver = > WebDriver(launcher.GetURL(), 'chrome', 'any') > On 2011/03/07 02:45:26, timothe faudot wrote: > > On 2011/03/04 19:25:12, Jason Leyba wrote: > > > You're not verifying that the server has actually started with native events > > > enabled. > > > > This test only ensures that we can start the server with the flag > > --native-events set and that it is stored in the session manager. > > That's my point - you're only testing that you can start the server with the > --native-events flag and create a session. There's nothing about this test that > verifies the server did anything with the --native-events flag. > > > For the test that send key events it is in > > webdriver_remote_tests.py/RemoteWebdriverNativeEventsTest > > I don't see any changes to webdriver_remote_tests in the latest patch. Ho yes sorry that file was put accidentally in an older changelist that didn't get included in this one, it is now in the patch set 3. But anyway this is bound to change if I use the requested session capabilities as you proposed, I'll put it on the patch set 4.
On 2011/03/07 03:32:56, Jason Leyba wrote: > If you're not going to support concurrent sessions when native events are > enabled, you need to explicitly document that somewhere. Perhaps the server > should even refuse to create more than one session if native events are enabled? > > Also, you didn't address my concerns about the ui_controls::SendKeyPress. Will > the generated key presses go only to the chrome window, or could they go to > another window (e.g. Firefox)? The latter would be inconsistent with how native > key events work for the Firefox and IE drivers. > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > File chrome/test/webdriver/chromedriver_tests.py (right): > > http://codereview.chromium.org/6630001/diff/2002/chrome/test/webdriver/chrome... > chrome/test/webdriver/chromedriver_tests.py:122: driver = > WebDriver(launcher.GetURL(), 'chrome', 'any') > On 2011/03/07 02:45:26, timothe faudot wrote: > > On 2011/03/04 19:25:12, Jason Leyba wrote: > > > You're not verifying that the server has actually started with native events > > > enabled. > > > > This test only ensures that we can start the server with the flag > > --native-events set and that it is stored in the session manager. > > That's my point - you're only testing that you can start the server with the > --native-events flag and create a session. There's nothing about this test that > verifies the server did anything with the --native-events flag. > > > For the test that send key events it is in > > webdriver_remote_tests.py/RemoteWebdriverNativeEventsTest > > I don't see any changes to webdriver_remote_tests in the latest patch. I use browser->window() to get the native handle so even if other windows have the focus, keys get sent to the right one.
Native events are now enabled on a per session basis using a rediredCapability with the key "chrome.nativeEvents" as suggested by Jason. I am still not sure how to warn a user that request nativeEvents on more than two sessions... Storing a boolean telling if a session with nativeEvents enabled is already running in the SessionManager is one way of doing it but what that works for one driver server running but not for multiple ones running at the same time. http://codereview.chromium.org/6630001/diff/1020/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/1020/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:114: driver = WebDriver(launcher.GetURL(), {}) The new python driver since 2.0b3 correctly handles capabilities, and force them to be specified using a dict. An empty dict will load the default ones, I could also use DesiredCapabilities.CHROME it's a matter of choice. http://codereview.chromium.org/6630001/diff/1020/chrome/test/webdriver/comman... File chrome/test/webdriver/commands/create_session.cc (right): http://codereview.chromium.org/6630001/diff/1020/chrome/test/webdriver/comman... chrome/test/webdriver/commands/create_session.cc:45: session->set_use_native_events(native_events_required); Should I add a parameter to the Session::Init() method instead of calling a special method to assign the value?
BTW, the BUG=chromium-1234 syntax in the commit message is not recognizedby Rietveld. BUG=1234 is. Are you sure you want the previous one? http://codereview.chromium.org/6630001/diff/1020/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/1020/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4658: bool use_native_events = false; As said before, having this switchable seems bad. If we need native events, can we just switch everything to native events? I can easily imagine trying to write code for this interface, and it seems really non-obvious when to use native events or what that means. Similarly, different behavior when "just sending key events" may confuse people trying to debug tests. And it's just 2 pieces of code trying to do a very similar thing. http://codereview.chromium.org/6630001/diff/1020/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4697: // Skip this event by acknowledging it without doing nothing. The reply is indistinguishable from the case where the event was actually processed. Please put some info in the reply message what has really happened. Also, please put the shorter, special case first and return early, to decrease indentation. In other words, instead of: if (e.type == foo) { ... } else { ... return; } just: if (e.type != foo) { ... return; } ...
I also changed the bug ID format to be parsed, thank you for pointing this out. http://codereview.chromium.org/6630001/diff/1020/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/1020/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4658: bool use_native_events = false; On 2011/03/07 20:07:37, Paweł Hajdan Jr. wrote: > As said before, having this switchable seems bad. If we need native events, can > we just switch everything to native events? > > I can easily imagine trying to write code for this interface, and it seems > really non-obvious when to use native events or what that means. Similarly, > different behavior when "just sending key events" may confuse people trying to > debug tests. And it's just 2 pieces of code trying to do a very similar thing. I understand that WebKit events were chosen because they were less flacky than native events. I think we should at least leave the choice to the user, as done in the firefox driver for instance via an option in the profile. I agree that it may be confusing for the user trying to debug tests, but that function should only be user in CJK countries where it is really useful, thus I expect the testers there knowing what they do regarding this matter. Again, I already put 8 lines on comments on the code, I am not a fan of this, code should be in the best case self-explaining, so if there is another way (like separate into another function on the proxy) I am willing to do it for the sake of clarity. http://codereview.chromium.org/6630001/diff/1020/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4697: // Skip this event by acknowledging it without doing nothing. On 2011/03/07 20:07:37, Paweł Hajdan Jr. wrote: > The reply is indistinguishable from the case where the event was actually > processed. Please put some info in the reply message what has really happened. > Also, please put the shorter, special case first and return early, to decrease > indentation. > > In other words, instead of: > > if (e.type == foo) { > ... > } else { > ... > return; > } > > just: > > if (e.type != foo) { > ... > return; > } > > ... Done.
http://codereview.chromium.org/6630001/diff/8004/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/8004/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4664: use_native_events) { Yeah, if you'd like to keep it, please extract the native events logic to a separate method or a function in anonymous namespace. http://codereview.chromium.org/6630001/diff/8004/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4672: Value::CreateStringValue("Skipped non raw key down event")); Reporting this important data via String is fragile. There should be rather a dict with a key "skipped" set to either true or false.
http://codereview.chromium.org/6630001/diff/8004/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/8004/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4664: use_native_events) { On 2011/03/08 07:53:05, Paweł Hajdan Jr. wrote: > Yeah, if you'd like to keep it, please extract the native events logic to a > separate method or a function in anonymous namespace. I separated this into two methods and factorized the common event handling part in another one. http://codereview.chromium.org/6630001/diff/8004/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4672: Value::CreateStringValue("Skipped non raw key down event")); On 2011/03/08 07:53:05, Paweł Hajdan Jr. wrote: > Reporting this important data via String is fragile. There should be rather a > dict with a key "skipped" set to either true or false. Done.
Code I commented in the drive-by LGTM with the following comments. Thank you. http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4671: } nit: Remove the braces {} for a 1-line "if" statement. http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4694: DictionaryValue* was_processed = new DictionaryValue(); nit: |was_processed| is not a good name for the entire dict. How about |reply_details| or |reply_dict|? http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4722: return; nit: This "return" statement seems not needed now. http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4727: return; nit: This "return" statement seems not needed now. http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.h:781: bool BuildNativeWebKeyEventFromArgs(DictionaryValue* args, nit: This should have a brief comment.
http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4671: } On 2011/03/08 09:25:13, Paweł Hajdan Jr. wrote: > nit: Remove the braces {} for a 1-line "if" statement. Done, also changed the same call on SendKeyEventToActiveBrowserWindow http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4694: DictionaryValue* was_processed = new DictionaryValue(); On 2011/03/08 09:25:13, Paweł Hajdan Jr. wrote: > nit: |was_processed| is not a good name for the entire dict. How about > |reply_details| or |reply_dict|? Done. http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4722: return; On 2011/03/08 09:25:13, Paweł Hajdan Jr. wrote: > nit: This "return" statement seems not needed now. Done. http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4727: return; On 2011/03/08 09:25:13, Paweł Hajdan Jr. wrote: > nit: This "return" statement seems not needed now. Done. http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/6630001/diff/3030/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.h:781: bool BuildNativeWebKeyEventFromArgs(DictionaryValue* args, On 2011/03/08 09:25:13, Paweł Hajdan Jr. wrote: > nit: This should have a brief comment. Done.
thanks for doing this! http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4662: void TestingAutomationProvider::SendKeyEventToActiveTab( This function has changed since a couple days ago. Please update your client http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4673: browser->GetSelectedTabContents()->render_view_host()-> 4 space indentation here http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4675: return; remove the extra return http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/automa... File chrome/test/webdriver/automation.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/automa... chrome/test/webdriver/automation.cc:243: void Automation::SendWebKeyEvent(int tab_id, I would prefer creating a separate method here for sending native events, since they call two different commands and should have different arguments. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/automa... chrome/test/webdriver/automation.cc:251: dict.SetString("command", "SendKeyEventToActiveBrowserWindow"); Modify the command name to say something about the fact that they are simulating OS-level keyboard input. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:114: driver = WebDriver(launcher.GetURL(), {}) I think this will probably break the tests. The python bindings currently in chromium tree expect 3 args, not 2. You can either modify src/DEPS to pull in a newer version of the bindings (should be done in a different CL) or wait for someone else to do it for you (probably we'll get to it this week or next) http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/keymap.cc File chrome/test/webdriver/keymap.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/keymap... chrome/test/webdriver/keymap.cc:176: } // namespace webdriver remove this file from your CL http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... File chrome/test/webdriver/session.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:168: scoped_ptr<ListValue> args(new ListValue); why the change to scoped_ptr here? http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:172: "{if(document.activeElement)" can you make this a bit more readable http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:619: LOG(ERROR) << "Failed to send key event. Event details:\n" looks like the indentation got messed up somehow http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/webdri... File chrome/test/webdriver/webdriver_remote_tests.py (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/webdri... chrome/test/webdriver/webdriver_remote_tests.py:100: class RemoteWebdriverNativeEventsTest(RemoteWebDriverTest): Do not add to this file. I was planning on deleting it. These tests are not run continuously.
hello there, I merged the conflicts that arose from the change in the automation and responded to the comments. Then I had this strange error showing up whenever I tried to send_keys(...): [2009:2009:789814591268:ERROR:automation_provider_observers.cc(1044)] DOM operation completed, but no reply message I really struggled on this one because basically the SendWebKitEvent is the same as the one from the trunk but then I tried using other functions that used notification observers (like click()) and the same error appeared... As I have not touched those functions, I tend to say that this error comes from a previous commit which I didn't managed to track yet. Anyway most of the previous comments were addressed so I uploaded the changes. http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4662: void TestingAutomationProvider::SendKeyEventToActiveTab( On 2011/03/10 00:28:58, kkania wrote: > This function has changed since a couple days ago. Please update your client Done. http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4673: browser->GetSelectedTabContents()->render_view_host()-> On 2011/03/10 00:28:58, kkania wrote: > 4 space indentation here Done. http://codereview.chromium.org/6630001/diff/6046/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:4675: return; On 2011/03/10 00:28:58, kkania wrote: > remove the extra return Done. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/automa... File chrome/test/webdriver/automation.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/automa... chrome/test/webdriver/automation.cc:243: void Automation::SendWebKeyEvent(int tab_id, On 2011/03/10 00:28:58, kkania wrote: > I would prefer creating a separate method here for sending native events, since > they call two different commands and should have different arguments. Done. I created a SendNativeWebKeyEvent uses different json args (only those necessary for the event to be handled in ui_controls::SendKeyPress) http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/automa... chrome/test/webdriver/automation.cc:251: dict.SetString("command", "SendKeyEventToActiveBrowserWindow"); On 2011/03/10 00:28:58, kkania wrote: > Modify the command name to say something about the fact that they are simulating > OS-level keyboard input. Changed to SendOSLevelKeyEvent http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:114: driver = WebDriver(launcher.GetURL(), {}) On 2011/03/10 00:28:58, kkania wrote: > I think this will probably break the tests. The python bindings currently in > chromium tree expect 3 args, not 2. You can either modify src/DEPS to pull in a > newer version of the bindings (should be done in a different CL) or wait for > someone else to do it for you (probably we'll get to it this week or next) Yes this is the syntax for the 2.0b3 bindings, the previous bindings only allow to specify some of the desiredCapabilities (browser, version) but not custom ones like the one I am using "chrome.nativeEvents" hence my change to the current version. I am keener on letting someone do the update as I don't really know what other parts of the tree are using the bindings now except chromedriver. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/keymap.cc File chrome/test/webdriver/keymap.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/keymap... chrome/test/webdriver/keymap.cc:176: } // namespace webdriver On 2011/03/10 00:28:58, kkania wrote: > remove this file from your CL Done. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... File chrome/test/webdriver/session.cc (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:168: scoped_ptr<ListValue> args(new ListValue); On 2011/03/10 00:28:58, kkania wrote: > why the change to scoped_ptr here? A remnant of my previous struggles with the focus test here, no real reason to keep it that's true, reverted back, thanks for pointing this out. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:172: "{if(document.activeElement)" On 2011/03/10 00:28:58, kkania wrote: > can you make this a bit more readable Done. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/sessio... chrome/test/webdriver/session.cc:619: LOG(ERROR) << "Failed to send key event. Event details:\n" On 2011/03/10 00:28:58, kkania wrote: > looks like the indentation got messed up somehow Done. http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/webdri... File chrome/test/webdriver/webdriver_remote_tests.py (right): http://codereview.chromium.org/6630001/diff/6046/chrome/test/webdriver/webdri... chrome/test/webdriver/webdriver_remote_tests.py:100: class RemoteWebdriverNativeEventsTest(RemoteWebDriverTest): On 2011/03/10 00:28:58, kkania wrote: > Do not add to this file. I was planning on deleting it. These tests are not run > continuously. Where should I add those tests then?
About the "DOM operation completed, but no reply message", don't worry about it. It is a phony error that I will submit a patch for today. Sorry it caused you such trouble. About needing to update the python client, I'll try to do it for you today or tomorrow. I'll CC you on the change. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4767: if (!BuildNativeWebKeyEventFromArgs(args, reply_message, &event)) it seems a bit strange to me how we are building a NativeWebKeyboardEvent, then stripping a couple items from it to send to SendKeyPress. Can we cut out the middle step? http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4773: AutomationJSONReply(this, reply_message) Since you send a reply synchronously in this func, you can just create an AutomationJSONReply reply; at top and just reply.SendError/Success to reduce code clutter http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4774: .SendError("Could not get the browser handle"); you can just .SendError(error) here http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4777: // The ui_controls::SendKeyPress function will generate up and down events instead of having a strange interface where we don't process half the events sent to us, can we just change this func to SendOSLevelKeyPress and drop the need for the type arg? Also, how about dropping the nativeKeyCode and indentifier params too? http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4781: DictionaryValue* reply_details = new DictionaryValue; reply.SendSuccess does not take ownership of the value. Create the DictionaryValue on the stack instead of the heap. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4800: bool control = !!(event.modifiers & WebKit::WebInputEvent::ControlKey); I am a bit surprised you need this !! dance. What warning does the compiler give without it? http://codereview.chromium.org/6630001/diff/13001/chrome/test/automation/auto... File chrome/test/automation/automation_json_requests.h (right): http://codereview.chromium.org/6630001/diff/13001/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:195: bool SendNativeWebKeyEventJSONRequest( drop the Web from this name http://codereview.chromium.org/6630001/diff/13001/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.h (right): http://codereview.chromium.org/6630001/diff/13001/chrome/test/webdriver/autom... chrome/test/webdriver/automation.h:58: void SendWebKeyEvent(int tab_id, I would prefer if you create a separate function for this, and have the new one take a ui::KeyboardCode and an int modifiers instead of a WebKeyEvent
oh, and about the tests, put them in chromedriver_tests.py
Okay, here is a cleaned up version, not using the webkeyevent as suggested, this simplifies greatly the proxy part. I also moved the tests to chromedriver_tests.py and removed the old test file from the CL. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4767: if (!BuildNativeWebKeyEventFromArgs(args, reply_message, &event)) On 2011/03/10 17:00:47, kkania wrote: > it seems a bit strange to me how we are building a NativeWebKeyboardEvent, then > stripping a couple items from it to send to SendKeyPress. Can we cut out the > middle step? Done. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4773: AutomationJSONReply(this, reply_message) On 2011/03/10 17:00:47, kkania wrote: > Since you send a reply synchronously in this func, you can just create an > AutomationJSONReply reply; at top and just reply.SendError/Success to reduce > code clutter Done. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4774: .SendError("Could not get the browser handle"); On 2011/03/10 17:00:47, kkania wrote: > you can just .SendError(error) here Done. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4777: // The ui_controls::SendKeyPress function will generate up and down events On 2011/03/10 17:00:47, kkania wrote: > instead of having a strange interface where we don't process half the events > sent to us, can we just change this func to SendOSLevelKeyPress and drop the > need for the type arg? Also, how about dropping the nativeKeyCode and > indentifier params too? Done. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4781: DictionaryValue* reply_details = new DictionaryValue; On 2011/03/10 17:00:47, kkania wrote: > reply.SendSuccess does not take ownership of the value. Create the > DictionaryValue on the stack instead of the heap. Done. Due to the changes up, the return dict isn't needed anymore, the key type filtering is done before in the automation pipeline, thus also reducing the useless calls to the automation proxy. Anyway the way of returning dicts is not standardized across the other methods in this file, how about another CL to decide that? some methods use new DicitonaryValue, some use scoped pointers and get(), etc. http://codereview.chromium.org/6630001/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4800: bool control = !!(event.modifiers & WebKit::WebInputEvent::ControlKey); On 2011/03/10 17:00:47, kkania wrote: > I am a bit surprised you need this !! dance. What warning does the compiler give > without it? There is now warning, it's just a common method to ensure correct typecast to bool, that was suggested by a previous reviewer. If you feel it's more troubling than useful I can remove them. http://codereview.chromium.org/6630001/diff/13001/chrome/test/automation/auto... File chrome/test/automation/automation_json_requests.h (right): http://codereview.chromium.org/6630001/diff/13001/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:195: bool SendNativeWebKeyEventJSONRequest( On 2011/03/10 17:00:47, kkania wrote: > drop the Web from this name Done. http://codereview.chromium.org/6630001/diff/13001/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.h (right): http://codereview.chromium.org/6630001/diff/13001/chrome/test/webdriver/autom... chrome/test/webdriver/automation.h:58: void SendWebKeyEvent(int tab_id, On 2011/03/10 17:00:47, kkania wrote: > I would prefer if you create a separate function for this, and have the new one > take a ui::KeyboardCode and an int modifiers instead of a WebKeyEvent Done.
http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4689: if (!BuildWebKeyEventFromArgs(args, reply_message, &event)) I am not sure what the benefit of having this additional function is. http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4708: if (!args->GetInteger("nativeKeyCode", &keycode)) { i think the keycode you are passing in is actually a ui::KeyboardCode, which is a windowsKeyCode, not a native one (except on win). I know the SendWebkitKey one passes both in as the same, but that is not really correct. http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4719: if (!GetBrowserFromJSONArgs(args, &browser, &error)) { I think you should also get TabContents here and then browser->SelectTabContentsAt( browser->GetIndexOfController(&tab_contents->controller()), true); or if you prefer, you can just remove the tab_index input parameter and require the user change the tab before http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.h:961: // Sends the WebKit key event from the OS level to the browser window, you're not sending a webkit key event, you're simulating OS level key input; please cleanup the comment http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.h:966: // "type": automation::kRawKeyDownType, you don't use type anymore do you? http://codereview.chromium.org/6630001/diff/23005/chrome/test/automation/auto... File chrome/test/automation/automation_json_requests.h (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:193: // Requests to send the WebKit event for the given |WebKeyEvent| to a this comment is no longer accurate http://codereview.chromium.org/6630001/diff/23005/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:199: ui::KeyboardCode keycode, the convention we've been using in these files is key_code, not keycode http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.cc (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... chrome/test/webdriver/automation.cc:230: automation(), windex, tab_index, keycode, modifiers); key_code http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.h (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... chrome/test/webdriver/automation.h:62: // Sends an OS level key event to the current browser. Waits until the key Your func doesn't wait until the key has been processed by the page. Change the func or fix the comment http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... chrome/test/webdriver/automation.h:65: ui::KeyboardCode keycode, key_code http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/chrom... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/chrom... chrome/test/webdriver/chromedriver_tests.py:148: #TODO(timothe): run this test on a machine with an IME enabled. how are you planning on doing this? you can just disable the test instead of commenting it. http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... File chrome/test/webdriver/session.cc (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... chrome/test/webdriver/session.cc:631: // The automation proxy will generate up/down events for us, we proxy -> provider http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... chrome/test/webdriver/session.cc:638: static_cast<ui::KeyboardCode>(key_events[i].key_code), isn't key_code already a ui::KeyboardCode http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... chrome/test/webdriver/session.cc:653: *success = false; indentation
Sorry for the wait, Tokyo was quite Chaotic last week, I focused on other things... http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4689: if (!BuildWebKeyEventFromArgs(args, reply_message, &event)) On 2011/03/11 16:32:13, kkania wrote: > I am not sure what the benefit of having this additional function is. As the fact of building a key event form the args took most of the lines of this method, I found it cleaner to refactor it into another one for this purpose only, leaving this one hopefully more understandable. http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4708: if (!args->GetInteger("nativeKeyCode", &keycode)) { Yes indeed it is a ui::KeyboardCode, I don't quite get the difference between a "windowskeycode" and a "native one" though. What do you suggest? On 2011/03/11 16:32:13, kkania wrote: > i think the keycode you are passing in is actually a ui::KeyboardCode, which is > a windowsKeyCode, not a native one (except on win). I know the SendWebkitKey one > passes both in as the same, but that is not really correct. http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4719: if (!GetBrowserFromJSONArgs(args, &browser, &error)) { On 2011/03/11 16:32:13, kkania wrote: > I think you should also get TabContents here and then > browser->SelectTabContentsAt( > browser->GetIndexOfController(&tab_contents->controller()), true); > > or if you prefer, you can just remove the tab_index input parameter and require > the user change the tab before Done. http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.h:961: // Sends the WebKit key event from the OS level to the browser window, On 2011/03/11 16:32:13, kkania wrote: > you're not sending a webkit key event, you're simulating OS level key input; > please cleanup the comment Done. http://codereview.chromium.org/6630001/diff/23005/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.h:966: // "type": automation::kRawKeyDownType, On 2011/03/11 16:32:13, kkania wrote: > you don't use type anymore do you? Done, I removed it. http://codereview.chromium.org/6630001/diff/23005/chrome/test/automation/auto... File chrome/test/automation/automation_json_requests.h (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:193: // Requests to send the WebKit event for the given |WebKeyEvent| to a On 2011/03/11 16:32:13, kkania wrote: > this comment is no longer accurate Done, removed WebKit. http://codereview.chromium.org/6630001/diff/23005/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:199: ui::KeyboardCode keycode, On 2011/03/11 16:32:13, kkania wrote: > the convention we've been using in these files is key_code, not keycode Done. http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.cc (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... chrome/test/webdriver/automation.cc:230: automation(), windex, tab_index, keycode, modifiers); On 2011/03/11 16:32:13, kkania wrote: > key_code Done. http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.h (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/autom... chrome/test/webdriver/automation.h:65: ui::KeyboardCode keycode, On 2011/03/11 16:32:13, kkania wrote: > key_code Done. http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/chrom... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/chrom... chrome/test/webdriver/chromedriver_tests.py:148: #TODO(timothe): run this test on a machine with an IME enabled. On 2011/03/11 16:32:13, kkania wrote: > how are you planning on doing this? you can just disable the test instead of > commenting it. @unittest.skip only works for python 2.7+, is it okay to use it? http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... File chrome/test/webdriver/session.cc (right): http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... chrome/test/webdriver/session.cc:631: // The automation proxy will generate up/down events for us, we On 2011/03/11 16:32:13, kkania wrote: > proxy -> provider Done. http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... chrome/test/webdriver/session.cc:638: static_cast<ui::KeyboardCode>(key_events[i].key_code), On 2011/03/11 16:32:13, kkania wrote: > isn't key_code already a ui::KeyboardCode Done. http://codereview.chromium.org/6630001/diff/23005/chrome/test/webdriver/sessi... chrome/test/webdriver/session.cc:653: *success = false; On 2011/03/11 16:32:13, kkania wrote: > indentation Done.
The patch set 15 now waits for the event to be finished using SendKeyPressNotifyWhenDone and a RunnableMethod that sends a success automation json reply. It also activates the tab automatically, this is written in the comments and fits more in the webdriver philosophy of being able to do only things that the user can.
http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4633: LOG(INFO) << "reply_message:" << reply_message; Please remove debugging LOG messages before checking in.
http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4633: LOG(INFO) << "reply_message:" << reply_message; On 2011/03/23 10:16:05, Paweł Hajdan Jr. wrote: > Please remove debugging LOG messages before checking in. Done.
http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4531: remove unecessary newline http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4536: if (!args->GetInteger("type", &type)) { fix indentation http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4624: if (!BuildWebKeyEventFromArgs(args, reply_message, &event)) instead of giving reply_message, can you give it an std::string* error_msg like GetTabFromJSONArgs does; then send the error here. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4646: if (!args->GetInteger("nativeKeyCode", &keycode)) { ui::KeyboardCode is supposed to be a platform independent key code (although really it is just a windows key code). A native key code is like a gdk key. How about rename this to "keyCode"? http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4664: // The key events will be sent ot the browser level, we need the current tab ot -> at http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4665: // containing the element we send the text in to be shown. I do not like this comment for two reasons: 1) It says it will send a key event to the browser level. What is a browser level? I know what you mean, but this is unclear. 2) It references elements. This function can be used for typing in the omnibox too, right? Please fix. http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/autom... chrome/test/webdriver/automation.cc:222: void Automation::SendNativeKeyEvent(int tab_id, put this func after SendWebKeyEvent to match header order http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/chrom... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/chrom... chrome/test/webdriver/chromedriver_tests.py:128: self.capabilities = DesiredCapabilities.CHROME I think the style guide suggests prefixing instance variables with _; at least, that's what we've been using here http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/chrom... chrome/test/webdriver/chromedriver_tests.py:141: driver.get(self.SEARCH) I know a few tests here use google search, but please add an input box to test_page.html or make a new page instead. We shouldn't use external sites for testing if possible http://codereview.chromium.org/6630001/diff/35001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/6630001/diff/35001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.h:973: void SendOSLevelKeyEvent(DictionaryValue* args, this comment is out of date http://codereview.chromium.org/6630001/diff/35001/chrome/test/automation/auto... File chrome/test/automation/automation_json_requests.h (right): http://codereview.chromium.org/6630001/diff/35001/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:194: // browser window containing the specified tab. Returns true on success. this comment is out of date
http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4531: On 2011/03/24 23:20:32, kkania wrote: > remove unecessary newline Done. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4536: if (!args->GetInteger("type", &type)) { On 2011/03/24 23:20:32, kkania wrote: > fix indentation Done. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4624: if (!BuildWebKeyEventFromArgs(args, reply_message, &event)) On 2011/03/24 23:20:32, kkania wrote: > instead of giving reply_message, can you give it an std::string* error_msg like > GetTabFromJSONArgs does; then send the error here. Done. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4646: if (!args->GetInteger("nativeKeyCode", &keycode)) { On 2011/03/24 23:20:32, kkania wrote: > ui::KeyboardCode is supposed to be a platform independent key code (although > really it is just a windows key code). A native key code is like a gdk key. > > How about rename this to "keyCode"? Done. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4664: // The key events will be sent ot the browser level, we need the current tab On 2011/03/24 23:20:32, kkania wrote: > ot -> at Done. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4665: // containing the element we send the text in to be shown. 1) Done, changed to "browser window". 2) Well, Typing in the omnibox is more a side effect than a feature: you must first get an element on the page using webdriver, then if you send the "ctrl+L" shortcut the event gets sent to the browser which understands it as "go to the omnibox" by default. Afterwards if you send keys to the previously selected element they will be sent to the omnibox as it is focused. If we want to make it clear to the user, I saw that the automation provider also have a SetOmniboxText, OmniboxAcceptInput and OmniboxMovePopupSelection, we could make this available from webdriver either by adding a new command to the wire protocol (don't know if that's a good idea...) or by specifying a special id that would return the omnibox when queried via get_element_by_id("#chrome_omnibox") for instance. On 2011/03/24 23:20:32, kkania wrote: > I do not like this comment for two reasons: > 1) It says it will send a key event to the browser level. What is a browser > level? I know what you mean, but this is unclear. > 2) It references elements. This function can be used for typing in the omnibox > too, right? > > Please fix. http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/autom... File chrome/test/webdriver/automation.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/autom... chrome/test/webdriver/automation.cc:222: void Automation::SendNativeKeyEvent(int tab_id, On 2011/03/24 23:20:32, kkania wrote: > put this func after SendWebKeyEvent to match header order Done. http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/chrom... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/chrom... chrome/test/webdriver/chromedriver_tests.py:128: self.capabilities = DesiredCapabilities.CHROME On 2011/03/24 23:20:32, kkania wrote: > I think the style guide suggests prefixing instance variables with _; at least, > that's what we've been using here Done. http://codereview.chromium.org/6630001/diff/28015/chrome/test/webdriver/chrom... chrome/test/webdriver/chromedriver_tests.py:141: driver.get(self.SEARCH) On 2011/03/24 23:20:32, kkania wrote: > I know a few tests here use google search, but please add an input box to > test_page.html or make a new page instead. We shouldn't use external sites for > testing if possible Done. http://codereview.chromium.org/6630001/diff/35001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/6630001/diff/35001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.h:973: void SendOSLevelKeyEvent(DictionaryValue* args, On 2011/03/24 23:20:32, kkania wrote: > this comment is out of date Done. http://codereview.chromium.org/6630001/diff/35001/chrome/test/automation/auto... File chrome/test/automation/automation_json_requests.h (right): http://codereview.chromium.org/6630001/diff/35001/chrome/test/automation/auto... chrome/test/automation/automation_json_requests.h:194: // browser window containing the specified tab. Returns true on success. On 2011/03/24 23:20:32, kkania wrote: > this comment is out of date Done.
http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4665: // containing the element we send the text in to be shown. I see. I don't think we want to add SetOmniboxText and friends to the webdriver protocol. Webdriver is focused on testing websites, not the browser. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4666: browser->SelectTabContentsAt( If you want to keep this selection here, change the name of this method and the JSON message to SendOSLevelKeyEventToTab. Otherwise, remove this select here, the tab_index parameter of the JSON message, and do the selection on the ChromeDriver side.
http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4665: // containing the element we send the text in to be shown. I agree, let's not add browser centric capabilities to the driver. On 2011/03/25 16:37:36, kkania wrote: > I see. I don't think we want to add SetOmniboxText and friends to the webdriver > protocol. Webdriver is focused on testing websites, not the browser. http://codereview.chromium.org/6630001/diff/28015/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4666: browser->SelectTabContentsAt( Done. Added ToTab. On 2011/03/25 16:37:36, kkania wrote: > If you want to keep this selection here, change the name of this method and the > JSON message to SendOSLevelKeyEventToTab. > > Otherwise, remove this select here, the tab_index parameter of the JSON message, > and do the selection on the ChromeDriver side.
LGTM after nits http://codereview.chromium.org/6630001/diff/49001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/49001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4632: void TestingAutomationProvider::SendSuccessReply(IPC::Message* reply_message) { convention is to put funcs in .cc in the same order listed in .h http://codereview.chromium.org/6630001/diff/49001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4642: .SendError("'nativeKeyCode' missing or invalid."); update this
http://codereview.chromium.org/6630001/diff/49001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6630001/diff/49001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4632: void TestingAutomationProvider::SendSuccessReply(IPC::Message* reply_message) { On 2011/03/29 04:55:12, kkania wrote: > convention is to put funcs in .cc in the same order listed in .h Done. http://codereview.chromium.org/6630001/diff/49001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4642: .SendError("'nativeKeyCode' missing or invalid."); On 2011/03/29 04:55:12, kkania wrote: > update this Done.
Some merging conflicts appeared after the recent "screenshot on error" update, so I fixed those.
Ho and by the way I don't have commit rights yet, so even with lgtm I'm kind of stuck here, if everybody agrees on the lgtm and someone can commit on my behalf I'll be grateful.
Greetings Tim, I have sent this change to the commit-queue and it is trying your change: <http://build.chromium.org/p/tryserver.chromium/waterfall>. It will land this change if its try jobs do not cause any breaks. :) Regards, Hironori Bono
Change committed as 80000
Thank you very much Bono-san for committing the change. Best, Timothe On Thu, Mar 31, 2011 at 11:32 AM, <commit-bot@chromium.org> wrote: > Change committed as 80000 > > > http://codereview.chromium.org/6630001/ >
Hey Timothe, looks like your new test is failing. Look under the chromedriver_tests step on this page: http://build.chromium.org/p/chromium.pyauto/waterfall I'll send you a CL disabling this test for now until we can figure out what's wrong
Is it possible that the test page html was cached by the test bot? hence my new text input cannot be found? I would suggest adding no-cache metas to the test page. On Thu, Mar 31, 2011 at 1:03 PM, <kkania@chromium.org> wrote: > Hey Timothe, > > looks like your new test is failing. Look under the chromedriver_tests > step on > this page: http://build.chromium.org/p/chromium.pyauto/waterfall > > I'll send you a CL disabling this test for now until we can figure out > what's > wrong > > > http://codereview.chromium.org/6630001/ >
No, it wasn't cached. But the CL I sent you fixed the problem, so that now it is passing at least on Linux. We'll have to wait and see if it passed on the other platforms
It passes on Win Vista, but not on Win7 it appears: http://build.chromium.org/p/chromium.pyauto/builders/Win7/builds/6689/steps/c...
Seems so, the failure is internal to ui_controls_win.cc+165 : SendKeyPressImpl as it seems to return false. I have no idea why and have no access to a win7 machine right now. Any suggestion on how to handle this? On Thu, Mar 31, 2011 at 2:06 PM, <kkania@chromium.org> wrote: > It passes on Win Vista, but not on Win7 it appears: > > > http://build.chromium.org/p/chromium.pyauto/builders/Win7/builds/6689/steps/c... > > > http://codereview.chromium.org/6630001/ >
From what I can see looking at the code: the only way SendKeyPressImpl can return false, hence triggering an internal send key error is the function call at +233: if (::SendInput(i, input, sizeof(INPUT)) != i) return false; All the previous tests of this function call a method with a boolean signature but that never returns false...so the only "return false;" statement that triggers the internal sendkeys error is at this line 233. I would suggest adding logging of a GetLastError() call before the return false to see what's happening on windows 7.The reason of SendInput returning false as written in the microsoft documentation (http://msdn.microsoft.com/en-us/library/ms646310(v=vs.85).aspx) is - This function fails when it is blocked by UIPI (User Interface Privilege Isolation) or - The input was already blocked by another thread Any chance win7 bots could have issues with those 2 previous causes? I'll try looking into this more deeply once I have access to a win7 machine, which will not be the case until end of next week as I'm travelling only with a macbook laptop right now, I am sorry for the trouble. On Thu, Mar 31, 2011 at 2:19 PM, Timothe Faudot <timothe@google.com> wrote: > Seems so, the failure is internal to ui_controls_win.cc+165 : > SendKeyPressImpl as it seems to return false. I have no idea why and have no > access to a win7 machine right now. Any suggestion on how to handle this? > > > On Thu, Mar 31, 2011 at 2:06 PM, <kkania@chromium.org> wrote: > >> It passes on Win Vista, but not on Win7 it appears: >> >> >> http://build.chromium.org/p/chromium.pyauto/builders/Win7/builds/6689/steps/c... >> >> >> http://codereview.chromium.org/6630001/ >> > >
I had access to a win7 machine today, I synched to trunk, build and ran the chromedriver_tests and they all worked, including the one that's failing on the buildbot. I didn't ran them as a superuser or anything so that may be a buildbot configuration issue in the end. On 2011/03/31 21:27:35, timothe wrote: > From what I can see looking at the code: > the only way SendKeyPressImpl can return false, hence triggering an internal > send key error is the function call at +233: > > if (::SendInput(i, input, sizeof(INPUT)) != i) return false; All > the previous tests of this function call a method with a boolean > signature but that never returns false...so the only "return false;" > statement that triggers the internal sendkeys error is at this line > 233. > I would suggest adding logging of a GetLastError() call before the > return false to see what's happening on windows 7.The reason of > SendInput returning false as written in the microsoft documentation > (http://msdn.microsoft.com/en-us/library/ms646310%28v=vs.85%29.aspx) is > > - This function fails when it is blocked by UIPI (User Interface > Privilege Isolation) > > or > > - The input was already blocked by another thread > > Any chance win7 bots could have issues with those 2 previous causes? > I'll try looking into this more deeply once I have access to a win7 > machine, which will not be the case until end of next week as I'm > travelling only with a macbook laptop right now, I am sorry for the > trouble. > > > On Thu, Mar 31, 2011 at 2:19 PM, Timothe Faudot <mailto:timothe@google.com> wrote: > > > Seems so, the failure is internal to ui_controls_win.cc+165 : > > SendKeyPressImpl as it seems to return false. I have no idea why and have no > > access to a win7 machine right now. Any suggestion on how to handle this? > > > > > > On Thu, Mar 31, 2011 at 2:06 PM, <mailto:kkania@chromium.org> wrote: > > > >> It passes on Win Vista, but not on Win7 it appears: > >> > >> > >> > http://build.chromium.org/p/chromium.pyauto/builders/Win7/builds/6689/steps/c... > >> > >> > >> http://codereview.chromium.org/6630001/ > >> > > > > |
