Multi-tab: Adding new Notification when tab selection changes.

Multi-tab: Adding new Notification when tab selection changes.

In this CL
1) TabStripModelObserver::ActiveTabChanged is only called when the active tab actually changes
   (where as before it was called to also signal tab selection changes).
2) TabStripModelObserver::TabSelectionChanged is called when the tab selection changes.
3) BaseTabStrip::SelectTabAt() is replaced by BaseTabStrip::SetSelection(). 

BUG=NONE
TEST=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89752

[dpapad, 2011-06-02 18:40:33]

I have not updated any classes that implement the TabStripModelObsever yet.
Doing an ack-grep returned 32 files. Is it safe to assume that wherever the
check 
"if (old_contents == new_contents) return;" does not occur I should replace with
TabSelectionChanged() instead?

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:575: if (old_selection_model.active() !=
active_index())
Instead of repeatedly checking this I would prefer to put this check in
NotifyActiveTabChanged (and maybe rename it to NotifyIfActiveTabChanged or just
update the comments).

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:1253: const TabStripSelectionModel&
model) {
Also how about adding a check here to determine if selected_indices_ changed
before sending notifications?

[sky, 2011-06-02 20:27:58]

> I have not updated any classes that implement the
> TabStripModelObsever yet.
> Doing an ack-grep returned 32 files. Is it safe to assume
> that wherever the check 
> "if (old_contents == new_contents) return;" does not occur I
> should replace with TabSelectionChanged() instead?

I suspect so, but I'll have to look at each one to make sure.

Also, don't forget to add some test coverage.

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:575: if (old_selection_model.active() !=
active_index())
On 2011/06/02 18:40:33, dpapad wrote:
> Instead of repeatedly checking this I would prefer to put this check in
> NotifyActiveTabChanged (and maybe rename it to NotifyIfActiveTabChanged or
just
> update the comments).

Yes. In fact how about a single SelectionChanged method that replaces 575-579.
It would compare active index, and if necessary call NotifyActiveTabChanged as
well as NotifySelectionChanged.
How about a single method you call when ever the selection might have changed
(replace 575-

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:1253: const TabStripSelectionModel&
model) {
On 2011/06/02 18:40:33, dpapad wrote:
> Also how about adding a check here to determine if selected_indices_ changed
> before sending notifications? 

Sure, but make sure you include the anchor/active index in that comparison.

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model_observer.h (right):

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model_observer.h:65: // Sent when the selection
changes. The previously selected tab is identified
Update this comment.

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model_observer.h:83: // Sent when the selection
changes.
Document what model contains.

[dpapad, 2011-06-02 23:29:50]

I tried to update all sites that override ActiveTabChanged. I removed the
"old_contents == new_contents" check. All remaining occurrences that did not
have this check were accessing the parameters of ActiveTabChanged (with one
exception chrome/browser/ui/toolbar/wrench_menu_model.cc), so for the moment
there is no site overriding TabSelectionChanged.

Regarding test coverage I will take a look in
chrome/browser/tabs/tab_strip_model_unittest.cc and see what tests could be
added.

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:575: if (old_selection_model.active() !=
active_index())
On 2011/06/02 20:27:58, sky wrote:
> On 2011/06/02 18:40:33, dpapad wrote:
> > Instead of repeatedly checking this I would prefer to put this check in
> > NotifyActiveTabChanged (and maybe rename it to NotifyIfActiveTabChanged or
> just
> > update the comments).
> 
> Yes. In fact how about a single SelectionChanged method that replaces 575-579.
> It would compare active index, and if necessary call NotifyActiveTabChanged as
> well as NotifySelectionChanged.
> How about a single method you call when ever the selection might have changed
> (replace 575-

Done.

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:1253: const TabStripSelectionModel&
model) {
On 2011/06/02 20:27:58, sky wrote:
> On 2011/06/02 18:40:33, dpapad wrote:
> > Also how about adding a check here to determine if selected_indices_ changed
> > before sending notifications? 
> 
> Sure, but make sure you include the anchor/active index in that comparison.

Done.

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model_observer.h (right):

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model_observer.h:65: // Sent when the selection
changes. The previously selected tab is identified
On 2011/06/02 20:27:58, sky wrote:
> Update this comment.

Done.

http://codereview.chromium.org/7033048/diff/1/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model_observer.h:83: // Sent when the selection
changes.
On 2011/06/02 20:27:58, sky wrote:
> Document what model contains.

Done.

[sky, 2011-06-03 15:35:56]

The one place that wants to know about any selection change is
BrowserTabStripController. Change the override of ActiveTabChanged to
TabSelectionChanged. Rename BaseTabStrip::SelectTabAt to SetSelection and pass
it the old and new selection models.
Here's what you'll want for the implementations:


void SideTabStrip::SetSelection(const SelectionModel& old_selection,
                                const SelectionModel& new_selection) {
  GetBaseTabAtModelIndex(new_selection.active())->SchedulePaint();
  if (old_selection.active() != new_selection.active())
    MakeTabVisible(ModelIndexToTabIndex(new_selection.active());
}


void TabStrip::SetSelection(const SelectionModel& old_selection,
                            const SelectionModel& new_selection) {
  // We have "tiny tabs" if the tabs are so tiny that the unselected ones are
  // a different size to the selected ones.
  bool tiny_tabs = current_unselected_width_ != current_selected_width_;
  if (!IsAnimating() && (!in_tab_close_ || tiny_tabs)) {
    DoLayout();
  } else {
    SchedulePaint();
  }

  TabStripSelectionModel::SelectedIndices no_longer_selected;
  std::set_difference(old_selection.selected_indices().begin(),
                      old_selection.selected_indices().end(),
                      new_selection.selected_indices().begin(),
                      new_selection.selected_indices().end(),
                      no_long_selected.begin());
  for (size_t i = 0; i < no_longer_selected.size(); ++i) {
    GetTabAtTabDataIndex(ModelIndexToTabIndex(no_longer_selected[i]))->
        StopMiniTabTitleAnimation();
  }
}

You'll need to update TouchTabStrip as well. That should just be a simple
rename.

I suspect TabStripGtk should do the same thing as TabStrip here.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:163:
selection_model_.SetSelectedIndex(index);
As mentioned in other email, make this send out both notifications.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:226: if (!selection_model_.empty()) {
Make these send out new notification too.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.h:529: // snapshot of the selection_model_
before the change.
'the selection_model_' -> '|selection_model_|'

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.h:533: // changed, by calling
NotifyActiveTabChanged and/or NotifySelectionChanged.
Document what model is.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model_observer.h (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:65: // Sent when the selection
changes. The previously selected tab is identified
'Sent when the selection changes' -> 'Sent when the active tab changes'.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:69: // side-effect of some other
function.
Also add something about 'TabSelectionChanged'. Maybe:
Note: it is possible for the selection to change but not the active tab. For
example, control-click may not change the active tab but does change the
selection. In this case 'ActiveTabChanged' is not sent. If you care about any
changes to the selection, use TabSelectionChanged.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:76: // tab or active tab change.
Add something about see also ActiveTabChanged for details.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:77: virtual void
TabSelectionChanged(const TabStripSelectionModel& model);
model -> old_model. And also document what it is.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_selection_model.cc (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_selection_model.cc:146: selected_indices().size()
== rhs.selected_indices().size() &&
can't use use == for comparing the two vectors?

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_selection_model.h (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_selection_model.h:99: bool Equals(const
TabStripSelectionModel& rhs) const;
Add a test for coverage of this.

[dpapad, 2011-06-03 17:49:01]

Changes to SideTabStrip, TabStrip, TouchTabStrip and test coverage for
TabStripSelectionModel::Equals will follow (in this CL). I just want to clarify
a few points first.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:163:
selection_model_.SetSelectedIndex(index);
On 2011/06/03 15:35:56, sky wrote:
> As mentioned in other email, make this send out both notifications.

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:226: if (!selection_model_.empty()) {
On 2011/06/03 15:35:56, sky wrote:
> Make these send out new notification too.

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.h:529: // snapshot of the selection_model_
before the change.
On 2011/06/03 15:35:56, sky wrote:
> 'the selection_model_' -> '|selection_model_|'

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.h:533: // changed, by calling
NotifyActiveTabChanged and/or NotifySelectionChanged.
On 2011/06/03 15:35:56, sky wrote:
> Document what model is.

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model_observer.h (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:65: // Sent when the selection
changes. The previously selected tab is identified
On 2011/06/03 15:35:56, sky wrote:
> 'Sent when the selection changes' -> 'Sent when the active tab changes'.

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:69: // side-effect of some other
function.
On 2011/06/03 15:35:56, sky wrote:
> Also add something about 'TabSelectionChanged'. Maybe:
> Note: it is possible for the selection to change but not the active tab. For
> example, control-click may not change the active tab but does change the
> selection. In this case 'ActiveTabChanged' is not sent. If you care about any
> changes to the selection, use TabSelectionChanged.

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:76: // tab or active tab change.
On 2011/06/03 15:35:56, sky wrote:
> Add something about see also ActiveTabChanged for details.

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_observer.h:77: virtual void
TabSelectionChanged(const TabStripSelectionModel& model);
On 2011/06/03 15:35:56, sky wrote:
> model -> old_model. And also document what it is.

Done.

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_selection_model.cc (right):

http://codereview.chromium.org/7033048/diff/1005/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_selection_model.cc:146: selected_indices().size()
== rhs.selected_indices().size() &&
On 2011/06/03 15:35:56, sky wrote:
> can't use use == for comparing the two vectors?

Done.

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:160: TabInsertedAt(contents, index,
active));
Should the |active| flag from TabInsertedAt be removed now? If the newly
inserted tab becomes active a new notification will follow. It looks like
TabInsertedAt and ActiveTabChanged step on each other's responsibilities.

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:232:
NotifyIfActiveOrSelectionChanged(old_model);
Isn't the |user_gesture| flag info being lost with this change? Same holds for
below.

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:250:
selection_model_.SetSelectedIndex(index);
This call might change the selection also, shouldn't there be a
NotifySelectionChanged notification also?

[sky, 2011-06-03 18:17:40]

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:160: TabInsertedAt(contents, index,
active));
On 2011/06/03 17:49:02, dpapad wrote:
> Should the |active| flag from TabInsertedAt be removed now? If the newly
> inserted tab becomes active a new notification will follow. It looks like
> TabInsertedAt and ActiveTabChanged step on each other's responsibilities.

I'm not completely sure. You'll have to look at what all consumers do. If you
remove it, it would mean consumers don't have an easy way to know selection is
changing as the result of an insert. That said, I don't know if anyone actually
cares about that cast.

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:232:
NotifyIfActiveOrSelectionChanged(old_model);
On 2011/06/03 17:49:02, dpapad wrote:
> Isn't the |user_gesture| flag info being lost with this change? Same holds for
> below.

What do you mean? For whatever reason Detach doesn't take a user_gesture field,
so we have to assume it's false.

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:250:
selection_model_.SetSelectedIndex(index);
On 2011/06/03 17:49:02, dpapad wrote:
> This call might change the selection also, shouldn't there be a
> NotifySelectionChanged notification also?

Yes.

[dpapad, 2011-06-03 18:35:10]

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:232:
NotifyIfActiveOrSelectionChanged(old_model);
On 2011/06/03 18:17:40, sky wrote:
> On 2011/06/03 17:49:02, dpapad wrote:
> > Isn't the |user_gesture| flag info being lost with this change? Same holds
for
> > below.
> 
> What do you mean? For whatever reason Detach doesn't take a user_gesture
field,
> so we have to assume it's false.

As of now NotifyIfActiveOrSelectionChanged() calls NotifyActiveTabChanged()
which always calls
FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                  ActiveTabChanged(old_tab, new_tab, active_index(), true));
with the user_gesture hardcoded to "true". I think it should be a parameter of
both NotifyActiveTabChanged() and NotifyIfActiveOrSelectionChanged() if we plan
to use it in cases where |user_gesture| can be both true/false.

[sky, 2011-06-03 19:00:09]

On Fri, Jun 3, 2011 at 11:35 AM,  <dpapad@chromium.org> wrote:
>
>
http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
> File chrome/browser/tabs/tab_strip_model.cc (right):
>
>
http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
> chrome/browser/tabs/tab_strip_model.cc:232:
> NotifyIfActiveOrSelectionChanged(old_model);
> On 2011/06/03 18:17:40, sky wrote:
>>
>> On 2011/06/03 17:49:02, dpapad wrote:
>> > Isn't the |user_gesture| flag info being lost with this change? Same
>
> holds for
>>
>> > below.
>
>> What do you mean? For whatever reason Detach doesn't take a
>
> user_gesture field,
>>
>> so we have to assume it's false.
>
> As of now NotifyIfActiveOrSelectionChanged() calls
> NotifyActiveTabChanged() which always calls
> FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
>                  ActiveTabChanged(old_tab, new_tab, active_index(),
> true));
> with the user_gesture hardcoded to "true". I think it should be a
> parameter of both NotifyActiveTabChanged() and
> NotifyIfActiveOrSelectionChanged() if we plan to use it in cases where
> |user_gesture| can be both true/false.
>
> http://codereview.chromium.org/7033048/
>

SGTM

[dpapad, 2011-06-06 16:28:20]

I think that I have included all requested changes. As you see there are 4 unit
tests failing, MultipleToSingle, TestBasicAPI, TabAwayRevertSelect and
PersistKeywordModeOnTabSwitch. I believe that they just need to be updated. For
example in chrome/browser/tabs/tab_strip_model_unittest.cc I have to override
TabSelectionChanged in MockTabStripModelObserver. Any help/suggestion for these
unit tests would be appreciated. After fixing/updating the existing unit tests,
I will add some test coverage for TabStripSelectionModel::Equals().

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/9001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model.cc:250:
selection_model_.SetSelectedIndex(index);
On 2011/06/03 18:17:40, sky wrote:
> On 2011/06/03 17:49:02, dpapad wrote:
> > This call might change the selection also, shouldn't there be a
> > NotifySelectionChanged notification also?
> 
> Yes.

Done.

[sky, 2011-06-06 21:01:14]

I only looked at TabStripModel. Once you work out the kinks here I'll look at
the failing unit tests.

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:165:
NotifyIfActiveOrSelectionChanged(old_model, false);
Can 165 be moved into the if block?

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:224: old_model.Copy(selection_model_);
Don't you want to copy the model after decrementing? In the insert code you copy
after incrementing the model.

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:237:
NotifyIfActiveOrSelectionChanged(old_model, false);
Won't this end up passing the wrong contents to the delegate since the remove
contents is no longer in the model?

[dpapad, 2011-06-06 22:17:11]

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:165:
NotifyIfActiveOrSelectionChanged(old_model, false);
On 2011/06/06 21:01:14, sky wrote:
> Can 165 be moved into the if block?

Done. Putting it in the if block seems reasonable.

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:224: old_model.Copy(selection_model_);
On 2011/06/06 21:01:14, sky wrote:
> Don't you want to copy the model after decrementing? In the insert code you
copy
> after incrementing the model.

Copying the model after decrementing, results in no tab being active when the
active tab is closed.

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:237:
NotifyIfActiveOrSelectionChanged(old_model, false);
On 2011/06/06 21:01:14, sky wrote:
> Won't this end up passing the wrong contents to the delegate since the remove
> contents is no longer in the model?

It will pass indices that might no longer exist to the observers. Is that
necessarily wrong? For example what should it pass if the last tab is closed
while being active?

[sky, 2011-06-06 22:42:16]

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:224: old_model.Copy(selection_model_);
On 2011/06/06 22:17:11, dpapad wrote:
> On 2011/06/06 21:01:14, sky wrote:
> > Don't you want to copy the model after decrementing? In the insert code you
> copy
> > after incrementing the model.
> 
> Copying the model after decrementing, results in no tab being active when the
> active tab is closed.

Did you update 226 as well?

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:237:
NotifyIfActiveOrSelectionChanged(old_model, false);
On 2011/06/06 22:17:11, dpapad wrote:
> On 2011/06/06 21:01:14, sky wrote:
> > Won't this end up passing the wrong contents to the delegate since the
remove
> > contents is no longer in the model?
> 
> It will pass indices that might no longer exist to the observers. Is that
> necessarily wrong?

The problem is NotifyIfActiveOrSelectionChanges digs out the previously selected
tabcontents using the old selection model with the updated vector. It's roughly
something like:

contents_data_.erase(index_to_remove);
TabContents* old_tab = GetTabContentsAt(index_to_remove)
ActiveTabChangedAt(old_tab);

GetTabContentsAt is invoked after the remove and is going to return the wrong
thing.

> For example what should it pass if the last tab is closed
> while being active?

I think you need to special case delete code and not use the general notify
methods.

[dpapad, 2011-06-07 20:19:56]

After the latest changes only TabStripModelTest.MultipleToSingle unit test is
failing, it just needs to be updated (will do it in follow up patches after we
settle down on the changes).

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:224: old_model.Copy(selection_model_);
On 2011/06/06 22:42:16, sky wrote:
> On 2011/06/06 22:17:11, dpapad wrote:
> > On 2011/06/06 21:01:14, sky wrote:
> > > Don't you want to copy the model after decrementing? In the insert code
you
> > copy
> > > after incrementing the model.
> > 
> > Copying the model after decrementing, results in no tab being active when
the
> > active tab is closed.
> 
> Did you update 226 as well?

I moved lines 223-224 right after 225 and the behavior was as described.

http://codereview.chromium.org/7033048/diff/12001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:237:
NotifyIfActiveOrSelectionChanged(old_model, false);
On 2011/06/06 22:42:16, sky wrote:
> On 2011/06/06 22:17:11, dpapad wrote:
> > On 2011/06/06 21:01:14, sky wrote:
> > > Won't this end up passing the wrong contents to the delegate since the
> remove
> > > contents is no longer in the model?
> > 
> > It will pass indices that might no longer exist to the observers. Is that
> > necessarily wrong?
> 
> The problem is NotifyIfActiveOrSelectionChanges digs out the previously
selected
> tabcontents using the old selection model with the updated vector. It's
roughly
> something like:
> 
> contents_data_.erase(index_to_remove);
> TabContents* old_tab = GetTabContentsAt(index_to_remove)
> ActiveTabChangedAt(old_tab);
> 
> GetTabContentsAt is invoked after the remove and is going to return the wrong
> thing.
> 
> > For example what should it pass if the last tab is closed
> > while being active?
> 
> I think you need to special case delete code and not use the general notify
> methods.

Done. I changed the notify functions I had added and reused the existing
function NotifyTabSelectedIfChanged, now named as NotifyIfActiveTabChanged, see
latest patch. Storing the old active TabCotnentsWrapper* solves the problem you
just described and it seems to work both for code that is adding and removing
tabs.

[sky, 2011-06-07 20:32:44]

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:165:
NotifyIfActiveTabChanged(selected_contents, active_index(), false);
You always end up invoking both of these methods at the same time. How about
combining them into a single method?

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:226: old_model.Copy(selection_model_);
I'm still confused by this. You've notified the observers the tab has been
removed, which means they should have shifted things down around already so that
using the selection model before its been adjusted doesn't makes sense.
Shouldn't you copy the old_model *after* 227? Compare this with
InsertTabContentsAt which does what I'm saying. And yes, you'll need to update
the if to make it work correctly.

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:1214: int to_index,
You always pass in to_index = to active_index(). Could you remove the parameter
and use active_index() here? Especially since 1228 uses active_index already.

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.h:519: // notifies observers. If
|old_contents| still exist a TabDeselected
'still exist' -> 'non-null'
TabDeselected -> TabDeactivated

[dpapad, 2011-06-07 23:20:08]

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:165:
NotifyIfActiveTabChanged(selected_contents, active_index(), false);
On 2011/06/07 20:32:44, sky wrote:
> You always end up invoking both of these methods at the same time. How about
> combining them into a single method?

Done. Combined to a single method named NotifyIfActiveOrSelectionChanged.

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:226: old_model.Copy(selection_model_);
On 2011/06/07 20:32:44, sky wrote:
> I'm still confused by this. You've notified the observers the tab has been
> removed, which means they should have shifted things down around already so
that
> using the selection model before its been adjusted doesn't makes sense.
> Shouldn't you copy the old_model *after* 227? Compare this with
> InsertTabContentsAt which does what I'm saying. And yes, you'll need to update
> the if to make it work correctly.

Done.

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:1214: int to_index,
On 2011/06/07 20:32:44, sky wrote:
> You always pass in to_index = to active_index(). Could you remove the
parameter
> and use active_index() here? Especially since 1228 uses active_index already.

Done.

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/7033048/diff/21001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.h:519: // notifies observers. If
|old_contents| still exist a TabDeselected
On 2011/06/07 20:32:44, sky wrote:
> 'still exist' -> 'non-null'
> TabDeselected -> TabDeactivated

Done.

[sky, 2011-06-07 23:29:29]

Yay, looks like you're on the right tab. Just add tests and all good. Make sure
you add coverage for some of the cases you missed earlier. For example, that the
correct old contents is passed to ActiveTabChanged when doing a remove.

  -Scott

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/side_tab_strip.h (right):

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/side_tab_strip.h:12: class TabStripSelectionModel;
Should be after TabRendererData.

[dpapad, 2011-06-08 18:22:51]

I am in the process of fixing/updating existing unit tests.

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:227: if (selection_model_.active() ==
TabStripSelectionModel::kUnselectedIndex) {
It turns out that this condition is not correct, caused the
TabStripModelTest.InsertBefore to hang (fixed now).

In the process of fixing it, I noticed a very puzzling behavior:

Could you take a very quick look at TabStripSelectionModel::SetSelectedIndex and
SetSelectionFromAnchorTo. These two functions call each other, specifically if
you invoke SetSelectedIndex(-1) the program enters an infinite loop!! I suppose
that this is a clear logic error.

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/side_tab_strip.h (right):

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/side_tab_strip.h:12: class TabStripSelectionModel;
On 2011/06/07 23:29:29, sky wrote:
> Should be after TabRendererData.

Done.

[sky, 2011-06-08 19:25:07]

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/25025/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:227: if (selection_model_.active() ==
TabStripSelectionModel::kUnselectedIndex) {
On 2011/06/08 18:22:51, dpapad wrote:
> It turns out that this condition is not correct, caused the
> TabStripModelTest.InsertBefore to hang (fixed now).
> 
> In the process of fixing it, I noticed a very puzzling behavior:
> 
> Could you take a very quick look at TabStripSelectionModel::SetSelectedIndex
and
> SetSelectionFromAnchorTo. These two functions call each other, specifically if
> you invoke SetSelectedIndex(-1) the program enters an infinite loop!! I
suppose
> that this is a clear logic error.

Ick! I wanted SetSelectionFromAnchorTo to behave like SetSelectedIndex if there
is no anchor. That's why it calls out like that. And then I wanted
SetSelectedIndex to share the code in SetSelectionFromAnchorTo for the actual
work. But that obviously is problematic as you found.

Seems best to make SetSelectedIndex not use SetSelectionFromAnchorTo at all.

[dpapad, 2011-06-09 00:38:29]

I added test coverage for TabStripSelectionModel::Equals. 

Regarding adding tests for TabSelectionChanged: 
  By looking at tab_strip_model_unittest.cc it seems that when a callback is
invoked creates a State variable, pushes it to |states_|, and then some checks
are performed on this vector. How can I initialize a State variable from within
TabSelectionChanged (the problem is that I can only retrieve the old_contents
from the old_model but not the new_contents)?

Also by making TabSelectionChanged push variables to |states_| I guess I will
have to update almost all tests since this will change the expected position
within the vector of the various elements that are already being pushed.

[sky, 2011-06-09 17:02:56]

On Wed, Jun 8, 2011 at 5:38 PM,  <dpapad@chromium.org> wrote:
> I added test coverage for TabStripSelectionModel::Equals.
>
> Regarding adding tests for TabSelectionChanged:
>  By looking at tab_strip_model_unittest.cc it seems that when a callback is
> invoked creates a State variable, pushes it to |states_|, and then some
> checks
> are performed on this vector. How can I initialize a State variable from
> within
> TabSelectionChanged (the problem is that I can only retrieve the
> old_contents
> from the old_model but not the new_contents)?

You could always make MockTabStripModelObserver take a handle to the
TabStrip. That way, TabSelectionChanged can get the new contents.

> Also by making TabSelectionChanged push variables to |states_| I guess I
> will
> have to update almost all tests since this will change the expected position
> within the vector of the various elements that are already being pushed.

Alternatively you make MockTabStripModelObserver take a boolean
indicating if it should track TabSelectionChanged.

I honestly don't like MockTabStripModelObserver::State. It's too
tedious to create State objects in code like that, and if they fail
you don't really know why. I really want to convert it to the
TabStripSelectionModel style which does string based comparisons. I
find this approach much more readable with better error descriptions.
I'm not saying do that now, but it's something to consider.

  -Scott

[dpapad, 2011-06-10 17:23:55]

I fixed all the existing unit tests and also added a new test for multiple
selection. I added a bool as you suggested for not tracking TabSelectionChanged
in existing tests (if tracked TestBasicAPI fails because the assert statements
need to be updated).

I agree that the string based comparisons in 
tab_strip_selection_model_unittest.cc are much easier to write and debug and I
will keep this in mind.

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_unittest.cc:2251:
ASSERT_EQ(observer.GetStateAt(4)->action,
It seems that closing a selected tab does not result in a TabSelectionChanged
notification being sent, that needs to be fixed, right?

[sky, 2011-06-10 17:56:04]

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_unittest.cc:2251:
ASSERT_EQ(observer.GetStateAt(4)->action,
On 2011/06/10 17:23:55, dpapad wrote:
> It seems that closing a selected tab does not result in a TabSelectionChanged
> notification being sent, that needs to be fixed, right?

Yes, I think its a good idea to force the notification to be sent even though
old == new.

http://codereview.chromium.org/7033048/diff/19059/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/19059/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:1213: TabContentsWrapper*
last_selected_contents = old_contents;
nit: just use old_contents instead of creating another varaible.

[dpapad, 2011-06-10 18:21:37]

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_unittest.cc:2251:
ASSERT_EQ(observer.GetStateAt(4)->action,
On 2011/06/10 17:56:04, sky wrote:
> On 2011/06/10 17:23:55, dpapad wrote:
> > It seems that closing a selected tab does not result in a
TabSelectionChanged
> > notification being sent, that needs to be fixed, right?
> 
> Yes, I think its a good idea to force the notification to be sent even though
> old == new.

No, sending a TabSelectionChanged notification does not depend on "old == new"
but on 
"if (!selection_model().Equals(old_model)) {...}" (see tab_strip_model.cc:1221) 
which is true in this case. So I assume that currently
NotifyIfActiveOrSelectionChagned() is not being called from anywhere within
CloseTabContentsAt(). Adding such a call should solve this problem without
changing the if conditions.

http://codereview.chromium.org/7033048/diff/19059/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/19059/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:1213: TabContentsWrapper*
last_selected_contents = old_contents;
On 2011/06/10 17:56:05, sky wrote:
> nit: just use old_contents instead of creating another varaible.

Done.

[sky, 2011-06-10 19:25:24]

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_unittest.cc:2251:
ASSERT_EQ(observer.GetStateAt(4)->action,
On 2011/06/10 18:21:37, dpapad wrote:
> On 2011/06/10 17:56:04, sky wrote:
> > On 2011/06/10 17:23:55, dpapad wrote:
> > > It seems that closing a selected tab does not result in a
> TabSelectionChanged
> > > notification being sent, that needs to be fixed, right?
> > 
> > Yes, I think its a good idea to force the notification to be sent even
though
> > old == new.
> 
> No, sending a TabSelectionChanged notification does not depend on "old == new"
> but on 
> "if (!selection_model().Equals(old_model)) {...}" (see
tab_strip_model.cc:1221) 
> which is true in this case. So I assume that currently
> NotifyIfActiveOrSelectionChagned() is not being called from anywhere within
> CloseTabContentsAt(). Adding such a call should solve this problem without
> changing the if conditions.

Maybe I'm confused. Lets say you have {0} active and [1-4] selected. If you
detach tab 4 then old_model becomes {0}, [1-3]. This is the same as the new
model, right?

[dpapad, 2011-06-13 21:36:18]

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/7033048/diff/21029/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_unittest.cc:2251:
ASSERT_EQ(observer.GetStateAt(4)->action,
On 2011/06/10 19:25:24, sky wrote:
> On 2011/06/10 18:21:37, dpapad wrote:
> > On 2011/06/10 17:56:04, sky wrote:
> > > On 2011/06/10 17:23:55, dpapad wrote:
> > > > It seems that closing a selected tab does not result in a
> > TabSelectionChanged
> > > > notification being sent, that needs to be fixed, right?
> > > 
> > > Yes, I think its a good idea to force the notification to be sent even
> though
> > > old == new.
> > 
> > No, sending a TabSelectionChanged notification does not depend on "old ==
new"
> > but on 
> > "if (!selection_model().Equals(old_model)) {...}" (see
> tab_strip_model.cc:1221) 
> > which is true in this case. So I assume that currently
> > NotifyIfActiveOrSelectionChagned() is not being called from anywhere within
> > CloseTabContentsAt(). Adding such a call should solve this problem without
> > changing the if conditions.
> 
> Maybe I'm confused. Lets say you have {0} active and [1-4] selected. If you
> detach tab 4 then old_model becomes {0}, [1-3]. This is the same as the new
> model, right?

You are right that the selection seems to not have changed (because we store the
old_model after calling DecrementFrom()), but this is not the only condition
that causes the TabStripSelectionModel::Equals() to return false and therefore
trigger the notification. See my other comment in TabStripModel for more detail.

http://codereview.chromium.org/7033048/diff/31001/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/31001/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:238:
NotifyIfActiveOrSelectionChanged(removed_contents, false, old_model);
Regardless of the old_model being different from the current model, this call is
within "if (index == old_active) {}" and therefore was not reached when closing
a not active, selected tab.

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:242: }
Now this will trigger a notification when a not active, selected tab is being
closed. I updated the unit tests appropriately.

[sky, 2011-06-13 22:20:16]

LGTM

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:242: if (was_selected) {
It's pretty subtle as to why you need this. Please add a comment.

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_observer.h (right):

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_observer.h:72: // but does change the
selection. In this case 'ActiveTabChanged' is not sent.
'ActiveTabChanged' -> |ActiveTabChanged|

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_unittest.cc:276: explicit
MockTabStripModelObserver(TabStripModel* model) :
: on next line.

[dpapad, 2011-06-14 16:58:23]

I am getting a huge amount of browser_tests failing on Windows. Could you please
take a look
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/36728. All of
them display "Did not complete". I have not found what is causing this so far.

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model.cc:242: if (was_selected) {
On 2011/06/13 22:20:16, sky wrote:
> It's pretty subtle as to why you need this. Please add a comment.

Done.

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_observer.h (right):

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_observer.h:72: // but does change the
selection. In this case 'ActiveTabChanged' is not sent.
On 2011/06/13 22:20:16, sky wrote:
> 'ActiveTabChanged' -> |ActiveTabChanged|

Done.

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
chrome/browser/tabs/tab_strip_model_unittest.cc:276: explicit
MockTabStripModelObserver(TabStripModel* model) :
On 2011/06/13 22:20:16, sky wrote:
> : on next line.

Done.

[sky, 2011-06-14 17:22:19]

On Tue, Jun 14, 2011 at 9:58 AM,  <dpapad@chromium.org> wrote:
> I am getting a huge amount of browser_tests failing on Windows. Could you
> please
> take a look
> http://build.chromium.org/p/tryserver.chromium/builders/win/builds/36728.
> All of
> them display "Did not complete". I have not found what is causing this so
> far.

It's not readily obvious to me either. In these cases I'll typically
do another try run. If that still fails, then it's time to open the
debugger:(

  -Scott

>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> File chrome/browser/tabs/tab_strip_model.cc (right):
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> chrome/browser/tabs/tab_strip_model.cc:242: if (was_selected) {
> On 2011/06/13 22:20:16, sky wrote:
>>
>> It's pretty subtle as to why you need this. Please add a comment.
>
> Done.
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> File chrome/browser/tabs/tab_strip_model_observer.h (right):
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> chrome/browser/tabs/tab_strip_model_observer.h:72: // but does change
> the selection. In this case 'ActiveTabChanged' is not sent.
> On 2011/06/13 22:20:16, sky wrote:
>>
>> 'ActiveTabChanged' -> |ActiveTabChanged|
>
> Done.
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> File chrome/browser/tabs/tab_strip_model_unittest.cc (right):
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> chrome/browser/tabs/tab_strip_model_unittest.cc:276: explicit
> MockTabStripModelObserver(TabStripModel* model) :
> On 2011/06/13 22:20:16, sky wrote:
>>
>> : on next line.
>
> Done.
>
> http://codereview.chromium.org/7033048/
>

[dpapad, 2011-06-14 18:45:27]

On 2011/06/14 17:22:19, sky wrote:
> On Tue, Jun 14, 2011 at 9:58 AM,  <mailto:dpapad@chromium.org> wrote:
> > I am getting a huge amount of browser_tests failing on Windows. Could you
> > please
> > take a look
> > http://build.chromium.org/p/tryserver.chromium/builders/win/builds/36728.
> > All of
> > them display "Did not complete". I have not found what is causing this so
> > far.
> 
> It's not readily obvious to me either. In these cases I'll typically
> do another try run. If that still fails, then it's time to open the
> debugger:(
> 
>   -Scott
> 
> >
> >
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> > File chrome/browser/tabs/tab_strip_model.cc (right):
> >
> >
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> > chrome/browser/tabs/tab_strip_model.cc:242: if (was_selected) {
> > On 2011/06/13 22:20:16, sky wrote:
> >>
> >> It's pretty subtle as to why you need this. Please add a comment.
> >
> > Done.
> >
> >
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> > File chrome/browser/tabs/tab_strip_model_observer.h (right):
> >
> >
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> > chrome/browser/tabs/tab_strip_model_observer.h:72: // but does change
> > the selection. In this case 'ActiveTabChanged' is not sent.
> > On 2011/06/13 22:20:16, sky wrote:
> >>
> >> 'ActiveTabChanged' -> |ActiveTabChanged|
> >
> > Done.
> >
> >
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> > File chrome/browser/tabs/tab_strip_model_unittest.cc (right):
> >
> >
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
> > chrome/browser/tabs/tab_strip_model_unittest.cc:276: explicit
> > MockTabStripModelObserver(TabStripModel* model) :
> > On 2011/06/13 22:20:16, sky wrote:
> >>
> >> : on next line.
> >
> > Done.
> >
> > http://codereview.chromium.org/7033048/
> >

It seems that all the failing tests experience the following error,
"... file_util_win.cc(774)] CreateFile failed for path ..."
both locally and on the trybots. I do not see how it is related to my change,
but there has to be some relation (a lot of the tests are tab related).

[sky, 2011-06-14 20:56:38]

It could just be that bot. I would do another try in hopes you get a
different bot.

  -Scott

On Tue, Jun 14, 2011 at 11:45 AM,  <dpapad@chromium.org> wrote:
> On 2011/06/14 17:22:19, sky wrote:
>>
>> On Tue, Jun 14, 2011 at 9:58 AM,  <mailto:dpapad@chromium.org> wrote:
>> > I am getting a huge amount of browser_tests failing on Windows. Could
>> > you
>> > please
>> > take a look
>> >
>> > http://build.chromium.org/p/tryserver.chromium/builders/win/builds/36728.
>> > All of
>> > them display "Did not complete". I have not found what is causing this
>> > so
>> > far.
>
>> It's not readily obvious to me either. In these cases I'll typically
>> do another try run. If that still fails, then it's time to open the
>> debugger:(
>
>>   -Scott
>
>> >
>> >
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
>>
>> > File chrome/browser/tabs/tab_strip_model.cc (right):
>> >
>> >
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
>>
>> > chrome/browser/tabs/tab_strip_model.cc:242: if (was_selected) {
>> > On 2011/06/13 22:20:16, sky wrote:
>> >>
>> >> It's pretty subtle as to why you need this. Please add a comment.
>> >
>> > Done.
>> >
>> >
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
>>
>> > File chrome/browser/tabs/tab_strip_model_observer.h (right):
>> >
>> >
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
>>
>> > chrome/browser/tabs/tab_strip_model_observer.h:72: // but does change
>> > the selection. In this case 'ActiveTabChanged' is not sent.
>> > On 2011/06/13 22:20:16, sky wrote:
>> >>
>> >> 'ActiveTabChanged' -> |ActiveTabChanged|
>> >
>> > Done.
>> >
>> >
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
>>
>> > File chrome/browser/tabs/tab_strip_model_unittest.cc (right):
>> >
>> >
>
>
http://codereview.chromium.org/7033048/diff/19061/chrome/browser/tabs/tab_str...
>>
>> > chrome/browser/tabs/tab_strip_model_unittest.cc:276: explicit
>> > MockTabStripModelObserver(TabStripModel* model) :
>> > On 2011/06/13 22:20:16, sky wrote:
>> >>
>> >> : on next line.
>> >
>> > Done.
>> >
>> > http://codereview.chromium.org/7033048/
>> >
>
> It seems that all the failing tests experience the following error,
> "... file_util_win.cc(774)] CreateFile failed for path ..."
> both locally and on the trybots. I do not see how it is related to my
> change,
> but there has to be some relation (a lot of the tests are tab related).
>
> http://codereview.chromium.org/7033048/
>

[dpapad, 2011-06-16 00:09:07]

I tried to debug the windows failing tests with no success. There seemed to be a
failure within the test macro (my breakpoints on the first line of such a test
where never being hit, some strange assertion was failing within the macro.

I reverted all the windows related changes (in chrome/browser/views, basically
bullet point 3 in this CL's description) and it seems to work fine without them.
Uploaded here just for testing http://codereview.chromium.org/7170033/, all
tryjobs passed.

[sky, 2011-06-16 01:06:30]

Did you run only the failing test and with --single-process?

Without the changes to views I would expect changes to the selection
that only result in TabSelectionChanged being sent to no longer
visually update the tabstrip.

  -Scott

On Wed, Jun 15, 2011 at 5:09 PM,  <dpapad@chromium.org> wrote:
> I tried to debug the windows failing tests with no success. There seemed to
> be a
> failure within the test macro (my breakpoints on the first line of such a
> test
> where never being hit, some strange assertion was failing within the macro.
>
> I reverted all the windows related changes (in chrome/browser/views,
> basically
> bullet point 3 in this CL's description) and it seems to work fine without
> them.
> Uploaded here just for testing http://codereview.chromium.org/7170033/, all
> tryjobs passed.
>
> http://codereview.chromium.org/7033048/
>

[dpapad, 2011-06-16 13:12:37]

On 2011/06/16 01:06:30, sky wrote:
> Did you run only the failing test and with --single-process?
> 
> Without the changes to views I would expect changes to the selection
> that only result in TabSelectionChanged being sent to no longer
> visually update the tabstrip.
> 
>   -Scott
> 

I had not tried --single-process. This revealed a problem that caused (probably)
all the failures. After trying the fix
(http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...)
only 2 tests failed (maybe unrelated). I will update the fix later or on Friday,
since I will be in googleserve most of the day.

[sky, 2011-06-16 16:19:59]

Glad to hear it! Let me know when you want me to take another look.

  -Scott

On Thu, Jun 16, 2011 at 6:12 AM,  <dpapad@chromium.org> wrote:
> On 2011/06/16 01:06:30, sky wrote:
>>
>> Did you run only the failing test and with --single-process?
>
>> Without the changes to views I would expect changes to the selection
>> that only result in TabSelectionChanged being sent to no longer
>> visually update the tabstrip.
>
>>   -Scott
>
>
> I had not tried --single-process. This revealed a problem that caused
> (probably)
> all the failures. After trying the fix
>
(http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...)
> only 2 tests failed (maybe unrelated). I will update the fix later or on
> Friday,
> since I will be in googleserve most of the day.
>
> http://codereview.chromium.org/7033048/
>

[dpapad, 2011-06-17 19:55:23]

I am still trying to debug the remaining two unit tests that fail.

http://codereview.chromium.org/7033048/diff/47001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/7033048/diff/47001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:253: it(no_longer_selected,
no_longer_selected.begin());
This was the cause of most of the unit test failures. |no_longer_selected| had
no space to store the result.

[sky, 2011-06-17 22:18:42]

On Fri, Jun 17, 2011 at 12:55 PM,  <dpapad@chromium.org> wrote:
> I am still trying to debug the remaining two unit tests that fail.
>
>
>
http://codereview.chromium.org/7033048/diff/47001/chrome/browser/ui/views/tab...
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
>
>
http://codereview.chromium.org/7033048/diff/47001/chrome/browser/ui/views/tab...
> chrome/browser/ui/views/tabs/tab_strip.cc:253: it(no_longer_selected,
> no_longer_selected.begin());
> This was the cause of most of the unit test failures.
> |no_longer_selected| had no space to store the result.

GAH! Sorry about that. That's what I get for not testing the code I'm
suggesting.

  -Scott

[dpapad, 2011-06-17 22:49:01]

I think I have found the cause of the remaining two failing unit tests
(SessionRestoreTest.RestoreIndividualTabFromWindow and
SessionRestoreTest.WindowWithOneTab). 

BaseTabStrip::ModelIndexToTabIndex() is returning tab_data_.size() (sometimes).
The returned value is being used to index |tab_data_| in tab_strip.cc:253 (in
this CL) and this results in "vector subscript out of range" error (line 764 in
<vector>). Can you help resolve this issue? I could send you a stack trace if it
would help.

[dpapad, 2011-06-20 17:51:55]

On 2011/06/17 22:49:01, dpapad wrote:
> I think I have found the cause of the remaining two failing unit tests
> (SessionRestoreTest.RestoreIndividualTabFromWindow and
> SessionRestoreTest.WindowWithOneTab). 
> 
> BaseTabStrip::ModelIndexToTabIndex() is returning tab_data_.size()
(sometimes).
> The returned value is being used to index |tab_data_| in tab_strip.cc:253 (in
> this CL) and this results in "vector subscript out of range" error (line 764
in
> <vector>). Can you help resolve this issue? I could send you a stack trace if
it
> would help.

It seems that all unit tests are fixed, finally. I will try to land this today,
is that ok?

[sky, 2011-06-20 18:19:32]

YAY! Land away.

  -Scott

On Mon, Jun 20, 2011 at 10:51 AM,  <dpapad@chromium.org> wrote:
> On 2011/06/17 22:49:01, dpapad wrote:
>>
>> I think I have found the cause of the remaining two failing unit tests
>> (SessionRestoreTest.RestoreIndividualTabFromWindow and
>> SessionRestoreTest.WindowWithOneTab).
>
>> BaseTabStrip::ModelIndexToTabIndex() is returning tab_data_.size()
>
> (sometimes).
>>
>> The returned value is being used to index |tab_data_| in tab_strip.cc:253
>> (in
>> this CL) and this results in "vector subscript out of range" error (line
>> 764
>
> in
>>
>> <vector>). Can you help resolve this issue? I could send you a stack trace
>> if
>
> it
>>
>> would help.
>
> It seems that all unit tests are fixed, finally. I will try to land this
> today,
> is that ok?
>
> http://codereview.chromium.org/7033048/
>