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

Adding a LaunchApp command to Chrome specific webdriver client. #168

Closed
wants to merge 16 commits into from
Closed
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,20 @@
package org.openqa.selenium.chrome;

import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableMap;

import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.chrome.ChromeDriverCommand;
import org.openqa.selenium.remote.Command;
import org.openqa.selenium.remote.DriverCommand;
import org.openqa.selenium.remote.CommandInfo;
import org.openqa.selenium.remote.HttpCommandExecutor;
import org.openqa.selenium.remote.Response;

import java.io.IOException;
import java.net.ConnectException;
import java.net.URL;
import java.util.Map;
import java.util.HashMap;

/**
* A specialized {@link HttpCommandExecutor} that will use a {@link ChromeDriverService} that lives
Expand All @@ -36,19 +41,34 @@
*/
class ChromeCommandExecutor extends HttpCommandExecutor {

private static final Map<String, CommandInfo> CHROME_COMMANDS_NAME_TO_URL = ImmutableMap.of(
ChromeDriverCommand.LAUNCH_APP,
post("/session/:sessionId/chromium/launch_app"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please simply use constructor, new CommandInfo(), instead of method post()


private final ChromeDriverService service;

/**
* Creates a new ChromeCommandExecutor which will communicate with the chromedriver as configured
* by the given {@code service}.
*
*
* @param service The ChromeDriverService to send commands to.
*/
public ChromeCommandExecutor(ChromeDriverService service) {
super(service.getUrl());
super(CHROME_COMMANDS_NAME_TO_URL, service.getUrl());
this.service = service;
}

/**
* Creates a new ChromeCommandExecutor which will communicate with the chromedriver as configured
* by the given {@code service}.
*
* @param serviceUrl The URL of the service to communicate with.
*/
public ChromeCommandExecutor(URL serviceUrl) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add this constructor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this constructor to support the case where the chromedriver server is running independently of the client, and we don't want to manage it here in client code.

super(CHROME_COMMANDS_NAME_TO_URL, serviceUrl);
this.service = null;
}

/**
* Sends the {@code command} to the chromedriver server for execution. The server will be started
* if requesting a new session. Likewise, if terminating a session, the server will be shutdown
Expand All @@ -60,7 +80,8 @@ public ChromeCommandExecutor(ChromeDriverService service) {
*/
@Override
public Response execute(Command command) throws IOException {
if (DriverCommand.NEW_SESSION.equals(command.getName())) {
if (ChromeDriverCommand.NEW_SESSION.equals(command.getName()) &&
this.service != null) {
service.start();
}

Expand All @@ -76,7 +97,8 @@ public Response execute(Command command) throws IOException {
Throwables.propagateIfPossible(t);
throw new WebDriverException(t);
} finally {
if (DriverCommand.QUIT.equals(command.getName())) {
if (ChromeDriverCommand.QUIT.equals(command.getName()) &&
this.service != null) {
service.stop();
}
}
Expand Down
21 changes: 21 additions & 0 deletions java/client/src/org/openqa/selenium/chrome/ChromeDriver.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,16 @@

package org.openqa.selenium.chrome;

import com.google.common.collect.ImmutableMap;

import java.net.URL;

import org.openqa.selenium.Capabilities;
import org.openqa.selenium.OutputType;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.chrome.ChromeDriverCommand;
import org.openqa.selenium.chrome.ChromeCommandExecutor;
import org.openqa.selenium.remote.DriverCommand;
import org.openqa.selenium.remote.FileDetector;
import org.openqa.selenium.remote.RemoteWebDriver;
Expand Down Expand Up @@ -160,6 +166,17 @@ public ChromeDriver(ChromeDriverService service, Capabilities capabilities) {
super(new DriverCommandExecutor(service), capabilities);
}

/**
* Creates a new ChromeDriver instance. The {@code service} will be started along with the
* driver, and shutdown upon calling {@link #quit()}.
*
* @param service The service to use.
* @param capabilities The capabilities required from the ChromeDriver.
*/
public ChromeDriver(URL serviceUrl, Capabilities capabilities) {
super(new ChromeCommandExecutor(serviceUrl), capabilities);
}

@Override
public void setFileDetector(FileDetector detector) {
throw new WebDriverException(
Expand All @@ -174,6 +191,10 @@ public <X> X getScreenshotAs(OutputType<X> target) {
return target.convertFromBase64Png(base64);
}

public void launchApp(String id) {
execute(ChromeDriverCommand.LAUNCH_APP, ImmutableMap.of("id", id));
}

@Override
protected void startSession(Capabilities desiredCapabilities,
Capabilities requiredCapabilities) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
Copyright 2014 Selenium committers
Copy link
Member

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)

Copy link
Author

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.

Copyright 2014 Google Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package org.openqa.selenium.chrome;

import org.openqa.selenium.remote.DriverCommand;

final class ChromeDriverCommand implements DriverCommand {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see how this class/enum helps anything, especially while it is is declared with package-private access. I suggest reverting this class and simply using string literal "chromium.launchApp" in one place where it's used.

private ChromeDriverCommand(){}

static final String LAUNCH_APP = "chromium.launchApp";
}
Original file line number Diff line number Diff line change
Expand Up @@ -496,15 +496,15 @@ private Response createResponse(HttpResponse httpResponse, HttpContext context,
return response;
}

private static CommandInfo get(String url) {
protected static CommandInfo get(String url) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we revert all changes in this file. They are not necessary. Other classes that already create instances of CommandInfo outside of this class simply use CommandInfo constructor(s), we could keep doing the same here.

return new CommandInfo(url, HttpVerb.GET);
}

private static CommandInfo post(String url) {
protected static CommandInfo post(String url) {
return new CommandInfo(url, HttpVerb.POST);
}

private static CommandInfo delete(String url) {
protected static CommandInfo delete(String url) {
return new CommandInfo(url, HttpVerb.DELETE);
}

Expand Down
22 changes: 22 additions & 0 deletions py/selenium/webdriver/chrome/command.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Copyright 2014 WebDriver committers
# Copyright 2014 Google Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from selenium.webdriver.remote.command import Command

class ChromeCommand(Command):
"""
Defines Chrome specific commands while including standard WebDriver ones.
"""
LAUNCH_APP = "launchApp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enums isn't really a thing in Python. Please just embed the "launchApp" string in chrome/webdriver.py.

24 changes: 24 additions & 0 deletions py/selenium/webdriver/chrome/remote_connection.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Copyright 2014 WebDriver committers
# Copyright 2014 Google Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from selenium.webdriver.remote.remote_connection import RemoteConnection
from selenium.webdriver.chrome.command import ChromeCommand

class ChromeRemoteConnection(RemoteConnection):

def __init__(self, remote_server_addr, keep_alive=False):
RemoteConnection.__init__(self, remote_server_addr, keep_alive)
self._commands[ChromeCommand.LAUNCH_APP] =\
('POST', '/session/$sessionId/chromium/launch_app')
10 changes: 10 additions & 0 deletions py/selenium/webdriver/chrome/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
# limitations under the License.

import base64
from command import ChromeCommand
from remote_connection import ChromeRemoteConnection
from selenium.webdriver.remote.command import Command
from selenium.webdriver.remote.webdriver import WebDriver as RemoteWebDriver
from selenium.common.exceptions import WebDriverException
Expand Down Expand Up @@ -61,13 +63,21 @@ def __init__(self, executable_path="chromedriver", port=0,
try:
RemoteWebDriver.__init__(self,
command_executor=self.service.service_url,
remote_connection_client=ChromeRemoteConnection,
Copy link
Contributor

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

desired_capabilities=desired_capabilities,
keep_alive=True)
except:
self.quit()
raise
self._is_remote = False

def launch_app(self, app_id):
"""
Launches an app with the specified id
"""
return self.execute(ChromeCommand.LAUNCH_APP,
{'id': app_id})

def quit(self):
"""
Closes the browser and shuts down the ChromeDriver executable
Expand Down
5 changes: 3 additions & 2 deletions py/selenium/webdriver/remote/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ class WebDriver(object):
"""

def __init__(self, command_executor='http://127.0.0.1:4444/wd/hub',
desired_capabilities=None, browser_profile=None, proxy=None, keep_alive=False):
desired_capabilities=None, browser_profile=None, proxy=None, keep_alive=False,
remote_connection_client=RemoteConnection):
"""
Create a new driver that will issue commands using the wire protocol.

Expand All @@ -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)
Copy link
Member

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.

Copy link
Contributor

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)

self._is_remote = True
self.session_id = None
self.capabilities = {}
Expand Down