[Persist] Simplify LoadTab method for NodeIdMapper [chromium/src : main]
Fiaz Muhammad (Gerrit)
Fiaz Muhammad voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Calder Kitagawa
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiaz Muhammad (Gerrit)
Message from Fiaz Muhammad![]( "Open in Gerrit")
- Calder Kitagawa
- Fiaz Muhammad
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiaz Muhammad (Gerrit)
Fiaz Muhammad added 1 comment![]( "Open in Gerrit")
I had this in another branch since I thought the earlier CL would have been merged. I should have probably just combined the two CLs.
Related details
- Calder Kitagawa
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
bool StorageNodeIdMapperAndroid::LoadTab(const TabInterface* tab) {
const TabAndroid* tab_android = static_cast<const TabAndroid*>(tab);
if (tab_id_to_callback_.contains(tab_android->GetAndroidId())) {
return false;
}
auto map_entry = tab_id_to_callback_.extract(tab_android->GetAndroidId());
DCHECK(!map_entry.empty());
std::move(map_entry.mapped()).Run(tab);
return true;
}Whoa are we using TabAndroid or [TabInterfaceAndroid](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/android/tab_interface_android.h;drc=f49e23d8e2bd190b42ec62284b8be10dcccd0446;l=19) here?
I'm a bit confused.
TabStripCollection hosts tabs as `TabInterfaceAndroid` instances (weak ptr to TabAndroid) to avoid the problem of memory management. I thought all our APIs were going to use TabAndroid directly or TabInterfaceAndroid when coming from a collection. This static cast has me concerned because it is now unclear which type we are using in interfaces that tab TabInterface* as an input.
Related details
- Fiaz Muhammad
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiaz Muhammad (Gerrit)
Fiaz Muhammad added 1 comment![]( "Open in Gerrit")
bool StorageNodeIdMapperAndroid::LoadTab(const TabInterface* tab) {
const TabAndroid* tab_android = static_cast<const TabAndroid*>(tab);
if (tab_id_to_callback_.contains(tab_android->GetAndroidId())) {
return false;
}
auto map_entry = tab_id_to_callback_.extract(tab_android->GetAndroidId());
DCHECK(!map_entry.empty());
std::move(map_entry.mapped()).Run(tab);
return true;
}Whoa are we using TabAndroid or [TabInterfaceAndroid](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/android/tab_interface_android.h;drc=f49e23d8e2bd190b42ec62284b8be10dcccd0446;l=19) here?
I'm a bit confused.
TabStripCollection hosts tabs as `TabInterfaceAndroid` instances (weak ptr to TabAndroid) to avoid the problem of memory management. I thought all our APIs were going to use TabAndroid directly or TabInterfaceAndroid when coming from a collection. This static cast has me concerned because it is now unclear which type we are using in interfaces that tab TabInterface* as an input.
Thanks for the clarification, that is really helpful. I know you mentioned it earlier offline, but this makes it a lot clearer.
I think in cases that will only be used by TabCollection-sourced tabs, such as this, we have passed the original TabInterface object to the service. I'm assuming this is a TabInterfaceAndroid\*. Maybe we should cast it to a TabAndroid\* before passing it?
Our service gets called with TabAndroid\*, such as when we call the Java class. We attempted to keep this consistent across the board. Maybe I could pass a standardizer to the service (from the factory). On android, this would perform casts to TabAndroid\*, and this would guarantee that we are consistent when it comes to handles. Thoughts?
Related details
- Calder Kitagawa
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiaz Muhammad (Gerrit)
Fiaz Muhammad added 1 comment![]( "Open in Gerrit")
bool StorageNodeIdMapperAndroid::LoadTab(const TabInterface* tab) {
const TabAndroid* tab_android = static_cast<const TabAndroid*>(tab);
if (tab_id_to_callback_.contains(tab_android->GetAndroidId())) {
return false;
}
auto map_entry = tab_id_to_callback_.extract(tab_android->GetAndroidId());
DCHECK(!map_entry.empty());
std::move(map_entry.mapped()).Run(tab);
return true;
}Fiaz MuhammadWhoa are we using TabAndroid or [TabInterfaceAndroid](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/android/tab_interface_android.h;drc=f49e23d8e2bd190b42ec62284b8be10dcccd0446;l=19) here?
I'm a bit confused.
TabStripCollection hosts tabs as `TabInterfaceAndroid` instances (weak ptr to TabAndroid) to avoid the problem of memory management. I thought all our APIs were going to use TabAndroid directly or TabInterfaceAndroid when coming from a collection. This static cast has me concerned because it is now unclear which type we are using in interfaces that tab TabInterface* as an input.
Thanks for the clarification, that is really helpful. I know you mentioned it earlier offline, but this makes it a lot clearer.
I think in cases that will only be used by TabCollection-sourced tabs, such as this, we have passed the original TabInterface object to the service. I'm assuming this is a TabInterfaceAndroid\*. Maybe we should cast it to a TabAndroid\* before passing it?
Our service gets called with TabAndroid\*, such as when we call the Java class. We attempted to keep this consistent across the board. Maybe I could pass a standardizer to the service (from the factory). On android, this would perform casts to TabAndroid\*, and this would guarantee that we are consistent when it comes to handles. Thoughts?
Here's a proof-of-concept. Let me know if it isn't correct: https://crrev.com/c/7125058
I use a pretty heavy assumption that TabInterface\* can be cast to TabAndroid\* in Android.