Support for page load and pageLoad timeout type

[aik099]

Currently, the WebdriverClassicDriver::applyTimeouts method supports these timeout types:

• script;
• implicit;
• page.

The script and implicit are timeout types shared by Selenium Server 2 and W3C-compatible Selenium Server (3 and 4).

However, Selenium 2 uses page load instead of page and W3C-compatible Selenium Server (3 and 4) uses pageLoad.

Proposing to add support for page load and pageLoad values to ease migration from MinkSelenium2Driver, which supplied given timeout types as-is to the Selenium Server.

P.S.

• It's better to remove page timeout to avoid confusion, but that might look like a BC break.
• FYI: The script timeout type is completely useless because it only affects \Facebook\WebDriver\Remote\RemoteWebDriver::executeAsyncScript method usages, that aren't found in this driver.

[uuf6429]

Proposing to add support for page load and pageLoad values to ease migration from MinkSelenium2Driver, which supplied given timeout types as-is to the Selenium Server.

This is already handled in the facebook/upstream webdriver:
https://github.com/php-webdriver/php-webdriver/blob/main/lib/WebDriverTimeouts.php#L84

All the end users need to do is to pass "page" timeout and the driver figures out the rest (unless I misunderstood what's being asked here).

As to the "script" timeout, I'm honestly not sure how this code came out to be. I think I would leave it there if it's not doing any harm.

[aik099]

All the end users need to do is to pass "page" timeout and the driver figures out the rest (unless I misunderstood what's being asked here).

Maybe I wasn't clear enough. What I've tried to say is that MinkSelenium2Driver users were passing either page load (Selenium 2) or pageLoad (Selenium 3) to the setTimeouts method. Such an approach won't work out of the box when switching to this Mink driver. To avoid need for Find & Replace action in whole test suite I recommend supporting for these new values.

As to the "script" timeout, I'm honestly not sure how this code came out to be. I think I would leave it there if it's not doing any harm.

I have no idea. Stumbled upon it while trying to improve test coverage for timeout-handling code.

[uuf6429]

Ah, ok, understood, so basically adding 2 new cases to applyTimeouts:

switch(...) {
    case 'page load': ⭐
    case 'pageLoad':  ⭐
    case 'page':
        ...

I'll fix that, but I'll leave the "script" one out for now (what you said seems true though).

[aik099]

My thoughts exactly.