Enable IndexedDB inspection in workers [chromium/src : main]
Wang Neden (Gerrit)
Wang Neden voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
| Commit-Queue | +2 |
1 comment
Hi Yang Guo,
This is the backend/native part of a change to enable storage inspection (like IndexedDB) for service workers, with a focus on Chrome Extensions.
The main logic here is refactoring InspectorIndexedDBAgent to operate on ExecutionContext and attaching the necessary protocol handlers for worker targets.
The corresponding frontend changes are in a separate CL in the devtools-frontend repository here:
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6917118
Any feedback on the backend changes in this CL would be greatly appreciated.
Thanks a lot!
Related details
- Yang Guo
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Thanks! Could you please add tests for the IndexedDB domain for workers (service/shared/dedicated) to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/http/tests/inspector-protocol/?
Related details
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang Neden (Gerrit)
Wang Neden voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
| Commit-Queue | +2 |
1 comment
Hi Alex,
This patchset implements the backend support for the new `Storage.getStorageKey` CDP command, which is a key step towards enabling storage inspection features (like IndexedDB) for worker targets.
**Summary of Changes:**
* **New CDP Command:** The new `Storage.getStorageKey(optional frameId)` command is implemented in `StorageHandler`. The old `getStorageKeyForFrame` is now officially deprecated in the PDL.
* **Web Tests:** As you suggested, I've added a suite of new web tests to validate the functionality:
* Three tests specifically verify that `Storage.getStorageKey` returns the correct key for dedicated, shared, and service workers.
* Three more comprehensive tests confirm that the full IndexedDB protocol (`requestDatabaseNames`, `requestData`, etc.) now works correctly when targeting each of the three worker types.
The private helper approach for `GetStorageKeyForFrame` maintains code quality while the deprecated public method ensures backward compatibility for now. I've also added a `TODO` pointing to the follow-up bug for its eventual removal.
Looking forward to your feedback!
Thanks
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 4 comments![]( "Open in Gerrit")
Thanks, I think this looks good. I had a few comments and I will start the CQ to see if tests pass before doing another review pass.
session->CreateAndAddHandler<protocol::StorageHandler>(GetId(),let's pass the host itself like it is done in the tracing handler below? This should allow us to avoid look ups by id.
explicit StorageHandler(const std::string& host_id,If passing the host itself would not work for some reason, let's rename host_id to target_id as in other handlers such as TargetHandler.
command getStorageKeyForFrame```suggestion
deprecated command getStorageKeyForFrame
```
Related details
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang Neden (Gerrit)
Wang Neden voted and added 4 comments![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
| Commit-Queue | +2 |
4 comments
Hi Alex,
Thanks a lot for your detailed feedback!
I've updated the patch to incorporate your suggestions:
1. The `StorageHandler` now receives the `DevToolsAgentHostImpl*` directly, removing the need for an ID lookup.
2. `getStorageKeyForFrame` has been marked as `deprecated`.
Regarding the web tests, the trybot failures you may have seen were caused by a pre-existing object lifetime issue. That issue has now been resolved in the separate CL you recently reviewed (https://chromium-review.googlesource.com/c/chromium/src/+/6965169), and the tests in this patch have been updated to align with that fix.
The corresponding DevTools frontend change has also been updated to keep everything in sync.
Thanks again for your guidance!
session->CreateAndAddHandler<protocol::StorageHandler>(GetId(),let's pass the host itself like it is done in the tracing handler below? This should allow us to avoid look ups by id.
Done
If passing the host itself would not work for some reason, let's rename host_id to target_id as in other handlers such as TargetHandler.
Done
command getStorageKeyForFrameWang Neden```suggestion
deprecated command getStorageKeyForFrame
```
Done
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
deprecated command getStorageKeyForFramelet's update existing tests to use the new command for frames https://source.chromium.org/search?q=getStorageKeyForFrame&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Fweb_tests%2Fhttp%2Ftests%2Finspector-protocol%2Fstorage%2F Otherwise, I think the frameId in the new method has no test coverage?
Related details
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
deprecated command getStorageKeyForFramelet's update existing tests to use the new command for frames https://source.chromium.org/search?q=getStorageKeyForFrame&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Fweb_tests%2Fhttp%2Ftests%2Finspector-protocol%2Fstorage%2F Otherwise, I think the frameId in the new method has no test coverage?
Although I guess it could be done when the deprecated method is removed.
Wang Neden (Gerrit)
Wang Neden voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
| Commit-Queue | +2 |
1 comment
Alex Rudenkolet's update existing tests to use the new command for frames https://source.chromium.org/search?q=getStorageKeyForFrame&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Fweb_tests%2Fhttp%2Ftests%2Finspector-protocol%2Fstorage%2F Otherwise, I think the frameId in the new method has no test coverage?
Although I guess it could be done when the deprecated method is removed.
That's a great point. You're right, it's much better to update the tests now to ensure the `frameId` parameter in the new `getStorageKey` command has proper test coverage.
I've updated the remaining web tests to use the new command as you suggested.
Thanks for catching that! The patch is ready for another look.
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
- Danil Somsikov
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |