[Vertical Tab] Handle kTabWantsAttentionStatus attention type [chromium/src : main]
Charles Meng (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 1 comment![]( "Open in Gerrit")
void RootTabCollectionNode::OnTabNeedsAttentionChanged(int model_index,
bool attention) {
tabs::TabInterface* tab = tab_strip_model_->GetTabAtIndex(model_index);
TabCollectionNode* tab_node = GetNodeForHandle(tab->GetHandle());
if (tab_node && tab_node->view()) {
static_cast<VerticalTabView*>(tab_node->view())
->SetTabNeedsAttention(attention);
}
}Let's support both groups and tabs. So let's just add a function to TabCollectionNode::NotifyAttentionChanged and use that model. Groups and Tabs should subscribe similar to OnDataChanged (OnAttentionChanged and handle it that way). Please also add group support as well to this.
Related details
- Charles Meng
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. |
Charles Meng (Gerrit)
Charles Meng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Charles Meng
| Commit-Queue | +1 |
1 comment
void RootTabCollectionNode::OnTabNeedsAttentionChanged(int model_index,
bool attention) {
tabs::TabInterface* tab = tab_strip_model_->GetTabAtIndex(model_index);
TabCollectionNode* tab_node = GetNodeForHandle(tab->GetHandle());
if (tab_node && tab_node->view()) {
static_cast<VerticalTabView*>(tab_node->view())
->SetTabNeedsAttention(attention);
}
}Let's support both groups and tabs. So let's just add a function to TabCollectionNode::NotifyAttentionChanged and use that model. Groups and Tabs should subscribe similar to OnDataChanged (OnAttentionChanged and handle it that way). Please also add group support as well to this.
Do we anticipate that there will be more than one place that needs to know that attention has changed? I don't see why we should use a callback list here. In fact it seems like the existing on_will_destroy_callback_list_ and on_data_changed_callback_list_ both only ever have a single element, namely the node's view.
I will create a separate bug for handling the tab group attention indicator notifications because our current tab group header doesn't support attention indicators yet.
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 added 1 comment![]( "Open in Gerrit")
void RootTabCollectionNode::OnTabNeedsAttentionChanged(int model_index,
bool attention) {
tabs::TabInterface* tab = tab_strip_model_->GetTabAtIndex(model_index);
TabCollectionNode* tab_node = GetNodeForHandle(tab->GetHandle());
if (tab_node && tab_node->view()) {
static_cast<VerticalTabView*>(tab_node->view())
->SetTabNeedsAttention(attention);
}
}Charles MengLet's support both groups and tabs. So let's just add a function to TabCollectionNode::NotifyAttentionChanged and use that model. Groups and Tabs should subscribe similar to OnDataChanged (OnAttentionChanged and handle it that way). Please also add group support as well to this.
Do we anticipate that there will be more than one place that needs to know that attention has changed? I don't see why we should use a callback list here. In fact it seems like the existing on_will_destroy_callback_list_ and on_data_changed_callback_list_ both only ever have a single element, namely the node's view.
I will create a separate bug for handling the tab group attention indicator notifications because our current tab group header doesn't support attention indicators yet.
I think at the very least we should move that code handling dynamically casting to view to TabCollectionNode i.e. add a NotifyAttentionChanged and then handle different collection types and cast to views from there instead of root collection node.
I generally think the design principal was to keep the view model and view decoupled where view listens for view model updates and reacts. This allows the same view model to be used even for a different view implementation e.g. horizontal tab view. This is why I was hoping we can avoid doing it this way.
Related details
- Charles Meng
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 added 1 comment![]( "Open in Gerrit")
void RootTabCollectionNode::OnTabNeedsAttentionChanged(int model_index,
bool attention) {
tabs::TabInterface* tab = tab_strip_model_->GetTabAtIndex(model_index);
TabCollectionNode* tab_node = GetNodeForHandle(tab->GetHandle());
if (tab_node && tab_node->view()) {
static_cast<VerticalTabView*>(tab_node->view())
->SetTabNeedsAttention(attention);
}
}Charles MengLet's support both groups and tabs. So let's just add a function to TabCollectionNode::NotifyAttentionChanged and use that model. Groups and Tabs should subscribe similar to OnDataChanged (OnAttentionChanged and handle it that way). Please also add group support as well to this.
Eshwar StalinDo we anticipate that there will be more than one place that needs to know that attention has changed? I don't see why we should use a callback list here. In fact it seems like the existing on_will_destroy_callback_list_ and on_data_changed_callback_list_ both only ever have a single element, namely the node's view.
I will create a separate bug for handling the tab group attention indicator notifications because our current tab group header doesn't support attention indicators yet.
I think at the very least we should move that code handling dynamically casting to view to TabCollectionNode i.e. add a NotifyAttentionChanged and then handle different collection types and cast to views from there instead of root collection node.
I generally think the design principal was to keep the view model and view decoupled where view listens for view model updates and reacts. This allows the same view model to be used even for a different view implementation e.g. horizontal tab view. This is why I was hoping we can avoid doing it this way.
Thought about this a bit more. I think we might want to consider storing this state in TabUIHelper and then populating that data in TabRendererData and leveraging OnDataChanged path. The reason being let's say we switch between HT and VT tabs, we want to keep the state. Having state in the view would lead to bugs in these scenarios. WDYT?
Charles Meng (Gerrit)
Charles Meng added 1 comment![]( "Open in Gerrit")
void RootTabCollectionNode::OnTabNeedsAttentionChanged(int model_index,
bool attention) {
tabs::TabInterface* tab = tab_strip_model_->GetTabAtIndex(model_index);
TabCollectionNode* tab_node = GetNodeForHandle(tab->GetHandle());
if (tab_node && tab_node->view()) {
static_cast<VerticalTabView*>(tab_node->view())
->SetTabNeedsAttention(attention);
}
}Charles MengLet's support both groups and tabs. So let's just add a function to TabCollectionNode::NotifyAttentionChanged and use that model. Groups and Tabs should subscribe similar to OnDataChanged (OnAttentionChanged and handle it that way). Please also add group support as well to this.
Eshwar StalinDo we anticipate that there will be more than one place that needs to know that attention has changed? I don't see why we should use a callback list here. In fact it seems like the existing on_will_destroy_callback_list_ and on_data_changed_callback_list_ both only ever have a single element, namely the node's view.
I will create a separate bug for handling the tab group attention indicator notifications because our current tab group header doesn't support attention indicators yet.
Eshwar StalinI think at the very least we should move that code handling dynamically casting to view to TabCollectionNode i.e. add a NotifyAttentionChanged and then handle different collection types and cast to views from there instead of root collection node.
I generally think the design principal was to keep the view model and view decoupled where view listens for view model updates and reacts. This allows the same view model to be used even for a different view implementation e.g. horizontal tab view. This is why I was hoping we can avoid doing it this way.
Thought about this a bit more. I think we might want to consider storing this state in TabUIHelper and then populating that data in TabRendererData and leveraging OnDataChanged path. The reason being let's say we switch between HT and VT tabs, we want to keep the state. Having state in the view would lead to bugs in these scenarios. WDYT?
That makes sense. That bug currently doesn't exist because we secretly have the horizontal tabstrip while the vertical tab strip is showing, but once we have only one tabstrip at a time this will be a problem.
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. |
Charles Meng (Gerrit)
Charles Meng added 1 comment![]( "Open in Gerrit")
void RootTabCollectionNode::OnTabNeedsAttentionChanged(int model_index,
bool attention) {
tabs::TabInterface* tab = tab_strip_model_->GetTabAtIndex(model_index);
TabCollectionNode* tab_node = GetNodeForHandle(tab->GetHandle());
if (tab_node && tab_node->view()) {
static_cast<VerticalTabView*>(tab_node->view())
->SetTabNeedsAttention(attention);
}
}Charles MengLet's support both groups and tabs. So let's just add a function to TabCollectionNode::NotifyAttentionChanged and use that model. Groups and Tabs should subscribe similar to OnDataChanged (OnAttentionChanged and handle it that way). Please also add group support as well to this.
Eshwar StalinDo we anticipate that there will be more than one place that needs to know that attention has changed? I don't see why we should use a callback list here. In fact it seems like the existing on_will_destroy_callback_list_ and on_data_changed_callback_list_ both only ever have a single element, namely the node's view.
I will create a separate bug for handling the tab group attention indicator notifications because our current tab group header doesn't support attention indicators yet.
Eshwar StalinI think at the very least we should move that code handling dynamically casting to view to TabCollectionNode i.e. add a NotifyAttentionChanged and then handle different collection types and cast to views from there instead of root collection node.
I generally think the design principal was to keep the view model and view decoupled where view listens for view model updates and reacts. This allows the same view model to be used even for a different view implementation e.g. horizontal tab view. This is why I was hoping we can avoid doing it this way.
Charles MengThought about this a bit more. I think we might want to consider storing this state in TabUIHelper and then populating that data in TabRendererData and leveraging OnDataChanged path. The reason being let's say we switch between HT and VT tabs, we want to keep the state. Having state in the view would lead to bugs in these scenarios. WDYT?
That makes sense. That bug currently doesn't exist because we secretly have the horizontal tabstrip while the vertical tab strip is showing, but once we have only one tabstrip at a time this will be a problem.
Done
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 voted and added 4 comments![]( "Open in Gerrit")
Votes added by Eshwar Stalin
| Code-Review | +1 |
4 comments
bool needs_attention() { return needs_attention_; }make the function const?
// that it needs their attention. The UI indication is set iff `attention` isnit: typo
void SetTabNeedsAttention(bool attention);Can be removed now?
Related details
- Charles Meng
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. |
Charles Meng (Gerrit)
Charles Meng added 3 comments![]( "Open in Gerrit")
bool needs_attention() { return needs_attention_; }Charles Mengmake the function const?
Done
// that it needs their attention. The UI indication is set iff `attention` isCharles Mengnit: typo
Shorthand for "if and only if".
void SetTabNeedsAttention(bool attention);Charles MengCan be removed now?
Done
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 1 comment![]( "Open in Gerrit")
if (IsActive()) {
icon_->SetAttention(TabIcon::AttentionType::kBlockedWebContents, false);
} else {
icon_->SetAttention(TabIcon::AttentionType::kBlockedWebContents,
data_.blocked);
}nit: could we combine these two like we did with vertical tab view in a future CL?
Related details
- Charles Meng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charles Meng (Gerrit)
Charles Meng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Charles Meng
| Auto-Submit | +1 |
icon_->SetAttention(TabIcon::AttentionType::kBlockedWebContents, false);
} else {
icon_->SetAttention(TabIcon::AttentionType::kBlockedWebContents,
data_.blocked);
}nit: could we combine these two like we did with vertical tab view in a future CL?
- 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 voted![]( "Open in Gerrit")
- Charles Meng
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")
Change information
[Vertical Tab] Handle kTabWantsAttentionStatus attention type
Previously we were only handling the web contents blocked attention
type. This CL additionally handles the tab wants attention case, so
that the attention indicator will show on background tabs if an alert
is triggered after a timeout, for example.
Screencast:
https://screencast.googleplex.com/cast/NTk5NDg4Nzk0NTEyNTg4OHxkMWQ0MWI3MS02NA
- M chrome/browser/ui/tab_ui_helper.h
- M chrome/browser/ui/tabs/tab_change_type.h
- M chrome/browser/ui/tabs/tab_renderer_data.cc
- M chrome/browser/ui/tabs/tab_renderer_data.h
- M chrome/browser/ui/tabs/tab_strip_model.cc
- M chrome/browser/ui/tabs/tab_strip_model_observer.cc
- M chrome/browser/ui/tabs/tab_strip_model_observer.h
- M chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.cc
- M chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.h
- M chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
- M chrome/browser/ui/views/tabs/browser_tab_strip_controller.h
- M chrome/browser/ui/views/tabs/tab.cc
- M chrome/browser/ui/views/tabs/tab.h
- M chrome/browser/ui/views/tabs/tab_strip.cc
- M chrome/browser/ui/views/tabs/tab_strip.h
- M chrome/browser/ui/views/tabs/tab_strip_unittest.cc
- M chrome/browser/ui/views/tabs/vertical/vertical_tab_view.cc
- M chrome/browser/ui/views/tabs/vertical/vertical_tab_view_browsertest.cc
Code-Review: +1 by Eshwar Stalin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |