[Java] FPS support for local grid using bidi #236
Conversation
| @@ -57,7 +57,7 @@ | |||
| <dependency> | |||
| <groupId>org.seleniumhq.selenium</groupId> | |||
| <artifactId>selenium-java</artifactId> | |||
| <version>4.10.0</version> | |||
| <version>4.32.0</version> | |||
There was a problem hiding this comment.
This is breaking, as newer versions of Selenium (4.12+ IIRC) have dropped support for Java 8 so our build step will fail.
| @@ -728,6 +743,18 @@ private String sauceVisualCheckLocal(String snapshotName, CheckOptions options) | |||
| return result.result.getId(); | |||
| } | |||
|
|
|||
| private byte[] getScreenshot(boolean fullPage) { | |||
| if (fullPage) { | |||
There was a problem hiding this comment.
Have you tested it across all major browsers and with various websites?
There was a problem hiding this comment.
BiDi is not currently supported in Safari, but it depends on our target use case whether that might be okay. Otherwise, Diego said this works across Firefox and chromium based browsers.
refactor cropping feature to support local fps
… simplified and account for DPR != 1
| clipRect = | ||
| new Rectangle( | ||
| (int) Math.round(clipElementDims.getX() * devicePixelRatio), | ||
| (int) Math.round(clipElementDims.getY() * devicePixelRatio), | ||
| (int) Math.round(clipElementDims.getHeight() * devicePixelRatio), | ||
| (int) Math.round(clipElementDims.getWidth() * devicePixelRatio)); |
There was a problem hiding this comment.
I feel like this should belong in a helper of some sort, or at least a function in this class.
There was a problem hiding this comment.
For what reason? It's only 4 lines, and those lines are pretty readable.
There was a problem hiding this comment.
I guess... it's just that this function is growing very quickly.
| WebElement clipElement = getClipElement(options); | ||
| Point scrollOffset = fullPage ? new Point(0, 0) : window.getViewport().getPoint(); |
There was a problem hiding this comment.
Does it make sense to set scrollOffset to window.getViewport().getPoint() if clipElement == null (i.e. as an else statement after if (clipElement != null))? I think it is always overriden if clipElement is not null.
There was a problem hiding this comment.
I don't think that'd be necessary since the default is set here (and we'd just be overriding it in the else).
And that's correct on clip front -- at least for non full page screenshots. We will always attempt to scroll the element into view so we update the position here after that call.
Description
Adds support for Full Page Screenshots using BiDi on a local selenium instance. Requires use of a
webSocketUrl: truecapability in order to be functional.Types of Changes