[py] Update Remote Connection creation in driver classes

[titusfortner]

Description

• Current code does not allow chrome or edge remote connections in remote webdriver because chromium class will not match on the browser name
• If the remote webdriver class has code for creating the handler, the subclasses do not need to
• There is extra code in chromium class (and a deprecation) that will only matter if someone is subclassing Chromium class with a different browser.
• Made all the quit methods the same
• Have all driver constructors call the quit method to stop the driver if there is an exception when starting session

Motivation and Context

Tidying some things up in preparation for another change

Considerations

• Do we actually need the extra Chromium class code, or can we safely assume there aren't additional implementations with separate vendor_code that we need to worry about? Would be nice to just remove those parameters.
• Should we put the try/except/quit code in the remote webdriver constructor instead of all the subclasses?
• Should we get rid of the subclass quit() methods and put self.service.stop() in the currently unused stop_client() method (https://github.com/SeleniumHQ/selenium/blob/selenium-4.10.0/py/selenium/webdriver/remote/webdriver.py#L467-L471)

[codecov-commenter]

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (6d7139d) 57.56% compared to head (d14f819) 57.76%.
Report is 4 commits behind head on trunk.

❗ Current head d14f819 differs from pull request most recent head cadb9de. Consider uploading reports for the commit cadb9de to get more accurate results

Files	Patch %	Lines
py/selenium/webdriver/ie/webdriver.py	25.00%	6 Missing ⚠️
py/selenium/webdriver/common/bidi/cdp.py	0.00%	4 Missing ⚠️
py/selenium/webdriver/chromium/webdriver.py	87.50%	1 Missing and 2 partials ⚠️
py/selenium/webdriver/safari/webdriver.py	50.00%	2 Missing ⚠️
py/selenium/webdriver/firefox/webdriver.py	80.00%	1 Missing ⚠️
py/selenium/webdriver/remote/webdriver.py	66.66%	1 Missing ⚠️
py/selenium/webdriver/remote/webelement.py	50.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #12364      +/-   ##
==========================================
+ Coverage   57.56%   57.76%   +0.19%     
==========================================
  Files          86       88       +2     
  Lines        5305     5332      +27     
  Branches      221      226       +5     
==========================================
+ Hits         3054     3080      +26     
+ Misses       2030     2026       -4     
- Partials      221      226       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[titusfortner]

@diemol I really wish I knew what was wrong in my local Python setup that tox is passing for me but failing on CI. Can you tell me what to fix?
@isaulv any suggestions for this code? Thanks.

[diemol]

Install the same tox version with the command used in GitHub actions and then run the same command.

[titusfortner]

Yes, that's the version I'm using...

(venv) titusfortner@SL-1837 selenium % tox --version
2.4.1 imported from /Users/titusfortner/code/selenium/py/venv/lib/python3.8/site-packages/tox/__init__.py

https://github.com/SeleniumHQ/selenium/blob/selenium-4.10.0/.github/workflows/ci-python.yml#L30

[isaulv]

• Do we actually need the extra Chromium class code, or can we safely assume there aren't additional implementations with separate vendor_code that we need to worry about? Would be nice to just remove those parameters.

IIRC, the grouping was created so that all Blink based browsers can use common code but allow for differences since the Blink based browsers have their own drivers. I wonder if we would have to do something slightly different for Chrome for Testing.

• Should we put the try/except/quit code in the remote webdriver constructor instead of all the subclasses?
  At the very least, quit should be only defined in the parent class, since it's not doing anything different. Python has pass through inheritance.

• Should we get rid of the subclass quit() methods and put self.service.stop() in the currently unused stop_client() method (https://github.com/SeleniumHQ/selenium/blob/selenium-4.10.0/py/selenium/webdriver/remote/webdriver.py#L467-L471)
  I don't know. I typically use quit and not stop client. I don't quite understand how the service code interacts with a driver.

[isaulv]

• If the remote webdriver class has code for creating the handler, the subclasses do not need to...

This part of the code is kind of hacky.
According to OO theory the parent shouldn't know anything about the child, but here we do care about the type of remote webdriver created depending on the browser specified.
The object hierarchy we have here is a where a local browser driver is a subtype of RemoteWebDriver, but the relationship is not so straightforward; a browser driver has special commands to interact with the local driver executable, while RemoteWebDriver only concerns itself with sending JSON messages to an intermediate node server who will then proxy that command to a local driver.
In some sense, RemoteWebDriver is an opaque type, or a dependent type, that is, the instance created is dependent on its input values.
In practical Python terms, if were to rewrite this to make OO sense, there should be a "AbstractWebdriver" base class that defines all the "Abstract" methods, and that there should be a factory function that creates a local ChromeDriver type, or a RemoteChromeDriver type, where it composes the types it needs for WebElement, RemoteConnection, etc.

This make it easier to subclass or just wholesale replace these objects with custom implementations. And you don't have to think about inheritance trees in a Java like manner.
I wonder if the Java implementation gets this right, but I am doubtful since I remember that sometimes you have to write Augmenter code for some features.

[isaulv]

I think this is good to go for now.
It provides consistency even if there are somethings that can be pulled to the parent class, but I would have to see how each driver behaves.

[titusfortner]

Ok, finally looking at this again...

According to OO theory the parent shouldn't know anything about the child, but here we do care about the type of remote webdriver created depending on the browser specified.

1. This is in the code already, it just doesn't work as intended
2. I mean, Ruby uses mixins for this, Java magically uses a ServiceLoader and .NET has a way to add commands based on the options class being used... Would be nice if there were a better way to do it in Python...

That being said... I'm not sure the end user can actually use this? I probably should create some tests as well 😂

[titusfortner]

Ok, I added the part that fixes the bug, but decided not to move logic from subclass to the superclass.