Skip to content

[py] Now the ClientWindowInfo object's attributes are validated through getters/setters #16093

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

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Jul 26, 2025

User description

🔗 Related Issues

💥 What does this PR do?

In this PR, I have encapsulated all instance attributes of ClientWindowInfo class in getters/setters and now the attributes are being validated through setters

🔧 Implementation Notes

It is not a standard practice to validate instance attributes inside a class method. More Pythonic approach is to have separate getters/setters to do the validations. The class method from_dict is cluttered with validations of different instance attributes before the class is getting instantiated. The more standard and structured approach for this is to validate through setters. After the changes made in this PR, the class method looks clean and readable.

In the current implementation although there are few getter methods like, get_state, get_client_window, get_width, get_height, get_x, get_y, which returns the value of corresponding attributes, the setting of these attributes are done in constructor, but the validations before they are being set is done in class method from_dict which I felt is bit strange.

The getters in the current code is not in pythonic style and it is Java way of doing things. The more pythonic approach is to have property descriptors, which I have implemented in this PR.

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Refactored ClientWindowInfo class to use Python properties instead of getter methods

  • Added validation in property setters for all attributes

  • Simplified from_dict class method by removing inline validations

  • Updated tests to use property access instead of getter methods


Diagram Walkthrough

flowchart LR
  A["Old getter methods"] --> B["Python properties"]
  C["from_dict validations"] --> D["Property setter validations"]
  E["Test getter calls"] --> F["Direct property access"]
Loading

File Walkthrough

Relevant files
Enhancement
browser.py
Convert getters to properties with validation                       

py/selenium/webdriver/common/bidi/browser.py

  • Replaced getter methods with Python @property decorators
  • Added setter methods with validation logic for all attributes
  • Simplified from_dict method by removing validation code
  • Added comprehensive docstrings for all properties
+109/-54
Tests
bidi_browser_tests.py
Update tests for property access                                                 

py/test/selenium/webdriver/common/bidi_browser_tests.py

  • Updated test assertions to use property access instead of getter
    methods
  • Changed get_client_window() to client_window property access
  • Changed get_state(), get_width(), get_height(), get_x(), get_y() to
    direct property access
+7/-7     

@selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jul 26, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

5678 - Not compliant

Non-compliant requirements:

• Fix ChromeDriver connection failure error that occurs after first instance
• Resolve "Error: ConnectFailure (Connection refused)" for subsequent ChromeDriver instantiations
• Ensure proper connection handling for multiple ChromeDriver instances

1234 - Not compliant

Non-compliant requirements:

• Fix JavaScript execution in link href on click() for Firefox
• Ensure click() triggers JavaScript in href attributes
• Restore functionality that worked in version 2.47.1 but broke in 2.48.0+

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

Missing Validation

The from_dict method no longer handles missing required keys or invalid data format, which could cause AttributeError or other exceptions when accessing properties if data is incomplete.

return cls(
    client_window = data.get("clientWindow"),
    state = data.get("state"),
    width = data.get("width"),
    height = data.get("height"),
    x = data.get("x"),
    y = data.get("y"),
    active = data.get("active"),
)
Property Access

Direct assignment to properties in init will trigger setters, but the constructor parameters should be validated before assignment to avoid partial object creation on validation failure.

self.client_window = client_window
self.state = state
self.width = width
self.height = height
self.x = x
self.y = y
self.active = active

Copy link
Contributor

qodo-merge-pro bot commented Jul 26, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle missing required dictionary fields

Using data.get() without default values can pass None to setters, causing
validation errors. Add proper error handling for missing required fields or
provide appropriate defaults.

py/selenium/webdriver/common/bidi/browser.py [224-232]

-return cls(
-    client_window = data.get("clientWindow"),
-    state = data.get("state"),
-    width = data.get("width"),
-    height = data.get("height"),
-    x = data.get("x"),
-    y = data.get("y"),
-    active = data.get("active"),
-)
+try:
+    return cls(
+        client_window=data["clientWindow"],
+        state=data["state"],
+        width=data["width"],
+        height=data["height"],
+        x=data["x"],
+        y=data["y"],
+        active=data["active"],
+    )
+except KeyError as e:
+    raise ValueError(f"Missing required field: {e}")
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that using data.get() can pass None to setters, but the setters already handle this by raising a ValueError. The proposed change improves the error message to be more specific about missing keys, which enhances debuggability.

Low
  • Update

@sandeepsuryaprasad
Copy link
Contributor Author

@navin772 can you please re-trigger the CI. I have fixed the formatting issue that i missed.. Thanks!

@sandeepsuryaprasad
Copy link
Contributor Author

@cgoldberg can we have this one merged..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 2/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants