Maintain an in memory map of handles to storage ids. [chromium/src : main]
0 views
Skip to first unread message
Sky Malice (Gerrit)
Sep 18, 2025, 6:38:38 PM (8 days ago) Sep 18
to Fiaz Muhammad, chromium...@chromium.org, jinsukk...@chromium.org
Attention needed from Fiaz Muhammad
Sky Malice added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Sky Malice . resolved
PTAL.
As I mentioned on chat, I'm realizing this id mapping doesn't work during a shadow write mode. This kind of breaks most all of the ideas.
Related details
Attention is currently required from:
- Fiaz Muhammad
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiaz Muhammad (Gerrit)
Sep 18, 2025, 7:17:41 PM (8 days ago) Sep 18
to Sky Malice, chromium...@chromium.org, jinsukk...@chromium.org
Attention needed from Sky Malice
Fiaz Muhammad added 2 comments![]( "Open in Gerrit")
File chrome/browser/tab/tab_state_storage_service.cc
Line 97, Patchset 2 (Latest):
for (const auto& child : tab_collection->GetChildren(pass_key_)) {Fiaz Muhammad . unresolved
This might be an eh suggestion, but I think our service probably shouldn't be doing the iteration itself.
I think our tree crawling should probably be done by a class that exposes it. Maybe a flat iterator that only exposes the direct children (ie no children of children) at the current level?
File components/tabs/public/tab_collection.h
Line 222, Patchset 2 (Latest):
base::PassKey<TabStateStorageService> pass_key) const {Fiaz Muhammad . unresolved
Is there any way we can combine these via a shared `CollectionCrawler` class or something?
Related details
Attention is currently required from:
- Sky Malice
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiaz Muhammad (Gerrit)
Sep 18, 2025, 7:27:50 PM (8 days ago) Sep 18
to Sky Malice, chromium...@chromium.org, jinsukk...@chromium.org
Attention needed from Sky Malice
Fiaz Muhammad added 1 comment![]( "Open in Gerrit")
File chrome/browser/tab/tab_state_storage_service.h
Line 68, Patchset 2 (Latest):
base::PassKey<TabStateStorageService> pass_key_{};Fiaz Muhammad . unresolved
Same here. I still feel like we shouldn't keep this here, and this should be managed by another object
Related details
Attention is currently required from:
- Sky Malice
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Malice (Gerrit)
Sep 25, 2025, 12:15:24 PM (yesterday) Sep 25
to Calder Kitagawa, Chromium LUCI CQ, Fiaz Muhammad, chromium...@chromium.org, jinsukk...@chromium.org
Attention needed from Calder Kitagawa and Fiaz Muhammad
Sky Malice added 3 comments![]( "Open in Gerrit")
File chrome/browser/tab/tab_state_storage_service.h
Same here. I still feel like we shouldn't keep this here, and this should be managed by another object
Sky Malice
Done
File chrome/browser/tab/tab_state_storage_service.cc
Line 97, Patchset 2:
for (const auto& child : tab_collection->GetChildren(pass_key_)) {Fiaz Muhammad . resolved
This might be an eh suggestion, but I think our service probably shouldn't be doing the iteration itself.
I think our tree crawling should probably be done by a class that exposes it. Maybe a flat iterator that only exposes the direct children (ie no children of children) at the current level?
Sky Malice
Done
File components/tabs/public/tab_collection.h
Line 222, Patchset 2:
base::PassKey<TabStateStorageService> pass_key) const {Fiaz Muhammad . resolved
Is there any way we can combine these via a shared `CollectionCrawler` class or something?
Attention is currently required from:
- Calder Kitagawa
- Fiaz Muhammad
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Sep 25, 2025, 12:35:44 PM (yesterday) Sep 25
to Sky Malice, Chromium LUCI CQ, Fiaz Muhammad, chromium...@chromium.org, jinsukk...@chromium.org
Attention needed from Fiaz Muhammad and Sky Malice
Calder Kitagawa voted and added 3 comments![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
3 comments
Patchset-level comments
File chrome/browser/tab/protocol/children.proto
Line 12, Patchset 3 (Latest):
repeated int32 storage_id = 1;Calder Kitagawa . unresolved
A comment on here for what this represents and how it is generated might be helpful (both now and asiprationational).
File chrome/browser/tab/tab_state_storage_service.cc
Line 114, Patchset 3 (Latest):
next_storage_id_ = max_id + 1;Calder Kitagawa . unresolved
This might also need to change right? Add TODO?
Related details
Attention is currently required from:
- Fiaz Muhammad
- Sky Malice
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Malice (Gerrit)
Sep 25, 2025, 12:50:31 PM (yesterday) Sep 25
to Calder Kitagawa, Chromium LUCI CQ, Fiaz Muhammad, chromium...@chromium.org, jinsukk...@chromium.org
Attention needed from Fiaz Muhammad
Sky Malice voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sky Malice
| Commit-Queue | +2 |
2 comments
File chrome/browser/tab/protocol/children.proto
A comment on here for what this represents and how it is generated might be helpful (both now and asiprationational).
Sky Malice
Done
File chrome/browser/tab/tab_state_storage_service.cc
This might also need to change right? Add TODO?
Sky Malice
Totally needs to change. Ideally we stop dealing with TabStates entirely, and just deal with collections.
Related details
Attention is currently required from:
- Fiaz Muhammad
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Sep 25, 2025, 1:23:54 PM (yesterday) Sep 25
to Sky Malice, Calder Kitagawa, Fiaz Muhammad, chromium...@chromium.org, jinsukk...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/tab/tab_state_storage_service.cc
Insertions: 4, Deletions: 0.
@@ -111,6 +111,10 @@
}
}
}
+ // TODO(https://crbug.com/427254267): While this avoids collisions and does
+ // set up a mapping for live Tab objects from Java, doesn't interact nicely
+ // with collections objects mappings. Ideally this will be fully replaced as
+ // soon as we can create and pass up collections objects on start up.
next_storage_id_ = max_id + 1;
std::move(callback).Run(std::move(tab_states));
}
```
```
The name of the file: chrome/browser/tab/protocol/children.proto
Insertions: 5, Deletions: 0.
@@ -9,5 +9,10 @@
option java_package = "org.chromium.chrome.browser.tabs.proto";
message Children {
+ // Used to serialize a list of ids into a single db blob cell. The storage id
+ // is created by the storage system and used to map in memory objects to rows
+ // in the database. Note that before this system is authoratative for reads,
+ // this mapping cannot be recreated on start up, and will require rebuilding
+ // (and deletion of the whole db) to fix.
repeated int32 storage_id = 1;
}
```
Change information
Commit message:
[Persist] Maintain an in memory map of handles to storage ids.
Bug: 427254267
Change-Id: I1882c7b84e747531e046509461f67cd6c0c400e6
Commit-Queue: Sky Malice <sk...@chromium.org>
Reviewed-by: Calder Kitagawa <ckit...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1520708}
Files:
- M chrome/browser/tab/protocol/BUILD.gn
- A chrome/browser/tab/protocol/children.proto
- M chrome/browser/tab/tab_state_storage_service.cc
- M chrome/browser/tab/tab_state_storage_service.h
Change size: M
Delta: 4 files changed, 109 insertions(+), 2 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Calder Kitagawa
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Malice (Gerrit)
2:02 AM (12 hours ago) 2:02 AM
to Chromium LUCI CQ, Calder Kitagawa, Fiaz Muhammad, chromium...@chromium.org, jinsukk...@chromium.org
Sky Malice has created a revert of this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages