-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Adding a LaunchApp command to Chrome specific webdriver client. #168
Conversation
From the Python side this looks ok but I wonder if the Your thoughts @jleyba ? |
@@ -38,18 +43,33 @@ | |||
|
|||
private final ChromeDriverService service; | |||
|
|||
private final static Map<String, CommandInfo> chromeCommandsNameToUrl = ImmutableMap.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Correct modifier order is
static final
- Use
CHROME_COMMAND_NAME_TO_URL
for the name as this is an immutable class constant. - Move this above the definition of the
service
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for the points above.
@AutomatedTester I'd prefer to keep it there, it looks other browsers have specific implementations so it would be consistent with that (ie, firefox, opera, etc... directories). If you feel strongly about it I can put it over in third_party. @jleyba Thanks for the review! I made changes based on your comments, please take another look when you have a chance. |
@@ -0,0 +1,26 @@ | |||
/* | |||
Copyright 2014 Selenium committers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the copyright should just be:
Copyright 2014 Software Freedom Conservancy
(remove the Selenium committers and Google reference)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. For here and the other files added.
I don't necessarily agree with putting it in third party, almost makes more sense for chromedriver to release another python package to pypi for users to also grab, since this falls under the section of browser specific extensions to the protocol. @shs96c input appreciated. |
Hmm, I think it would be really helpful to end users to have this available in Selenium over having to download another package, in many cases users probably won't know this is available. There's already a hacky way to launch apps through the UI, so including this in Selenium will encourage users to do something better. |
Sorry for not replying sooner. I agree that yet another download is not useful. I suggest raising this on David David Burns On Thu, Mar 6, 2014 at 7:32 PM, richardrb [email protected] wrote:
|
@richardrb do you have any example specs which make use of this? |
Any updates? @AutomatedTester? |
There is a discussion on https://groups.google.com/forum/m/#!topic/selenium-developers/-GS1wyTH2aI about this that has gone dead, I suggest posting there for an update — On Mon, Oct 20, 2014 at 5:23 PM, Alexander Bayandin
|
@@ -62,7 +63,7 @@ def __init__(self, command_executor='http://127.0.0.1:4444/wd/hub', | |||
proxy.add_to_capabilities(desired_capabilities) | |||
self.command_executor = command_executor | |||
if type(self.command_executor) is bytes or type(self.command_executor) is str: | |||
self.command_executor = RemoteConnection(command_executor, keep_alive=keep_alive) | |||
self.command_executor = remote_connection_client(command_executor, keep_alive=keep_alive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see that the symbol remote_connection_client
is defined here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the commits in the PR added remote_connection_client parameter to this constructor. However that change is unnecessary and everything in this file needs to be reverted. (I'll add comments in chrome/webdriver.py explaining how)
@richardrb, this change looks mostly good to me. Can you make the few changes requested and rebase? Sorry it's taken so long to get around to. @AutomatedTester, I can't load the link you posted. If it's dead, should we close this? |
As discussed in https://groups.google.com/forum/m/#!topic/selenium-developers/-GS1wyTH2aI we are going to close this and this will need to be released separately by the Chromium Project p.s. I don't necessarily agree with this so suggest bringing it up in that email thread. |
@shs96c @lukeis @andreastt @AutomatedTester Some 20 days ago there was a post that suggested that how to implement vendor specific commands in the client bindings and where to put those client bindings' code are two separate questions. It didn't generate a lot of stir, fair enough. If there is anyone who is actively against reopening and merging [1] this PR or against the proposal in the post (which is basically the same thing), can you please reply there or here? While a decision about the place for vendor specific client code is being made in the other thread I see no reason to block changes in that code. In fact, IEDriver and FirefoxDriver, ChromeDriver and possibly other vendor specific client side changes went in just fine recently. (phantomjs specific #475 almost made it in, too) [1] After fixing issues including @andreastt suggestions and reverting unnecessary changes in core files webdriver/remote/webdriver.py and selenium/remote/HttpCommandExecutor.java. |
Well, maybe there's no one who is actively again this anymore. Let's reopen, make last adjustments and merge this. @richardrb , can you please
If any of these changes you don't know how or don't have time to do, let me know and I may do them; I'd make sure this change would still go in attributed to you even if I need to make adjustments before pushing. |
@@ -61,6 +63,7 @@ def __init__(self, executable_path="chromedriver", port=0, | |||
try: | |||
RemoteWebDriver.__init__(self, | |||
command_executor=self.service.service_url, | |||
remote_connection_client=ChromeRemoteConnection, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remote_connection_client parameter is unnecessary, it should not be passed here and reverted from remote/webdriver.py.
What you need to do is to pass an instance of ChromeRemoteConnection as a value of command_executor parameter.
See the current latest revision for an updated documentation that should make this less confusing:
- command_executor - Either a string representing URL of the remote server or a custom |
Lack of response from PR author. |
/session/$sessionId/chromium/launch_app This is using the approach first proposed by @richardrb in pull request #168
/session/$sessionId/chromium/launch_app This is using the approach proposed by @richardrb in pull request #168
/session/$sessionId/chromium/launch_app This is related to pull request #168
Adding a LaunchApp command for the Chrome specific Java/Python webdriver clients, which is supported by chromedriver 2.9 and above. Many of our users use the client code here, and this will make it easier for Chrome app developers be able to test their apps using chromedriver.