[Extensions] Port chrome.tabs.discard to desktop Android [chromium/src : main]
Kelvin Jiang (Gerrit)
Kelvin Jiang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keigo Oka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 5 comments![]( "Open in Gerrit")
Overall shape looks good. A couple comments.
BrowserWindowInterface* browser_window =nit: Check browser_window for null before using it. On ChromeOS it can be null for unusual window types. See https://chromium-review.googlesource.com/c/chromium/src/+/7596693
Related details
- Darryl James
- Kelvin Jiang
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. |
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 5 comments![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Commit-Queue | +1 |
5 comments
That... was an interesting test flake
"api/tabs/tabs_api_android.cc",Kelvin JiangHooray!
Acknowledged
"api/tabs/tabs_api_non_android.cc",Kelvin JiangHooray!
Acknowledged
nit: Check browser_window for null before using it. On ChromeOS it can be null for unusual window types. See https://chromium-review.googlesource.com/c/chromium/src/+/7596693
- Darryl James
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook voted and added 2 comments![]( "Open in Gerrit")
Votes added by James Cook
| Code-Review | +1 |
2 comments
LGTM
TabListInterface* tab_list = TabListInterface::From(browser_window);Sorry I didn't notice before... I wonder if we need to check tab_list for null as well. WDYT?
Related details
- Darryl James
- Kelvin Jiang
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. |
Darryl James (Gerrit)
Darryl James added 2 comments![]( "Open in Gerrit")
tab_object.discarded = contents->WasDiscarded();Is it right that ENABLE_EXTENSIONS is true for desktop Android? I have a feeling that desktop Android can take the top path as well.
return performance_manager::policies::PageDiscardingHelper::GetFromGraph(
graph)
->DiscardAPage(discard_reason, urgent_protection_time)
.first_content_after_discard;What are your thoughts on splitting this call up to avoid nullptr dereferences?
Ex:
```
if (!GetFromGraph()) {
return nullptr;
}
if (!DiscardAPage()) {
return nullptr;
}return first_content_after_discard;
```
It could be the case that these will always return a value, in that case disregard this comment 👍
Related details
- Kelvin Jiang
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. |
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
tab_object.discarded = contents->WasDiscarded();Is it right that ENABLE_EXTENSIONS is true for desktop Android? I have a feeling that desktop Android can take the top path as well.
Actually, ENABLE_EXTENSIONS is false for desktop Android. ENABLE_EXTENSIONS_CORE is true. I wonder if this block would be clearer if we switched to BUILDFLAG(IS_ANDROID), since it sounds like there might be long-term differences in discard behavior between Android and other platforms.
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
tab_object.discarded = contents->WasDiscarded();James CookIs it right that ENABLE_EXTENSIONS is true for desktop Android? I have a feeling that desktop Android can take the top path as well.
Actually, ENABLE_EXTENSIONS is false for desktop Android. ENABLE_EXTENSIONS_CORE is true. I wonder if this block would be clearer if we switched to BUILDFLAG(IS_ANDROID), since it sounds like there might be long-term differences in discard behavior between Android and other platforms.
Thanks for the correction! I keep confusing the 2 flags with each other - my personal preference would be to use `IS_ANDROID` purely because I keep mixing the extensions flags up but that is entirely user error 😂
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Commit-Queue | +1 |
3 comments
TabListInterface* tab_list = TabListInterface::From(browser_window);Sorry I didn't notice before... I wonder if we need to check tab_list for null as well. WDYT?
We should add a CHECK here since at this point we do have a valid browser window corresponding to the tab's ID
James CookIs it right that ENABLE_EXTENSIONS is true for desktop Android? I have a feeling that desktop Android can take the top path as well.
Darryl JamesActually, ENABLE_EXTENSIONS is false for desktop Android. ENABLE_EXTENSIONS_CORE is true. I wonder if this block would be clearer if we switched to BUILDFLAG(IS_ANDROID), since it sounds like there might be long-term differences in discard behavior between Android and other platforms.
Thanks for the correction! I keep confusing the 2 flags with each other - my personal preference would be to use `IS_ANDROID` purely because I keep mixing the extensions flags up but that is entirely user error 😂
Done
return performance_manager::policies::PageDiscardingHelper::GetFromGraph(
graph)
->DiscardAPage(discard_reason, urgent_protection_time)
.first_content_after_discard;What are your thoughts on splitting this call up to avoid nullptr dereferences?
Ex:
```
if (!GetFromGraph()) {
return nullptr;
}if (!DiscardAPage()) {
return nullptr;
}return first_content_after_discard;
```It could be the case that these will always return a value, in that case disregard this comment 👍
Done for the page discarding helper: DiscardAPage always returns a struct value
Related details
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |