Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BiDi][rb] Add set viewport for browsing context #15290

Merged
merged 5 commits into from
Mar 3, 2025

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Feb 16, 2025

User description

Motivation and Context

This PR implements the setViewport command for the Ruby binding

In order to add full support for the BiDi protocol we are implementing https://w3c.github.io/webdriver-bidi/#command-browsingContext-setViewport for the Ruby binding

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.

PR Type

Enhancement, Tests


Description

  • Added set_viewport method to manage browsing context viewport.

  • Implemented integration test for set_viewport functionality.

  • Updated type signature for set_viewport in Ruby interface file.


Changes walkthrough 📝

Relevant files
Enhancement
browsing_context.rb
Add `set_viewport` method for browsing context                     

rb/lib/selenium/webdriver/bidi/browsing_context.rb

  • Added set_viewport method to set viewport dimensions and device pixel
    ratio.
  • Utilized @bidi.send_cmd to execute the browsingContext.setViewport
    command.
  • +6/-0     
    browsing_context.rbs
    Update type signature for `set_viewport` method                   

    rb/sig/lib/selenium/webdriver/bidi/browsing_context.rbs

  • Updated type signature for set_viewport method.
  • Defined parameters and return type for set_viewport.
  • +2/-0     
    Tests
    browsing_context_spec.rb
    Add integration test for `set_viewport` method                     

    rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb

  • Added integration test for set_viewport method.
  • Verified viewport dimensions and device pixel ratio using JavaScript.
  • +9/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Sorry, something went wrong.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Parameter Validation

    The set_viewport method does not validate that width, height and device_pixel_ratio parameters are positive numbers. Invalid values could cause unexpected behavior.

    def set_viewport(context_id: nil, width: nil, height: nil, device_pixel_ratio: nil)
      context_id ||= @bridge.window_handle
      params = {context: context_id, viewport: {width:, height:}, device_pixel_ratio: device_pixel_ratio.to_f}
      @bidi.send_cmd('browsingContext.setViewport', **params)
    end
    Error Handling

    The set_viewport method does not handle potential errors from the BiDi command. Should catch and handle cases where the command fails.

    @bidi.send_cmd('browsingContext.setViewport', **params)

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 16, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate viewport dimension parameters

    Add parameter validation to ensure width, height and device_pixel_ratio are
    positive numbers before sending the command

    rb/lib/selenium/webdriver/bidi/browsing_context.rb [98-102]

     def set_viewport(context_id: nil, width: nil, height: nil, device_pixel_ratio: nil)
       context_id ||= @bridge.window_handle
    +  raise ArgumentError, 'Width and height must be positive integers' unless width.to_i > 0 && height.to_i > 0
    +  raise ArgumentError, 'Device pixel ratio must be positive' if device_pixel_ratio && device_pixel_ratio.to_f <= 0
       params = {context: context_id, viewport: {width:, height:}, device_pixel_ratio: device_pixel_ratio.to_f}
       @bidi.send_cmd('browsingContext.setViewport', **params)
     end
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Adding validation for viewport dimensions and pixel ratio is crucial for preventing invalid API calls and providing clear error messages. This improves robustness and helps catch potential issues early.

    Medium
    Learned
    best practice
    Add validation checks for required parameters to prevent potential null reference errors

    Add validation checks for required parameters width and height at the start of
    the set_viewport method to ensure they are not nil, as they are required for
    setting viewport dimensions. This will prevent potential errors when calling the
    BiDi command.

    rb/lib/selenium/webdriver/bidi/browsing_context.rb [98-102]

     def set_viewport(context_id: nil, width: nil, height: nil, device_pixel_ratio: nil)
    +  raise ArgumentError, "width parameter is required" if width.nil?
    +  raise ArgumentError, "height parameter is required" if height.nil?
       context_id ||= @bridge.window_handle
       params = {context: context_id, viewport: {width:, height:}, device_pixel_ratio: device_pixel_ratio.to_f}
       @bidi.send_cmd('browsingContext.setViewport', **params)
     end
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low

    Sorry, something went wrong.

    @aguspe aguspe added the C-rb label Feb 16, 2025

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature.
    Copy link
    Contributor

    @luke-hill luke-hill left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Seems good. Couple of minor questions about defaulting / documenting

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature.
    @aguspe aguspe merged commit 3a6f47b into SeleniumHQ:trunk Mar 3, 2025
    22 of 23 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    * Add set viewport for browsing context
    
    * Fix comment
    
    * Fix device_pixel_ratio
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    3 participants