fixed errors in browser.py for 15697 #15847

pallavigitwork · 2025-06-03T05:43:04Z

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-merge-pro · 2025-06-03T05:43:28Z

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-merge-pro · 2025-06-03T05:43:51Z

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
Possible issue	✅ ~~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

pallavigitwork · 2025-06-03T06:18:08Z

Requesting kindly @cgoldberg for review.

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

Co-authored-by: Navin Chandra <navinchandra772@gmail.com>

pallavigitwork · 2025-06-03T15:00:46Z

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

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

pallavigitwork · 2025-06-03T15:11:44Z

@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 · 2025-06-03T15:15:31Z

@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.

pallavigitwork · 2025-06-03T15:16:40Z

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

navin772 · 2025-06-03T15:20:13Z

@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.

pallavigitwork · 2025-06-03T15:23:15Z

ok @navin772 thank you.

pallavigitwork · 2025-06-03T15:39:39Z

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

navin772 · 2025-06-03T15:44:26Z

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

pallavigitwork · 2025-06-03T15:45:36Z

:) ok thank you @navin772

pallavigitwork · 2025-06-03T18:46:51Z

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!

pallavigitwork · 2025-06-04T01:29:24Z

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

fixed errors in browser.py for 15697

11e8805

selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jun 3, 2025

qodo-merge-pro bot added the Review effort 2/5 label Jun 3, 2025

navin772 requested changes Jun 3, 2025

View reviewed changes

navin772 mentioned this pull request Jun 3, 2025

[py] Fix : Mypy type annotation errors - 2 #15848

Merged

pallavigitwork and others added 7 commits June 3, 2025 06:26

Update browser.py

67f2a9e

Co-authored-by: Navin Chandra <navinchandra772@gmail.com>

Update browser.py

6a87672

Co-authored-by: Navin Chandra <navinchandra772@gmail.com>

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

9c04377

Co-authored-by: Navin Chandra <navinchandra772@gmail.com>

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

ac52d2d

Co-authored-by: Navin Chandra <navinchandra772@gmail.com>

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

7b87c13

Co-authored-by: Navin Chandra <navinchandra772@gmail.com>

Merge branch 'trunk' into Fix15697-1-pal

ecc8cec

modified file as per suggestions by Navin

4d1dcc7

pallavigitwork requested a review from navin772 June 3, 2025 14:59

pallavigitwork requested a review from cgoldberg June 3, 2025 15:01

navin772 requested changes Jun 3, 2025

View reviewed changes

py/selenium/webdriver/common/bidi/browser.py Outdated Show resolved Hide resolved

py/selenium/webdriver/common/bidi/browser.py Outdated Show resolved Hide resolved

modified file as per suggestions by Navin

3b9617f

pallavigitwork requested a review from navin772 June 3, 2025 15:28

fixed get error

8fcf232

navin772 approved these changes Jun 3, 2025

View reviewed changes

cgoldberg approved these changes Jun 3, 2025

View reviewed changes

cgoldberg merged commit 4b7f476 into SeleniumHQ:trunk Jun 4, 2025
16 checks passed

pallavigitwork deleted the Fix15697-1-pal branch August 13, 2025 08:34

Uh oh!

fixed errors in browser.py for 15697 #15847

fixed errors in browser.py for 15697 #15847

Uh oh!

Conversation

pallavigitwork commented Jun 3, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-merge-pro bot commented Jun 3, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

cgoldberg commented Jun 3, 2025

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

navin772 commented Jun 3, 2025

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

navin772 commented Jun 3, 2025

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

pallavigitwork commented Jun 3, 2025

Uh oh!

cgoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

pallavigitwork commented Jun 4, 2025

Uh oh!

Uh oh!

Uh oh!

pallavigitwork commented Jun 3, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Jun 3, 2025 •

edited

Loading