User description
🔗 Related Issues
💥 What does this PR do?
Adds getData, addDataCollector and removeDataCollector commands as in BiDi spec:
- w3c.github.io/webdriver-bidi#command-network-addDataCollector
- w3c.github.io/webdriver-bidi#command-network-getData
- w3c.github.io/webdriver-bidi#command-network-removeDataCollector
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes
- New feature (non-breaking change which adds functionality and tests!)
PR Type
Enhancement
Description
-
Add BiDi network data collection commands per W3C spec
-
Implement addDataCollector, removeDataCollector, and getData methods
-
Create parameter classes for data collection configuration
-
Add comprehensive test coverage for all new functionality
Diagram Walkthrough
flowchart LR
A["Network Module"] --> B["addDataCollector"]
A --> C["removeDataCollector"]
A --> D["getData"]
B --> E["AddDataCollectorParameters"]
D --> F["GetDataParameters"]
D --> G["BytesValue"]
E --> H["DataType enum"]
F --> H
File Walkthrough
| Relevant files |
|---|
| Enhancement |
Network.javaAdd network data collection methods
java/src/org/openqa/selenium/bidi/module/Network.java
- Add three new methods:
addDataCollector, removeDataCollector, getData
- Import new parameter classes and JSON handling utilities
- Implement result mapping for
getData command response
|
+44/-0 |
AddDataCollectorParameters.javaCreate AddDataCollectorParameters class
java/src/org/openqa/selenium/bidi/network/AddDataCollectorParameters.java
- New parameter class for
addDataCollector command
- Support for data types, max size, collector type, contexts
- Builder pattern with fluent API and validation
|
+78/-0 |
DataType.javaAdd DataType enum
java/src/org/openqa/selenium/bidi/network/DataType.java
- New enum defining supported data types
- Currently supports RESPONSE data type
|
+33/-0 |
GetDataParameters.javaCreate GetDataParameters class
java/src/org/openqa/selenium/bidi/network/GetDataParameters.java
- New parameter class for
getData command
- Support for data type, request ID, collector, disown flag
- Builder pattern with validation
|
+59/-0 |
|
| Tests |
NetworkCommandsTest.javaAdd data collection command tests
java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java
- Add comprehensive tests for all new data collection methods
- Test various parameter combinations and edge cases
- Verify error handling for invalid scenarios
|
+157/-0 |
|
PR Reviewer Guide 🔍
Here are some key observations to aid the review process:
|
🎫 Ticket compliance analysis 🔶
5678 - Partially compliant
Compliant requirements:
Non-compliant requirements:
- Investigate and resolve ChromeDriver "ConnectFailure" on repeated instantiation.
- Reproduce and address environment-specific issue.
- Provide fix or guidance for the error scenario.
Requires further human verification:
- Validation on the specified OS/Chrome/ChromeDriver/Selenium versions in a real environment.
- Confirmation that the error no longer appears across multiple driver instantiations.
1234 - Partially compliant
Compliant requirements:
Non-compliant requirements:
- Ensure click() triggers href JavaScript in Firefox as in 2.47.1.
Requires further human verification:
- Manual/browser testing across Firefox versions to confirm behavior.
|
| ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪ |
| 🧪 PR contains tests |
| 🔒 No security concerns identified |
⚡ Recommended focus areas for review
Result Mapping
The getData result mapping uses a manual Map read and a second JsonInput to parse BytesValue. Verify schema stability and that "bytes" is always present; consider direct parsing or null-safety to avoid NPE if backend omits "bytes".
private final Function<JsonInput, BytesValue> getDataResultMapper =
jsonInput -> {
Map<String, Object> result = jsonInput.read(Map.class);
@SuppressWarnings("unchecked")
Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes");
try (StringReader reader = new StringReader(JSON.toJson(bytesMap));
JsonInput bytesInput = JSON.newInput(reader)) {
return BytesValue.fromJson(bytesInput);
}
};
Validation
Only maxEncodedDataSize is validated; allow-empty checks for dataTypes and collectorType values are minimal. Consider enforcing non-empty dataTypes and validating collectorType against supported values to prevent server errors.
public AddDataCollectorParameters(List<DataType> dataTypes, long maxEncodedDataSize) {
Require.nonNull("Data types", dataTypes);
if (maxEncodedDataSize <= 0) {
throw new IllegalArgumentException("Max encoded data size must be positive");
}
dataTypes.forEach(dataType -> this.dataTypes.add(dataType.toString()));
this.maxEncodedDataSize = maxEncodedDataSize;
}
public AddDataCollectorParameters collectorType(String collectorType) {
this.collectorType = Require.nonNull("Collector type", collectorType);
return this;
}
Defaults/Flags
The "disown" flag is always included (defaults false). Confirm server tolerates explicit false; if optional, consider omitting when false. Also ensure request and collector IDs format are validated upstream.
public Map<String, Object> toMap() {
Map<String, Object> map = new HashMap<>();
map.put("dataType", dataType.toString());
map.put("request", request);
if (collector != null) {
map.put("collector", collector);
}
map.put("disown", disown);
return Map.copyOf(map);
}
|
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | Impact |
| High-level |
✅ Simplify the JSON parsing logic
Suggestion Impact:The commit removed the Json/StringReader round-trip and refactored getDataResultMapper to iterate the JsonInput object directly, reading the "bytes" field via BytesValue.fromJson(jsonInput) and skipping others.
code diff:
@@ -71,14 +67,18 @@
private final Function<JsonInput, BytesValue> getDataResultMapper =
jsonInput -> {
- Map<String, Object> result = jsonInput.read(Map.class);
- @SuppressWarnings("unchecked")
- Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes");
-
- try (StringReader reader = new StringReader(JSON.toJson(bytesMap));
- JsonInput bytesInput = JSON.newInput(reader)) {
- return BytesValue.fromJson(bytesInput);
+ jsonInput.beginObject();
+ BytesValue bytes = null;
+ while (jsonInput.hasNext()) {
+ String name = jsonInput.nextName();
+ if ("bytes".equals(name)) {
+ bytes = BytesValue.fromJson(jsonInput);
+ } else {
+ jsonInput.skipValue();
+ }
}
+ jsonInput.endObject();
+ return bytes;
};
The JSON parsing for the getData command in Network.java is inefficient due to a
redundant serialize-deserialize cycle. Refactor this to process the nested JSON
map directly.
Examples:
java/src/org/openqa/selenium/bidi/module/Network.java [72-82]
private final Function<JsonInput, BytesValue> getDataResultMapper =
jsonInput -> {
Map<String, Object> result = jsonInput.read(Map.class);
@SuppressWarnings("unchecked")
Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes");
try (StringReader reader = new StringReader(JSON.toJson(bytesMap));
JsonInput bytesInput = JSON.newInput(reader)) {
return BytesValue.fromJson(bytesInput);
}
... (clipped 1 lines)
Solution Walkthrough:
Before:
private final Function<JsonInput, BytesValue> getDataResultMapper =
jsonInput -> {
Map<String, Object> result = jsonInput.read(Map.class);
Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes");
try (StringReader reader = new StringReader(JSON.toJson(bytesMap));
JsonInput bytesInput = JSON.newInput(reader)) {
return BytesValue.fromJson(bytesInput);
}
};
After:
private final Function<JsonInput, BytesValue> getDataResultMapper =
jsonInput -> {
jsonInput.beginObject();
while (jsonInput.hasNext()) {
if ("bytes".equals(jsonInput.nextName())) {
return BytesValue.fromJson(jsonInput);
} else {
jsonInput.skipValue();
}
}
jsonInput.endObject();
// Or throw an exception if "bytes" is not found
return null;
};
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies an inefficient JSON processing pattern and proposes a valid simplification, which improves performance and code clarity.
| Medium
|
| Possible issue |
Validate against an empty list
In the AddDataCollectorParameters constructor, add a validation check to ensure
the dataTypes list is not empty, as required by the BiDi specification.
java/src/org/openqa/selenium/bidi/network/AddDataCollectorParameters.java [34-42]
public AddDataCollectorParameters(List<DataType> dataTypes, long maxEncodedDataSize) {
Require.nonNull("Data types", dataTypes);
+ Require.nonNull("Max encoded data size", maxEncodedDataSize);
+ if (dataTypes.isEmpty()) {
+ throw new IllegalArgumentException("Data types list must not be empty");
+ }
if (maxEncodedDataSize <= 0) {
throw new IllegalArgumentException("Max encoded data size must be positive");
}
dataTypes.forEach(dataType -> this.dataTypes.add(dataType.toString()));
this.maxEncodedDataSize = maxEncodedDataSize;
}
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a missing validation for an empty dataTypes list, which improves API robustness by enforcing a contract from the BiDi specification.
| Medium
|
| General |
✅ Improve performance by avoiding re-serialization
Suggestion Impact:The commit replaced Map-based parsing and re-serialization with streaming parsing using JsonInput.beginObject/hasNext/nextName/skipValue and directly invoked BytesValue.fromJson, eliminating JSON re-serialization and removing Json/StringReader usage.
code diff:
@@ -71,14 +67,18 @@
private final Function<JsonInput, BytesValue> getDataResultMapper =
jsonInput -> {
- Map<String, Object> result = jsonInput.read(Map.class);
- @SuppressWarnings("unchecked")
- Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes");
-
- try (StringReader reader = new StringReader(JSON.toJson(bytesMap));
- JsonInput bytesInput = JSON.newInput(reader)) {
- return BytesValue.fromJson(bytesInput);
+ jsonInput.beginObject();
+ BytesValue bytes = null;
+ while (jsonInput.hasNext()) {
+ String name = jsonInput.nextName();
+ if ("bytes".equals(name)) {
+ bytes = BytesValue.fromJson(jsonInput);
+ } else {
+ jsonInput.skipValue();
+ }
}
+ jsonInput.endObject();
+ return bytes;
};
Refactor getDataResultMapper to use the JsonInput streaming API, avoiding
inefficient intermediate object creation and JSON re-serialization when parsing
the response.
java/src/org/openqa/selenium/bidi/module/Network.java [72-82]
private final Function<JsonInput, BytesValue> getDataResultMapper =
jsonInput -> {
- Map<String, Object> result = jsonInput.read(Map.class);
- @SuppressWarnings("unchecked")
- Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes");
-
- try (StringReader reader = new StringReader(JSON.toJson(bytesMap));
- JsonInput bytesInput = JSON.newInput(reader)) {
- return BytesValue.fromJson(bytesInput);
+ BytesValue bytesValue = null;
+ jsonInput.beginObject();
+ while (jsonInput.hasNext()) {
+ if ("bytes".equals(jsonInput.nextName())) {
+ bytesValue = BytesValue.fromJson(jsonInput);
+ } else {
+ jsonInput.skipValue();
+ }
}
+ jsonInput.endObject();
+ return bytesValue;
};
[Suggestion processed]
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies an inefficient JSON parsing pattern and proposes a more performant, streaming-based approach which improves code quality and efficiency.
| Low
|
|
| |
So, data collection doesn't make sense outside the context of an interception. You have to pass a request id to get the data, which is only accessible from a handler.
I think this should all be private API. When you register a handler you specify if you need the body, and that method registers the collector and manages it in the background.
I think this should all be private API. When you register a handler you specify if you need the body, and that method registers the collector and manages it in the background.
@titusfortner should we overload the event handlers to something like this and manage the collector inside the event handler?:
public void onBeforeRequestSent(Consumer<BeforeRequestSent> consumer, boolean captureBody)
Also, will this be applicable to only these events:
onBeforeRequestSentonResponseCompletedonResponseStarted
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}
