[TabRendererData] Move discard code into the TabUiHelper [chromium/src : main]
Steven Luong (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 7 comments![]( "Open in Gerrit")
base::ByteSize DiscardedMemorySavings();Should this be optional?
using TitleUpdatedCallbackList =
base::RepeatingCallbackList<void(std::u16string)>;
base::CallbackListSubscription AddTitleUpdatedCallback(
TitleUpdatedCallbackList::CallbackType callback);Seems like this isn't used other than test, can we remove?
base::CallbackListSubscription AddTabUiChangeCallback(Please capitalize UI
title_change_callbacks_.Notify(GetTitle());Can we use the new callback? Also look at other places where this needs to be notified.
// Notify observers that the tab should update its UI to show discard status.
if (ShouldShowDiscardUi()) {Shouldn't this be fired unconditionally? What if we want to update from showing -> not showing state?
data.should_show_discard_status = tab_ui_helper->ShouldShowDiscardUi();nit: ShouldShowDiscardStatus instead of UI.
data.discarded_memory_savings = tab_ui_helper->DiscardedMemorySavings();nit: GetDiscardedMemorySavings?
Related details
- Steven Luong
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. |
Steven Luong (Gerrit)
Steven Luong added 7 comments![]( "Open in Gerrit")
base::ByteSize DiscardedMemorySavings();Steven LuongShould this be optional?
Right now the TabRendererData code treats non-discarded memory savings as 0 since the tab is not discarded but semantically making it optional probably makes more sense.
using TitleUpdatedCallbackList =
base::RepeatingCallbackList<void(std::u16string)>;
base::CallbackListSubscription AddTitleUpdatedCallback(
TitleUpdatedCallbackList::CallbackType callback);Seems like this isn't used other than test, can we remove?
Ack. Yeah I will remove this in a follow up CL since the callback should be moved into the generic tab changed callback.
base::CallbackListSubscription AddTabUiChangeCallback(Steven LuongPlease capitalize UI
Acknowledged
Can we use the new callback? Also look at other places where this needs to be notified.
Ack. Will move this to the new callback in a follow up CL since we can probably just delete the title change.
// Notify observers that the tab should update its UI to show discard status.
if (ShouldShowDiscardUi()) {Shouldn't this be fired unconditionally? What if we want to update from showing -> not showing state?
TabUIHelper::WasDiscarded() is only called after the tab is discarded. The only time we go from showing->not showing the UI is when the discarded tab gets reloaded and that doesn't trigger a WasDiscarded() call.
data.should_show_discard_status = tab_ui_helper->ShouldShowDiscardUi();nit: ShouldShowDiscardStatus instead of UI.
Done
data.discarded_memory_savings = tab_ui_helper->DiscardedMemorySavings();Steven Luongnit: GetDiscardedMemorySavings?
Done
Related details
- Eshwar Stalin
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. |
Eshwar Stalin (Gerrit)
Eshwar Stalin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Eshwar Stalin
| Code-Review | +1 |
2 comments
// Notify observers that the tab should update its UI to show discard status.
if (ShouldShowDiscardUi()) {Steven LuongShouldn't this be fired unconditionally? What if we want to update from showing -> not showing state?
TabUIHelper::WasDiscarded() is only called after the tab is discarded. The only time we go from showing->not showing the UI is when the discarded tab gets reloaded and that doesn't trigger a WasDiscarded() call.
Ok in that case, we need to notify in that scenario as well. Would that be covered when we notify during page reload?
I think this is where I think we need to dig into how that all needs to be coordinated since some of that logic is currently happening in browser.cc. From what I see it doing is its coalesing updates and batching them. So it does provide a functionality that cannot be simply handled in TabUIHelper. I believe what we can do is have browser.cc call into TabUIHelper to trigger the update instead of going to TabStripModel.
This feedback isn't blocking this CL but effectively tab_ui_change_callbacks_ in its current state doesn't really work. So two options 1) Leave for now. It really doesn't work but we will fix it correctly in the future. Until then don't have any clients use tab_ui_change_callbacks_. 2) Land the change for observation as a separate CL once we consolidate more things into TabUIHelper and we fully integrate with browser.cc.
Eitherway is fine with me.
content::WebContents* const web_contents = tab().GetContents();This uses the new contents that was created after discarding correct?
Related details
- Steven Luong
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. |
Steven Luong (Gerrit)
Steven Luong added 1 comment![]( "Open in Gerrit")
// Notify observers that the tab should update its UI to show discard status.
if (ShouldShowDiscardUi()) {Steven LuongShouldn't this be fired unconditionally? What if we want to update from showing -> not showing state?
Eshwar StalinTabUIHelper::WasDiscarded() is only called after the tab is discarded. The only time we go from showing->not showing the UI is when the discarded tab gets reloaded and that doesn't trigger a WasDiscarded() call.
Ok in that case, we need to notify in that scenario as well. Would that be covered when we notify during page reload?
I think this is where I think we need to dig into how that all needs to be coordinated since some of that logic is currently happening in browser.cc. From what I see it doing is its coalesing updates and batching them. So it does provide a functionality that cannot be simply handled in TabUIHelper. I believe what we can do is have browser.cc call into TabUIHelper to trigger the update instead of going to TabStripModel.
This feedback isn't blocking this CL but effectively tab_ui_change_callbacks_ in its current state doesn't really work. So two options 1) Leave for now. It really doesn't work but we will fix it correctly in the future. Until then don't have any clients use tab_ui_change_callbacks_. 2) Land the change for observation as a separate CL once we consolidate more things into TabUIHelper and we fully integrate with browser.cc.
Eitherway is fine with me.
Ack. Yup when the TabUIHelper accounts for when tabs are reloading, then it will automatically handle the state from showing -> not showing discard UI. In that case, I'll move forward with option 2.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steven Luong (Gerrit)
Steven Luong voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[TabRendererData] Move discard code into the TabUiHelper
Moves the discard logic from the TabRendererData and into the
TabUiHelper.
- M chrome/browser/ui/tab_ui_helper.cc
- M chrome/browser/ui/tab_ui_helper.h
- M chrome/browser/ui/tab_ui_helper_browsertest.cc
- M chrome/browser/ui/tabs/tab_renderer_data.cc
Code-Review: +1 by Eshwar Stalin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |