Skip to content

[bidi][java] Update the capture screenshot APIs to include all parameters and remove scroll parameter#13743

Merged
diemol merged 2 commits intoSeleniumHQ:trunkfrom
pujagani:add-capturescreenshot-params
Mar 27, 2024
Merged

[bidi][java] Update the capture screenshot APIs to include all parameters and remove scroll parameter#13743
diemol merged 2 commits intoSeleniumHQ:trunkfrom
pujagani:add-capturescreenshot-params

Conversation

@pujagani
Copy link
Copy Markdown
Contributor

@pujagani pujagani commented Mar 27, 2024 •
edited by qodo-code-review Bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

W3C BiDi spec is updated for captureScreenshot command. Additional parameters have been added and the "scrollIntoView" parameter is removed for element screenshots. The changes reflect this. This involves a breaking change of using "scrollIntoView" parameter. Avoiding deprecating it since the latest version of browser (including the one in Selenium Gtihub CI) will not support it.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

Bug fix, Enhancement

Description

  • Introduced new classes (BoxClipRectangle, CaptureScreenshotParameters, ClipRectangle, ElementClipRectangle) to support flexible screenshot capture options according to the updated W3C BiDi spec.
  • Updated BrowsingContext to include an enhanced captureScreenshot method that supports new screenshot parameters, including clipping areas and image format/quality.
  • Removed the scrollIntoView parameter from captureElementScreenshot to align with the latest BiDi spec, marking a breaking change.
  • Added comprehensive tests for the new screenshot capture functionality.

Changes walkthrough

Relevant files
Enhancement
BoxClipRectangle.java
Add BoxClipRectangle for box-shaped clipping in screenshots

java/src/org/openqa/selenium/bidi/browsingcontext/BoxClipRectangle.java

  • Introduced a new class BoxClipRectangle for defining a box-shaped
    clipping area for screenshots.
  • Implements toMap method to convert properties to a Map.
    		•  +38/-0   
    BrowsingContext.java
    Update captureScreenshot APIs to align with BiDi spec       

    java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java

  • Added a new captureScreenshot method that accepts
    CaptureScreenshotParameters.
  • Modified captureElementScreenshot to remove the scrollIntoView
    parameter and add handle.
  • Adjusted existing screenshot methods to align with updated BiDi spec.
    		•  +18/-14 
    CaptureScreenshotParameters.java
    Add CaptureScreenshotParameters for flexible screenshot options

    java/src/org/openqa/selenium/bidi/browsingcontext/CaptureScreenshotParameters.java

  • Introduced CaptureScreenshotParameters class for specifying screenshot
    capture options.
  • Supports setting origin, image format, quality, and clipping
    rectangle.
    		•  +66/-0   
    ClipRectangle.java
    Introduce ClipRectangle base class for screenshot clipping

    java/src/org/openqa/selenium/bidi/browsingcontext/ClipRectangle.java

  • Created an abstract ClipRectangle class as a base for clipping area
    definitions.
  • Defines an enumeration for clip type (element or box).
    		•  +49/-0   
    ElementClipRectangle.java
    Add ElementClipRectangle for element-based clipping in screenshots

    java/src/org/openqa/selenium/bidi/browsingcontext/ElementClipRectangle.java

  • New ElementClipRectangle class for element-based clipping in
    screenshots.
  • Allows specifying an element by sharedId and optionally a handle.
    		•  +41/-0   
    Tests
    BrowsingContextTest.java
    Update tests for new screenshot capture methods                   

    java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextTest.java

  • Updated tests to use the new screenshot capture methods.
  • Removed tests for deprecated scrollIntoView parameter.
  • Added tests for capturing screenshots with various parameters.
    		•  +21/-15 

    ✨ PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @qodo-code-review qodo-code-review Bot added P-enhancement PR with a new feature P-bug fix PR addresses a known issue labels Mar 27, 2024
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Mar 27, 2024

    PR Description updated to latest commit (be06c21)

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Mar 27, 2024

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR introduces significant changes across multiple files, including new classes and methods related to capturing screenshots with updated parameters according to the W3C BiDi spec. The complexity of these changes and their impact on existing functionality require a thorough review to ensure compatibility and correctness.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Missing Tests: The PR description mentions that all new and existing tests passed, but there are no new tests added for the newly introduced functionality and changes. It's crucial to add unit tests for the new classes (BoxClipRectangle, CaptureScreenshotParameters, ClipRectangle, ElementClipRectangle) and methods to ensure they work as expected and to prevent future regressions.

    Breaking Change Concern: The removal of the "scrollIntoView" parameter and its replacement with new parameters could potentially break existing clients relying on the previous behavior. While the PR description justifies this change due to spec updates and browser support, it's important to clearly communicate these breaking changes to users and possibly provide guidance on migrating to the new API.

    🔒 Security concerns

    No
    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

    [pr_reviewer]
some_config1=...
some_config2=...

    See the review usage page for a comprehensive guide on using this tool.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Mar 27, 2024 •
    edited
    Loading

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Make the map field immutable to ensure thread safety.

    Consider making the map field in BoxClipRectangle class immutable after the object is
    constructed to ensure thread safety and avoid accidental modifications. One way to achieve
    this is by using Collections.unmodifiableMap when returning the map in the toMap method.

    java/src/org/openqa/selenium/bidi/browsingcontext/BoxClipRectangle.java [36]

    -return map;
+return Collections.unmodifiableMap(map);
    Validate input parameters to prevent potential issues.

    In the ElementClipRectangle class, consider validating the input parameters (sharedId and
    handle) to ensure they are not null or empty. This can prevent potential issues when these
    values are used later on.

    java/src/org/openqa/selenium/bidi/browsingcontext/ElementClipRectangle.java [32-33]

    +if (sharedId == null || sharedId.isEmpty()) {
+  throw new IllegalArgumentException("sharedId cannot be null or empty");
+}
+if (handle != null && handle.isEmpty()) {
+  throw new IllegalArgumentException("handle cannot be empty");
+}
 map.put("sharedId", sharedId);
-map.put("handle", handle);
+if (handle != null) {
+  map.put("handle", handle);
+}
    Add null checks to improve method robustness.

    For the captureScreenshot method in BrowsingContext, consider adding null checks for the
    parameters argument to avoid potential NullPointerExceptions when calling
    parameters.toMap(). This can improve the method's robustness.

    java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java [236]

    -params.putAll(parameters.toMap());
+if (parameters != null) {
+  params.putAll(parameters.toMap());
+}
    Mark the type field as final to ensure immutability.

    In the ClipRectangle class, consider marking the type field as final since it is only set
    in the constructor and should not change during the lifetime of the object. This can help
    ensure the immutability of the class instances.

    java/src/org/openqa/selenium/bidi/browsingcontext/ClipRectangle.java [38]

    -private Type type;
+private final Type type;
    Enhancement
    Overload the imageFormat method to support more image format options.

    To enhance the flexibility of the imageFormat method in CaptureScreenshotParameters,
    consider overloading it to support additional image format options such as specifying
    image quality for formats that support it. This can provide more control over the
    screenshot output.

    java/src/org/openqa/selenium/bidi/browsingcontext/CaptureScreenshotParameters.java [53-55]

    -public CaptureScreenshotParameters imageFormat(String type, double quality) {
-  map.put("format", Map.of("type", type, "quality", quality));
+public CaptureScreenshotParameters imageFormat(String type, Optional<Double> quality) {
+  if (quality.isPresent()) {
+    map.put("format", Map.of("type", type, "quality", quality.get()));
+  } else {
+    map.put("format", Map.of("type", type));
+  }
   return this;
 }
    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

    [pr_code_suggestions]
some_config1=...
some_config2=...

    See the improve usage page for a comprehensive guide on using this tool.

    @pujagani pujagani added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Mar 27, 2024
    @diemol diemol merged commit a179a98 into SeleniumHQ:trunk Mar 27, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol diemol approved these changes

    Assignees

    No one assigned

    Labels

    B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings P-bug fix PR addresses a known issue P-enhancement PR with a new feature Review effort [1-5]: 4

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @pujagani @diemol