Skip to content

Commit

Permalink
py: marionette is a remote connection
Browse files Browse the repository at this point in the history
RemoteWebDriver sets the internal property _is_remote to true on
construction.  The firefox.WebDriver ctor used to re-set it to false,
which is now only true when using the Firefox add-on approach.
  • Loading branch information
andreastt committed Jan 19, 2016
1 parent b10cc01 commit 3438ab9
Showing 1 changed file with 15 additions and 15 deletions.
30 changes: 15 additions & 15 deletions py/selenium/webdriver/firefox/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
# specific language governing permissions and limitations
# under the License.


try:
import http.client as http_client
except ImportError:
Expand Down Expand Up @@ -53,35 +52,36 @@ def __init__(self, firefox_profile=None, firefox_binary=None, timeout=30,
if capabilities is None:
capabilities = DesiredCapabilities.FIREFOX

if "marionette" in capabilities and capabilities['marionette'] is True:
# Let's use Marionette! WOOOOHOOOOO!
# marionette
if "marionette" in capabilities and capabilities["marionette"] is True:

This comment has been minimized.

Copy link
@isaulv

isaulv Jan 19, 2016

Contributor

This could be simplified to if capabilities.get("marionette"):

This comment has been minimized.

Copy link
@andreastt

andreastt Jan 19, 2016

Author Member

That’s not quite the same as what’s there currently. if capabilities.get("marionette"): tests that the value, if it’s there, evaluates to something truthy.

if "marionette" in capabilities and capabilities["marionette"] is True tests for boolean equivalence.

This comment has been minimized.

Copy link
@isaulv

isaulv Jan 19, 2016

Contributor

As a matter of strictness, your code would make sense except that you should use == True rather than is True.
I think if the specification is that marionette is a bool, a simple if test should suffice; otherwise if other values are allowed, then your code would make sense because now you're implying other values are acceptable.

This comment has been minimized.

Copy link
@andreastt

andreastt Jan 19, 2016

Author Member

I don’t know the reason why it was like that before. Probably verifying that the marionette key is set is sufficient, as you say.

Paging @AutomatedTester.

This comment has been minimized.

Copy link
@isaulv

isaulv Jan 19, 2016

Contributor

At least use == True so that it compares values and not object identities. In Python IRC I sometimes see people post why is True comparisons are failing.

This comment has been minimized.

Copy link
@andreastt

andreastt Jan 20, 2016

Author Member

Changed it to use a loose truthyness check in 3395766. @AutomatedTester can revert that with proper Python idioms if he’d like, but at least in this way the code does not behave in a surprising manner.

if "binary" in capabilities:
self.binary = capabilities["binary"]
self.service = Service(executable_path, firefox_binary=self.binary)
self.service.start()

RemoteWebDriver.__init__(self,
command_executor=FirefoxRemoteConnection(
remote_server_addr=self.service.service_url),
executor = FirefoxRemoteConnection(
remote_server_addr=self.service.service_url)
RemoteWebDriver.__init__(
self,
command_executor=executor

This comment has been minimized.

Copy link
@adamreyher

adamreyher Jan 19, 2016

Missing a comma.

This comment has been minimized.

Copy link
@andreastt

andreastt Jan 19, 2016

Author Member

Thanks! Addressed in 9e8d764.

desired_capabilities=capabilities,
keep_alive=True)

# use old Firefox add-on
else:
# Oh well... sometimes the old way is the best way.
if self.binary is None:
self.binary = FirefoxBinary()

if proxy is not None:
proxy.add_to_capabilities(capabilities)

RemoteWebDriver.__init__(self,
command_executor=ExtensionConnection("127.0.0.1", self.profile,
self.binary, timeout),
desired_capabilities=capabilities,
keep_alive=True)


self._is_remote = False
executor = ExtensionConnection("127.0.0.1", self.profile, self.binary, timeout)
RemoteWebDriver.__init__(
self,
command_executor=executor,
desired_capabilities=capabilities,
keep_alive=True)
self._is_remote = False

def quit(self):
"""Quits the driver and close every associated window."""
Expand Down

0 comments on commit 3438ab9

Please sign in to comment.