fixed errors in browser.py for 15697

[rpallavisharma]

User description

🔗 Related Issues

💥 What does this PR do?

fixed errors in browser.py for 15697

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)
• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)
• Breaking change (fix or feature that would cause existing functionality to change)

---

PR Type

Bug fix

---

Description

• Added strict type validation to ClientWindowInfo.from_dict

• Raised ValueError for missing or invalid fields

• Improved error handling for dictionary parsing

---

Changes walkthrough 📝

	Relevant files
Bug fix	browser.py Add type checks and error handling to from_dict method      py/selenium/webdriver/common/bidi/browser.py Enhanced from_dict to validate field types strictly Now raises ValueError for missing or invalid fields Improved robustness of dictionary parsing for window info +44/-9

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 1234 - Not compliant Non-compliant requirements: • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click() Requires further human verification: • Need to verify if the PR changes actually fix the JavaScript click issue in Firefox 5678 - Not compliant Non-compliant requirements: • Fix "Error: ConnectFailure (Connection refused)" when instantiating ChromeDriver Requires further human verification: • Need to verify if the PR changes address the ChromeDriver connection issues

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error handling catches KeyError but the code uses get() which returns None instead of raising KeyError. This might mask issues where fields are missing entirely. except (KeyError, TypeError) as e: raise ValueError(f"Invalid data format for ClientWindowInfo: {e}") Missing Documentation The docstring has been updated to include the new ValueError exception, but there's no explanation of what specific validation rules are enforced for each field. Raises: ------ ValueError: If required fields are missing or have invalid types. """

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to ecc8cec

Category	Suggestion	Impact
Incremental [*]	✅ Fix indentation error Suggestion Impact: The commit fixed the indentation error by replacing tabs with spaces on line 142 (width = data['width']) and also fixed a similar indentation issue on line 149 (x = data['x']) code diff: - width = data["width"] + width = data["width"] if not isinstance(width, int) or width < 0: raise ValueError(f"width must be a non-negative integer, got {width}") @@ -147,7 +149,7 @@ if not isinstance(height, int) or height < 0: raise ValueError(f"height must be a non-negative integer, got {height}") - x = data["x"] + x = data["x"] if not isinstance(x, int): Fix the indentation error in line 142 where a tab character is used instead of spaces, which will cause a syntax error. py/selenium/webdriver/common/bidi/browser.py [142-148] +width = data["width"] +if not isinstance(width, int) or width < 0: + raise ValueError(f"width must be a non-negative integer, got {width}") +height = data["height"] +if not isinstance(height, int) or height < 0: + raise ValueError(f"height must be a non-negative integer, got {height}") [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: This identifies a critical syntax error where tab characters are used instead of spaces on line 142, which will cause an IndentationError and prevent the code from running. This is a high-impact issue that must be fixed.	High
Update

---

Previous suggestions

✅ Suggestions up to commit 6a87672

Category	Suggestion	Impact
Incremental [*]	✅ Fix indentation error Suggestion Impact: The commit fixed the indentation error by replacing tabs with spaces on line 142 (width = data['width']) and also fixed a similar indentation issue on line 149 (x = data['x']) code diff: - width = data["width"] + width = data["width"] if not isinstance(width, int) or width < 0: raise ValueError(f"width must be a non-negative integer, got {width}") @@ -147,7 +149,7 @@ if not isinstance(height, int) or height < 0: raise ValueError(f"height must be a non-negative integer, got {height}") - x = data["x"] + x = data["x"] if not isinstance(x, int): Fix the indentation error on line 142. The line uses tabs instead of spaces, which is inconsistent with the rest of the code and will cause a Python indentation error. py/selenium/webdriver/common/bidi/browser.py [142] +width = data["width"] +if not isinstance(width, int) or width < 0: + raise ValueError(f"width must be a non-negative integer, got {width}") - Suggestion importance[1-10]: 9 __ Why: Critical indentation error using tabs instead of spaces that would cause Python IndentationError and prevent the code from running.	High
Possible issue	Handle optional fields correctly Check if optional fields (x, y, active) are None before validating their types. The current implementation will raise errors for missing optional fields, which contradicts the original behavior. py/selenium/webdriver/common/bidi/browser.py [150-160] x = data.get("x") -if not isinstance(x, int): +if x is not None and not isinstance(x, int): raise ValueError("x must be an integer") y = data.get("y") -if not isinstance(y, int): +if y is not None and not isinstance(y, int): raise ValueError("y must be an integer") active = data.get("active") -if not isinstance(active, bool): +if active is not None and not isinstance(active, bool): raise ValueError("active must be a boolean") Suggestion importance[1-10]: 8 __ Why: Important fix that prevents validation errors for legitimately missing optional fields. The current code would incorrectly raise ValueError when optional fields are absent.	Medium
	✅ Consistent field access pattern Suggestion Impact: The commit implemented exactly what was suggested: changed data.get('height') to data['height'] and added validation to ensure height is a non-negative integer with improved error message code diff: - height = data.get("height") - if not isinstance(height, int): - raise ValueError("height must be an integer") + height = data["height"] + if not isinstance(height, int) or height < 0: + raise ValueError(f"height must be a non-negative integer, got {height}") Use data["height"] instead of data.get("height") for consistency with other required fields. The current implementation will accept None values, which contradicts the validation check. py/selenium/webdriver/common/bidi/browser.py [146-148] -height = data.get("height") -if not isinstance(height, int): - raise ValueError("height must be an integer") +height = data["height"] +if not isinstance(height, int) or height < 0: + raise ValueError(f"height must be a non-negative integer, got {height}") [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: Good suggestion for consistency with required fields like width and state, and adding non-negative validation improves robustness of the validation logic.	Medium

✅ Suggestions up to commit 11e8805

Category	Suggestion	Impact
Possible issue	✅ Check for missing fields Suggestion Impact: The commit partially addressed the suggestion by changing data.get('state') to data['state'], which will raise a KeyError if the state field is missing rather than returning None. This achieves a similar goal as the suggestion but uses a different approach. The commit also added validation for state values. code diff: - state = data.get("state") + state = data["state"] if not isinstance(state, str): raise ValueError("state must be a string") + if state not in ClientWindowState.VALID_STATES: + raise ValueError(f"Invalid state: {state}. Must be one of {ClientWindowState.VALID_STATES}") The code uses data.get() which returns None for missing keys, but then checks types without checking for None values. This could lead to misleading error messages when fields are missing versus when they have incorrect types. py/selenium/webdriver/common/bidi/browser.py [131-146] try: client_window = data.get("clientWindow") + if client_window is None: + raise ValueError("clientWindow is required") if not isinstance(client_window, str): raise ValueError("clientWindow must be a string") state = data.get("state") + if state is None: + raise ValueError("state is required") if not isinstance(state, str): raise ValueError("state must be a string") width = data.get("width") + if width is None: + raise ValueError("width is required") if not isinstance(width, int): raise ValueError("width must be an integer") height = data.get("height") + if height is None: + raise ValueError("height is required") if not isinstance(height, int): raise ValueError("height must be an integer") [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that data.get() returns None for missing keys, and the current code would give misleading error messages. Adding explicit None checks provides clearer distinction between missing and invalid type errors.	Medium
General	Fix incorrect exception handling The except (KeyError, TypeError) block will never catch KeyError exceptions since data.get() returns None for missing keys rather than raising KeyError. This makes the error handling inconsistent. py/selenium/webdriver/common/bidi/browser.py [169-170] -except (KeyError, TypeError) as e: +except TypeError as e: raise ValueError(f"Invalid data format for ClientWindowInfo: {e}") Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that KeyError will never be raised since data.get() returns None instead of raising exceptions. Removing unnecessary exception handling improves code clarity.	Low

[rpallavisharma]

Requesting kindly @cgoldberg for review.

[rpallavisharma]

@navin772 , kindly please check if its fine now. Thank you for the suggestions.

[rpallavisharma]

@navin772 -- running bazel command gives this error - ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/py/selenium/webdriver/common/devtools/v135/pycache/animation.cpython-39.pyc' (belonging to //py:selenium) and 'bazel-out/darwin_arm64-fastbuild/bin/py/selenium/webdriver/common/devtools/v135' (belonging to //py:create-cdp-srcs-v135) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
what does this means?

[cgoldberg]

@pallavigitwork I get the same error (and have for months) and pretty much can't use bazel for anything except the initial build. I use PyTest directly for running tests.

[rpallavisharma]

ok @cgoldberg thank you, i will do the same.

[navin772]

@pallavigitwork yes, I recently started getting that error (don't know how to fix it yet), but a temporary workaround would be to change the outdir here (Corey suggested this in slack) to something else like:

outdir = "selenium/webdriver/common/devtools/1/{}".format(devtools_version),

Note the 1/ after devtools. Or just use pytest.

[rpallavisharma]

ok @navin772 thank you.

[rpallavisharma]

@navin772 please kindly also let me know what does RBE stands for. sorry if it sounds silly. im not aware of this abbreviation.

[navin772]

@pallavigitwork RBE stands for Remote Build Execution. Even I got to know it few months back!

[rpallavisharma]

Thank you Navin for all the help.

[cgoldberg]

@pallavigitwork LGTM .... Go ahead and merge it. If you don't have access, let me know and I'll do it. Thanks!

[rpallavisharma]

Thank you @navin772
Thank you @cgoldberg
No I don't have merge access. Kindly merge.