[java] have overloaded constructors to have configurable connect and read timeouts while instantiating WebDriver object

[code-with-abdullah]

Currently the code only has 10 seconds set as connect timeout and 3 minutes set as read timeout.

This fix will allow this to configurable at constructor level i.e you now have a separate constructor for ChromeDriver that supports read timeout and connect timeout as well.

[CLAassistant]

All committers have signed the CLA.

[titusfortner]

I don't love the idea of creating new constructors for each of the drivers, and if we do go that route, we should implement the full ClientConfig rather than just these 2 values. Also, this is missing Edge & IE support, and for this kind of implementation we'd want to set default values and have the old constructors consume the new constructor using those values. (important because of the upcoming changes to driver classes with DriverFinder implementation).

[diemol]

+1 to what @titusfortner says. Timeouts can be set already through the builder, we are figuring out how to avoid the user needing to set a default value.

[code-with-abdullah]

@titusfortner @diemol First of all thanks for your feedback.

As rightly pointed out this was missing Edge and IE support initially. That has now been updated.
For the point that we should have default values and consume those in old constructors, java does not support default value constructors, can you please guide on how this can be addressed?
Timeouts can be set already through the builder but I could not find a way that would allow end user to create an instance of ChromeDriver with these configurations. This implementation can be used to further enhance the builder method in all classes to provide support for timeout at the time of driver object instantiation.

[titusfortner]

I generally agree that even though it *can be done with the builder, you aren't working with a ChromeDriver instance, but an augmented RemoteWebDriver instance, which isn't as straightforward, and it would be nice to support it directly.

But, I don't think we just want to tack on extra timeout values. Instead of:

  public ChromeDriver(ChromeDriverService service, ChromeOptions options, int connectTimeoutSeconds, int readTimeoutSeconds) {  }

we should figure out how to implement:

  public ChromeDriver(ChromeDriverService service, ChromeOptions options, ClientConfig clientConfig) {  }

It actually looks like that might be easier to implement? I'm not following some of the changes being made here, so I'd need to pull it and test it to make sure it is actually doing what we want.

What I meant by using defaults and consume is something like:

DEFAULT_CONNECT_TIMEOUT = Duration.ofSeconds(10);
DEFAULT_READ_TIMEOUT = Duration.ofMinutes(3);

  public ChromeDriver(ChromeDriverService service, ChromeOptions options) {
this(service, options, DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT);
}

  public ChromeDriver(ChromeDriverService service, ChromeOptions options, int connectTimeoutSeconds, int readTimeoutSeconds) {
    super(new ChromeDriverCommandExecutor(service), Require.nonNull("Driver options", options), ChromeOptions.CAPABILITY, connectTimeoutSeconds, readTimeoutSeconds);
    casting = new AddHasCasting().getImplementation(getCapabilities(), getExecuteMethod());
    cdp = new AddHasCdp().getImplementation(getCapabilities(), getExecuteMethod());
  }

One constructor uses the other with a default instead of duplicating logic. That way when I go to make a change in the constructor logic, I only have to do it in one place.

This is actually easier if we use ClientConfig, though, because we don't need default constants, we can just new up an instance:

  public ChromeDriver(ChromeDriverService service, ChromeOptions options) {
this(service, options, ClientConfig.defaultConfig());
}

[diemol]

We have been chatting internally and we think it makes sense to add the extra constructor to define the timeout locally. However, we'd prefer if a ClientConfig is passed instead of the two variables. @krmahadevan has helped to illustrate this in #11545, perhaps you can get inspiration from there and get this PR in shape. If you need more help, please reach out to us in the Slack channel #selenium-tlc, https://www.selenium.dev/support/.

Thank you for putting this together.

[code-with-abdullah]

Hi @diemol

Thanks for your feedback!
I shall update this PR.

[diemol]

Hi @diemol

Thanks for your feedback! I shall update this PR.

Perfect, let us know if you need any help.

[code-with-abdullah]

Dear @diemol.
I have updated the PR as per your request. Please review and let me know if there are any changes needed and I will integrate those as soon as possible.

[diemol]

Left some additional comments, @code-with-abdullah.

[code-with-abdullah]

Great. Bundle of thanks to both of you for your precious feedback. I shall integrate all changes mentioned above and build the code as suggested before my next commit.

[code-with-abdullah]

@diemol and @krmahadevan

Thanks for your input on the PR. All the changes you requested have been integrated. Please review and let me know if any further changes are required.

[BilalKamran]

Seems one of the users in the commits has not signed the CLA, please check.

@diemol Resolved

[BilalKamran]

@krmahadevan @diemol requested changes are done, pr please

[krmahadevan]

@diemol the changes look good to me. Please help take a look incase, I missed something.

[BilalKamran]

@diemol please let us know if any change needed, we really hope to get this resolved and released with 4.9.

[diemol]

Thank you, @code-with-abdullah!

Thanks for you help, @krmahadevan!

[code-with-abdullah]

Thanks a lot @diemol
Stay blessed :)