Changes semantics of TabSelectedAt. For multi-selection the TabStrip

Changes semantics of TabSelectedAt. For multi-selection the TabStrip
is going to still have the notion of the selected tab, but I need some
way to notify observers when other aspects of the selection
change. For example, if you shift click to extend the selection the
'selected tab' won't change. I'm changing TabSelectedAt so that if the
selected TabContents hasn't changed the old/new are the same.

This change only adds the necessary code to ignore the new
semantics. Code that actually invokes TabSelectedAt will come later.

BUG=30572
TEST=none

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

[sky, 2011-03-02 03:52:33]

I also considered adding another method that means the 'selection changed but
not selected tab', but that seemed equally unappealing.

  -Scott

[Ben Goodger (Google), 2011-03-02 18:01:46]

Maybe you could rename existing TabSelectedAt to TabActivated or some such. It
seems really awkward to have to do this check at all the call sites - or is
there common code at each call site that both impls would have to do?>

-Ben

[Peter Kasting, 2011-03-02 18:36:52]

Drive-by: Apparently, the bug for tab multiselection is
http://code.google.com/p/chromium/issues/detail?id=30572 .

[sky, 2011-03-02 19:58:50]

On 2011/03/02 18:01:46, Ben Goodger wrote:
> Maybe you could rename existing TabSelectedAt to TabActivated or some such. It
> seems really awkward to have to do this check at all the call sites - or is
> there common code at each call site that both impls would have to do?>
> 
> -Ben

Take a look at the latest. Do you like this better?

  -Scott

[Ben Goodger (Google), 2011-03-02 20:07:27]

LGTM to this first one after discussion

On Tue, Mar 1, 2011 at 7:52 PM, <sky@chromium.org> wrote:

> Reviewers: Ben Goodger,
>
> Message:
> I also considered adding another method that means the 'selection changed
> but
> not selected tab', but that seemed equally unappealing.
>
>  -Scott
>
> Description:
> Changes semantics of TabSelectedAt. For multi-selection the TabStrip
> is going to still have the notion of the selected tab, but I need some
> way to notify observers when other aspects of the selection
> change. For example, if you shift click to extend the selection the
> 'selected tab' won't change. I'm changing TabSelectedAt so that if the
> selected TabContents hasn't changed the old/new are the same.
>
> This change only adds the necessary code to ignore the new
> semantics. Code that actually invokes TabSelectedAt will come later.
>
> BUG=none
> TEST=none
>
> Please review this at http://codereview.chromium.org/6591097/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/aeropeek_manager.cc
>  M chrome/browser/chromeos/wm_overview_controller.cc
>  M chrome/browser/extensions/extension_browser_event_router.cc
>  M chrome/browser/tabs/tab_strip_model_observer.h
>  M chrome/browser/tabs/tab_strip_model_order_controller.cc
>  M chrome/browser/ui/browser.cc
>  M chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
>  M chrome/browser/ui/gtk/browser_window_gtk.cc
>  M chrome/browser/ui/touch/frame/touch_browser_frame_view.cc
>  M chrome/browser/ui/views/frame/browser_view.cc
>
>
> Index: chrome/browser/aeropeek_manager.cc
> diff --git a/chrome/browser/aeropeek_manager.cc
> b/chrome/browser/aeropeek_manager.cc
> index
>
af6a0545bfb5a0d5d56519e81ca0e240246c8341..4d1159d3a74db98e83373d8a1c6662b86612abe0
> 100644
> --- a/chrome/browser/aeropeek_manager.cc
> +++ b/chrome/browser/aeropeek_manager.cc
> @@ -1123,6 +1123,9 @@ void
> AeroPeekManager::TabSelectedAt(TabContentsWrapper* old_contents,
>                                     TabContentsWrapper* new_contents,
>                                     int index,
>                                     bool user_gesture) {
> +  if (old_contents == new_contents)
> +    return;
> +
>   // Deactivate the old window in the thumbnail list and activate the new
> one
>   // to synchronize the thumbnail list with TabStrip.
>   if (old_contents) {
> Index: chrome/browser/chromeos/wm_overview_controller.cc
> diff --git a/chrome/browser/chromeos/wm_overview_controller.cc
> b/chrome/browser/chromeos/wm_overview_controller.cc
> index
>
31e839e13c8cd4d2d8883501411f3ab0413e5551..490e4b4517846a49c83302e16983b7deff3a30eb
> 100644
> --- a/chrome/browser/chromeos/wm_overview_controller.cc
> +++ b/chrome/browser/chromeos/wm_overview_controller.cc
> @@ -253,6 +253,9 @@ void BrowserListener::TabSelectedAt(TabContentsWrapper*
> old_contents,
>                                     TabContentsWrapper* new_contents,
>                                     int index,
>                                     bool user_gesture) {
> +  if (old_contents == new_contents)
> +    return;
> +
>   UpdateSelectedIndex(index);
>  }
>
> Index: chrome/browser/extensions/extension_browser_event_router.cc
> diff --git a/chrome/browser/extensions/extension_browser_event_router.cc
> b/chrome/browser/extensions/extension_browser_event_router.cc
> index
>
e0341a56b28cae3f897d3842e0dd3f932fc69c81..c68e7d03537c9a4c9ce5c286dc0820691f196e8c
> 100644
> --- a/chrome/browser/extensions/extension_browser_event_router.cc
> +++ b/chrome/browser/extensions/extension_browser_event_router.cc
> @@ -349,6 +349,9 @@ void ExtensionBrowserEventRouter::TabSelectedAt(
>     TabContentsWrapper* new_contents,
>     int index,
>     bool user_gesture) {
> +  if (old_contents == new_contents)
> +    return;
> +
>   ListValue args;
>   args.Append(Value::CreateIntegerValue(
>       ExtensionTabUtil::GetTabId(new_contents->tab_contents())));
> Index: chrome/browser/tabs/tab_strip_model_observer.h
> diff --git a/chrome/browser/tabs/tab_strip_model_observer.h
> b/chrome/browser/tabs/tab_strip_model_observer.h
> index
>
2eafec03b446819241e10693f6ea2621baa38829..0c09ac90b5a02f42a991219b530c594e8794bbbf
> 100644
> --- a/chrome/browser/tabs/tab_strip_model_observer.h
> +++ b/chrome/browser/tabs/tab_strip_model_observer.h
> @@ -61,9 +61,16 @@ class TabStripModelObserver {
>   // happens.
>   virtual void TabDeselected(TabContentsWrapper* contents);
>
> -  // The selected TabContents changed from |old_contents| to
> |new_contents| at
> -  // |index|. |user_gesture| specifies whether or not this was done by a
> user
> -  // input event (e.g. clicking on a tab, keystroke) or as a side-effect
> of
> +  // Sent when the selection changes. The previously selected tab is
> identified
> +  // by |old_contents| and the newly selected tab by |new_contents|.
> |index| is
> +  // the index of |new_contents|. When using multiple selection this may
> be sent
> +  // even when the selected tab (as returned by selected_index()) has not
> +  // changed. For example, if the selection is extended this method is
> invoked
> +  // to inform observers the selection has changed, but |old_contents| and
> +  // |new_contents| are the same.  If you only care about when the
> selected tab
> +  // changes, check for when |old_contents| differs from
> +  // |new_contents|. |user_gesture| specifies whether or not this was done
> by a
> +  // user input event (e.g. clicking on a tab, keystroke) or as a
> side-effect of
>   // some other function.
>   virtual void TabSelectedAt(TabContentsWrapper* old_contents,
>                              TabContentsWrapper* new_contents,
> Index: chrome/browser/tabs/tab_strip_model_order_controller.cc
> diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.cc
> b/chrome/browser/tabs/tab_strip_model_order_controller.cc
> index
>
bd309fa4e176e871450bf924d6cb66d4a7879f9f..cc3eaecea7555956562066254836b162096e6416
> 100644
> --- a/chrome/browser/tabs/tab_strip_model_order_controller.cc
> +++ b/chrome/browser/tabs/tab_strip_model_order_controller.cc
> @@ -111,6 +111,9 @@ void TabStripModelOrderController::TabSelectedAt(
>     TabContentsWrapper* new_contents,
>     int index,
>     bool user_gesture) {
> +  if (old_contents == new_contents)
> +    return;
> +
>   NavigationController* old_opener = NULL;
>   if (old_contents) {
>     int index = tabstrip_->GetIndexOfTabContents(old_contents);
> Index: chrome/browser/ui/browser.cc
> diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
> index
>
66c99a2a06206f443c04e0aa9a76f49c483ecd12..d3b403f7e092cc54f4c34aa6778f0f34b81649fb
> 100644
> --- a/chrome/browser/ui/browser.cc
> +++ b/chrome/browser/ui/browser.cc
> @@ -2699,7 +2699,8 @@ void Browser::TabSelectedAt(TabContentsWrapper*
> old_contents,
>                             TabContentsWrapper* new_contents,
>                             int index,
>                             bool user_gesture) {
> -  DCHECK(old_contents != new_contents);
> +  if (old_contents == new_contents)
> +    return;
>
>   // On some platforms we want to automatically reload tabs that are
>   // killed when the user selects them.
> Index: chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
> diff --git
a/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mmb/chrome/browser/ui/cocoa/tabs/
> tab_strip_controller.mm
> index
>
4311bda6645f02e238a9c5eadc9647af35ab58f4..e3488827dbad110fef4bacbab88352e3ce3b7514
> 100644
> --- a/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
> +++ b/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
> @@ -1038,7 +1038,7 @@ private:
>   // Take closing tabs into account.
>   NSInteger index = [self indexFromModelIndex:modelIndex];
>
> -  if (oldContents) {
> +  if (oldContents && oldContents != newContents) {
>     int oldModelIndex =
>         browser_->GetIndexOfController(&(oldContents->controller()));
>     if (oldModelIndex != -1) {  // When closing a tab, the old tab may be
> gone.
> Index: chrome/browser/ui/gtk/browser_window_gtk.cc
> diff --git a/chrome/browser/ui/gtk/browser_window_gtk.cc
> b/chrome/browser/ui/gtk/browser_window_gtk.cc
> index
>
9621a6cf9bd1b03acd36be64152618a1bcc2054f..ab8c0fd0db15ffc5f4dd9e6505aaab77396a6da7
> 100644
> --- a/chrome/browser/ui/gtk/browser_window_gtk.cc
> +++ b/chrome/browser/ui/gtk/browser_window_gtk.cc
> @@ -1129,7 +1129,8 @@ void
> BrowserWindowGtk::TabSelectedAt(TabContentsWrapper* old_contents,
>                                      TabContentsWrapper* new_contents,
>                                      int index,
>                                      bool user_gesture) {
> -  DCHECK(old_contents != new_contents);
> +  if (old_contents == new_contents)
> +    return;
>
>   if (old_contents && !old_contents->tab_contents()->is_being_destroyed())
>     old_contents->view()->StoreFocus();
> Index: chrome/browser/ui/touch/frame/touch_browser_frame_view.cc
> diff --git a/chrome/browser/ui/touch/frame/touch_browser_frame_view.cc
> b/chrome/browser/ui/touch/frame/touch_browser_frame_view.cc
> index
>
0f8e53556623816f474a0edc036ff35299b8f573..641e3af55c32dec4d2ca6b9fc0a17df0c98b09b0
> 100644
> --- a/chrome/browser/ui/touch/frame/touch_browser_frame_view.cc
> +++ b/chrome/browser/ui/touch/frame/touch_browser_frame_view.cc
> @@ -178,6 +178,9 @@ void
> TouchBrowserFrameView::TabSelectedAt(TabContentsWrapper* old_contents,
>                                           TabContentsWrapper* new_contents,
>                                           int index,
>                                           bool user_gesture) {
> +  if (new_contents == old_contents)
> +    return;
> +
>   TabContents* contents = new_contents->tab_contents();
>   bool* editable = GetFocusedStateAccessor()->GetProperty(
>       contents->property_bag());
> Index: chrome/browser/ui/views/frame/browser_view.cc
> diff --git a/chrome/browser/ui/views/frame/browser_view.cc
> b/chrome/browser/ui/views/frame/browser_view.cc
> index
>
64fd8f1a0b3e4815d5d74311ee0b544f58875882..3fd00ee537fac457885a1b650d06c8700fad37a6
> 100644
> --- a/chrome/browser/ui/views/frame/browser_view.cc
> +++ b/chrome/browser/ui/views/frame/browser_view.cc
> @@ -1485,7 +1485,8 @@ void BrowserView::TabSelectedAt(TabContentsWrapper*
> old_contents,
>                                 TabContentsWrapper* new_contents,
>                                 int index,
>                                 bool user_gesture) {
> -  DCHECK(old_contents != new_contents);
> +  if (old_contents == new_contents)
> +    return;
>
>   ProcessTabSelected(new_contents, true);
>  }
>
>
>