Currently, observer callbacks are tightly tied to internal state of TabStripModel, which makes it very difficult to change the implementation of TabStripModel, and makes re-entrancy extra unsafe.
I spent a while trying to figure out how to simplify TabStrip::SetSelection [as I recently added state to the class - selected_tabs_] to get it to work correctly. I came up with many alternatives, but none were simpler than the current code.
The reason that the previous code appeared simpler is that it did not work correctly.
e.g. If you check out 9fedcd6889424af9766ebf07eab67d42ec0a9091 [last commit before my changes], then:
Calling TabStripModel::DetachWebContentsAt(...) will not cause the following notifications to fire:
* tab_at(old_selection.active())->ActiveStateChanged();
* tab_at(no_longer_selected[i])->NotifyAccessibilityEvent(ax::mojom::Event::kSelectionRemove, true);
After discussion with sky, I believe that the appropriate notification to expose from TabStripModelObserver is:
OnStateChanged(vector<Deltas>, old_selection [pre-application of deltas], new_selection [post-application of deltas])
This replaces:
TabInsertedAt, TabClosingAt, TabDetachedAt, TabDeactivated, ActiveTabChanged, TabSelectionChanged, TabMoved.
This allows:
1) Atomic update for observers for both the selection state and tab list. This simplifies internal logic in TabStrip and StackedTabStripLayout.
2) Tab restore could be changed to treat multi-tab closes as a single unit.
3) Animations could be improved for multi-tabs closes.
4) Easier to enforce non-reentrancy.
5) Further decouples implementation of TabStripModel from observer notifications, allowing for slight simplification of implementation. This also allows easier decoupling of TabStripModel implementation from WebContents.
This sounds like a great cleanup.
Presumably the "delta" and "selection" objects would be easily queryable for equivalent tab state that was implicitly communicated via the replaced observer methods? Otherwise there will be a lot of similar code written at all of the observer implementations.
I suppose it would be possible to add the new observer method, deprecate the old ones, port code bit by bit, and then remove them one by one? Doing this would allow us to fix some of the dependent bugs immediately while allowing the overall refactor to proceed at its own pace, and ensuring that the situation doesn't continue to get worse (more users of the old methods).
I was planning on putting this backburner since this was no pressing reason to refactor. Since this is now blocking 805775, perhaps we should up the priority?
I'll throw something together today.
Comment 1 Deleted