Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add executing custom command to Chrome Python bindings #5989

Merged

Conversation

@zhangwenqiang00
Copy link
Contributor

commented Jun 4, 2018

implement send_command_and_get_result interface, so we can control
chrome via devtools protocol commands.

@@ -115,6 +115,28 @@ def set_network_conditions(self, **network_conditions):
'network_conditions': network_conditions
})

def send_command_and_get_result(self, cmd, cmd_args):

This comment has been minimized.

Copy link
@AutomatedTester

AutomatedTester Jun 4, 2018

Member

This is really a poor API name since most methods in any language are essentially send a command and get a result

This comment has been minimized.

Copy link
@zhangwenqiang00

zhangwenqiang00 Jun 5, 2018

Author Contributor

shouldn't we be consistent with chromedriver interface name?
or change to "send_devtools_command_and_get_result" is ok?

@lmtierney

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

There's also just a send_command last time I checked, unless they got rid of it? I had this change all ready to push to master previously, but guidance from @kereliuk was that this endpoint was not intended for public use and will probably change in the near future. I believe this is because with extension commands, according to the the w3c spec, google needs to add /goog/ somewhere in the endpoint.

@lmtierney

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

I did find our previous conversation and updated the issue mentioning this functionality accordingly #5159 (comment)

@lmtierney

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

Unfortunately there has been no activity from the chromium team on this matter thus far

@kereliuk

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

@lmtierney Whoops, the /goog/ rename fell through my stack. I'm not 100% sure historically why the endpoint was added, but it points to a commonly used internal function call that is used and I assume was not intended to be used publicly.

I did discuss this with other Chromium folks and I can submit a change to chromedriver to have the endpoint be in the correct /goog/ extension style by EOD today and it will be pushed in our next release.

Here is a bug tracking the issue.

I guess in this PR it makes sense to either

  • rename the function name and have it hit the endpoint and return a not implemented error.
  • rename the function name and have it hit the send_command endpoint
  • wait until the next ChromeDriver is released and then push this PR with everything updated

Either way, I can own the responsibility of either submitting a PR here or pinging Lucas if I can't figure out where add the name change once the next ChromeDriver is out :)

@lmtierney

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

How about we wait until the next chromedriver release, then we can have @zhangwenqiang00 update the PR

@kereliuk

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

Sounds good!

@kereliuk

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2018

ChromeDriver 2.40 has been released with the endpoint /goog/cdp/execute. Should be good to go! @zhangwenqiang00

zhangwenqiang
Add executing chrome devtools protocol cmd to Chrome Python bindings
implement ExecuteCDP interface, so we can control
chrome via devtools protocol commands.

@zhangwenqiang00 zhangwenqiang00 force-pushed the zhangwenqiang00:zhangwenqiang/chromeCustomCommand branch from 78795e5 to 69630c3 Jun 22, 2018

@zhangwenqiang00

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2018

updated! @kereliuk

@@ -25,3 +25,4 @@ def __init__(self, remote_server_addr, keep_alive=True):
self._commands["launchApp"] = ('POST', '/session/$sessionId/chromium/launch_app')
self._commands["setNetworkConditions"] = ('POST', '/session/$sessionId/chromium/network_conditions')
self._commands["getNetworkConditions"] = ('GET', '/session/$sessionId/chromium/network_conditions')
self._commands['ExecuteCDP'] = ('POST', '/session/$sessionId/goog/cdp/execute')

This comment has been minimized.

Copy link
@kereliuk

kereliuk Jun 22, 2018

Contributor

Should this be camelCase to match the above commands?

I am not the expert here so maybe Lucas or David can chime in.

Execute Chrome Devtools Protocol command and get returned result
The command and command args should follow chrome devtools protocol domains/commands, refer to link
https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/devtools/protocol.json

This comment has been minimized.

Copy link
@kereliuk

kereliuk Jun 22, 2018

Contributor

Might be good to add this link in the docstring as I think it is a better more readable source.
https://chromedevtools.github.io/devtools-protocol/

@kereliuk
Copy link
Contributor

left a comment

Made some docstring/formatting comments but otherwise looks good!

@lmtierney

This comment has been minimized.

Copy link
Member

commented Jun 25, 2018

Sorry I was on vacation this past week or so. It looks like you've addressed @kereliuk's changes and we are planning on releasing a new version tomorrow so I'll pull it in now. The only thing we might think of changing is either changing the cmd_args to default to None or accept **kwargs

@lmtierney lmtierney merged commit 4082b77 into SeleniumHQ:master Jun 25, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@lmtierney

This comment has been minimized.

Copy link
Member

commented Jun 25, 2018

Thank you!

scottazord added a commit to Consunet/Apps that referenced this pull request Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.