Implementation for switching between recently used tabs using ctrl tilde or quoteleft.

Implementation for switching between recently used tabs using ctrl tilde or quoteleft.


BUG=43459
TEST=Done UI Testing and unit test cases.

[NaveenBobbili (Motorola), 2012-04-18 10:38:03]

Hi Scott,
This feature is very similar to alt+tab feature in desktop. We are maintaining
an Most Recently Used tabs list to switch to previously used tab. I have
submitted initial implementation which works. 
Please provide comments about what else is necessary and any other
optimizations. modularization which is required to finish up this feature.

Thanks,
Naveen B

[sky, 2012-04-18 14:37:01]

Thanks for the patch! Before you embark on ui changes like this you
need to run them by the ux folks: Glen, Brian and Ben, which are now
cc'd.

  -Scott

On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
> Reviewers: sky,
>
> Message:
> Hi Scott,
> This feature is very similar to alt+tab feature in desktop. We are
> maintaining
> an Most Recently Used tabs list to switch to previously used tab. I have
> submitted initial implementation which works.
> Please provide comments about what else is necessary and any other
> optimizations. modularization which is required to finish up this feature.
>
> Thanks,
> Naveen B
>
> Description:
> First cut implementation of switching between recently used tabs using ctrl
> tilde or quoteleft.
>
>
> BUG=43459
> TEST=Done UI Testing only.
>
>
> Please review this at https://chromiumcodereview.appspot.com/10117016/
>
> SVN Base: http://git.chromium.org/chromium/src.git@master
>
> Affected files:
>  M chrome/app/chrome_command_ids.h
>  M chrome/app/chrome_dll.rc
>  M chrome/browser/ui/browser.h
>  M chrome/browser/ui/browser.cc
>  M chrome/browser/ui/gtk/accelerators_gtk.cc
>
>
> Index: chrome/app/chrome_command_ids.h
> diff --git a/chrome/app/chrome_command_ids.h
> b/chrome/app/chrome_command_ids.h
> index
>
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
> 100644
> --- a/chrome/app/chrome_command_ids.h
> +++ b/chrome/app/chrome_command_ids.h
> @@ -315,6 +315,9 @@
>  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
>  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
>
> +// Select the last visited tab.
> +#define IDC_SELECT_NEXT_MRU_TAB 52201
> +
>  // NOTE: The last valid command value is 57343 (0xDFFF)
>  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
>
> Index: chrome/app/chrome_dll.rc
> diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
> index
>
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
> 100644
> --- a/chrome/app/chrome_dll.rc
> +++ b/chrome/app/chrome_dll.rc
> @@ -122,6 +122,7 @@ BEGIN
>     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL, SHIFT
>     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
> +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
>  END
>
>  IDR_CHROMEFRAME ACCELERATORS
> Index: chrome/browser/ui/browser.cc
> diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
> index
>
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
> 100644
> --- a/chrome/browser/ui/browser.cc
> +++ b/chrome/browser/ui/browser.cc
> @@ -531,6 +531,9 @@ Browser::~Browser() {
>     select_file_dialog_->ListenerDestroyed();
>
>   TabRestoreServiceDestroyed(tab_restore_service_);
> +
> +  // Clear the tabs mru list
> +  tabs_mru_list_.clear();
>  }
>
>  bool Browser::IsFullscreenForTabOrPending() const {
> @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
>   tab_handler_->GetTabStripModel()->SelectLastTab();
>  }
>
> +void Browser::SelectNextMRUTab() {
> +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
> +  std::list<TabContentsWrapper*>::iterator it = tabs_mru_list_.begin();
> +  // Advance to second element in MRU list if there are more than one tabs
> open.
> +  if (tabs_mru_list_.size() > 1) {
> +    std::advance(it, 1);
> +  }
> +  if (*it) {
> +
>  ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
> +                true);
> +  }
> +}
> +
>  void Browser::DuplicateTab() {
>   content::RecordAction(UserMetricsAction("Duplicate"));
>   DuplicateContentsAt(active_index());
> @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
>                                     break;
>     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();              break;
>
> +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
> break;
> +
>     default:
>       LOG(WARNING) << "Received Unimplemented Command: " << id;
>       break;
> @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
>
>
 ///////////////////////////////////////////////////////////////////////////////
>  // Browser, TabStripModelObserver implementation:
> -
>  void Browser::TabInsertedAt(TabContentsWrapper* contents,
>                             int index,
>                             bool foreground) {
> @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
> contents,
>
>   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
>                  content::Source<WebContents>(contents->web_contents()));
> +
> +  // Insert the tab at the beginning of the MRU list
> +  tabs_mru_list_.push_front(contents);
>  }
>
>  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
> @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
> tab_strip_model,
>
>   // Sever the TabContents' connection back to us.
>   SetAsDelegate(contents, NULL);
> +
> +  // Remove the index if found in MRU List
> +  std::list<TabContentsWrapper*>::iterator it =
> +      std::find(tabs_mru_list_.begin(),
> +                tabs_mru_list_.end(),
> +                contents);
> +  if (it != tabs_mru_list_.end()) {
> +     tabs_mru_list_.erase(it);
> +  }
>  }
>
>  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index) {
>   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
> +
> +  // Remove the index if found in MRU List
> +  std::list<TabContentsWrapper*>::iterator it =
> +      std::find(tabs_mru_list_.begin(),
> +                tabs_mru_list_.end(),
> +                contents);
> +  if (it != tabs_mru_list_.end()) {
> +    tabs_mru_list_.erase(it);
> +  }
>  }
>
>  void Browser::TabDeactivated(TabContentsWrapper* contents) {
> @@ -3603,6 +3641,17 @@ void Browser::ActiveTabChanged(TabContentsWrapper*
> old_contents,
>   }
>
>   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
> +
> +
> +  // Bring the activated tab content to the front of the list
> +  std::list<TabContentsWrapper*>::iterator it =
> +      std::find(tabs_mru_list_.begin(),
> +                tabs_mru_list_.end(),
> +                new_contents);
> +  if (it != tabs_mru_list_.end()) {
> +    tabs_mru_list_.push_front(*it);
> +    tabs_mru_list_.erase(it);
> +  }
>  }
>
>  void Browser::TabMoved(TabContentsWrapper* contents,
> @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
> tab_strip_model,
>
>   content::DevToolsManager::GetInstance()->TabReplaced(
>       old_contents->web_contents(), new_contents->web_contents());
> +
> +  // Replace the tab contents accordingly in the list. If the tab contents
> are
> +  // not found add to the end of the list.
> +  std::list<TabContentsWrapper*>::iterator it =
> +      std::find(tabs_mru_list_.begin(),
> +                tabs_mru_list_.end(),
> +                old_contents);
> +  if (it != tabs_mru_list_.end()) {
> +    tabs_mru_list_.insert(it,new_contents);
> +    tabs_mru_list_.erase(it);
> +  } else {
> +    tabs_mru_list_.push_back(new_contents);
> +  }
>  }
>
>  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents, int
> index) {
> @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
>   //       update BrowserList::CloseAllBrowsers.
>   MessageLoop::current()->PostTask(
>       FROM_HERE, base::Bind(&Browser::CloseFrame,
> weak_factory_.GetWeakPtr()));
> +
> +  // Clear the tabs MRU List
> +  tabs_mru_list_.clear();
>  }
>
>
 ///////////////////////////////////////////////////////////////////////////////
> @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
>   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB, normal_window);
>   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
>                                         normal_window);
> +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
> +                                        normal_window);
>   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT, normal_window);
>   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
> normal_window);
>   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0, normal_window);
> Index: chrome/browser/ui/browser.h
> diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
> index
>
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
> 100644
> --- a/chrome/browser/ui/browser.h
> +++ b/chrome/browser/ui/browser.h
> @@ -10,6 +10,7 @@
>  #include <set>
>  #include <string>
>  #include <vector>
> +#include <list>
>
>  #include "base/basictypes.h"
>  #include "base/compiler_specific.h"
> @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
>                 public ProfileSyncServiceObserver,
>                 public InstantDelegate {
>  public:
> +  // List that maintains the tab indices in the most recently visited order
> +  typedef std::list<TabContentsWrapper*> TabsMRUList;
> +
>   // SessionService::WindowType mirrors these values.  If you add to this
>   // enum, look at SessionService::WindowType to see if it needs to be
>   // updated.
> @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
>   void MoveTabPrevious();
>   void SelectNumberedTab(int index);
>   void SelectLastTab();
> +  void SelectNextMRUTab();
>   void DuplicateTab();
>   void WriteCurrentURLToClipboard();
>   void ConvertPopupToTabbedBrowser();
> @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
>   // before DidEndColorChooser is called.
>   scoped_ptr<content::ColorChooser> color_chooser_;
>
> +  TabsMRUList tabs_mru_list_;
> +
>   DISALLOW_COPY_AND_ASSIGN(Browser);
>  };
>
> Index: chrome/browser/ui/gtk/accelerators_gtk.cc
> diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
> b/chrome/browser/ui/gtk/accelerators_gtk.cc
> index
>
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
> 100644
> --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
> +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
> @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
>   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
>     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
> +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
> +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
>   { GDK_t, IDC_RESTORE_TAB,
>     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>
>

[NaveenBobbili (Motorola), 2012-04-19 07:27:11]

Thanks for letting me know.
This patch is mostly back end work .
I would like to implement a feature which shows a preview of tabs using
ctrl + tilde in order for the user to jump to any tab. Very similar to what
alt + tab shows in desktop or ctrl + tab in opera browser.
Please let me know if it falls inline with chromium browser UX plans.

On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:

> Thanks for the patch! Before you embark on ui changes like this you
> need to run them by the ux folks: Glen, Brian and Ben, which are now
> cc'd.
>
>  -Scott
>
> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
> > Reviewers: sky,
> >
> > Message:
> > Hi Scott,
> > This feature is very similar to alt+tab feature in desktop. We are
> > maintaining
> > an Most Recently Used tabs list to switch to previously used tab. I have
> > submitted initial implementation which works.
> > Please provide comments about what else is necessary and any other
> > optimizations. modularization which is required to finish up this
> feature.
> >
> > Thanks,
> > Naveen B
> >
> > Description:
> > First cut implementation of switching between recently used tabs using
> ctrl
> > tilde or quoteleft.
> >
> >
> > BUG=43459
> > TEST=Done UI Testing only.
> >
> >
> > Please review this at https://chromiumcodereview.appspot.com/10117016/
> >
> > SVN Base: http://git.chromium.org/chromium/src.git@master
> >
> > Affected files:
> >  M chrome/app/chrome_command_ids.h
> >  M chrome/app/chrome_dll.rc
> >  M chrome/browser/ui/browser.h
> >  M chrome/browser/ui/browser.cc
> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
> >
> >
> > Index: chrome/app/chrome_command_ids.h
> > diff --git a/chrome/app/chrome_command_ids.h
> > b/chrome/app/chrome_command_ids.h
> > index
> >
>
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
> > 100644
> > --- a/chrome/app/chrome_command_ids.h
> > +++ b/chrome/app/chrome_command_ids.h
> > @@ -315,6 +315,9 @@
> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
> >
> > +// Select the last visited tab.
> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
> > +
> >  // NOTE: The last valid command value is 57343 (0xDFFF)
> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
> >
> > Index: chrome/app/chrome_dll.rc
> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
> > index
> >
>
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
> > 100644
> > --- a/chrome/app/chrome_dll.rc
> > +++ b/chrome/app/chrome_dll.rc
> > @@ -122,6 +122,7 @@ BEGIN
> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL, SHIFT
> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
> >  END
> >
> >  IDR_CHROMEFRAME ACCELERATORS
> > Index: chrome/browser/ui/browser.cc
> > diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
> > index
> >
>
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
> > 100644
> > --- a/chrome/browser/ui/browser.cc
> > +++ b/chrome/browser/ui/browser.cc
> > @@ -531,6 +531,9 @@ Browser::~Browser() {
> >     select_file_dialog_->ListenerDestroyed();
> >
> >   TabRestoreServiceDestroyed(tab_restore_service_);
> > +
> > +  // Clear the tabs mru list
> > +  tabs_mru_list_.clear();
> >  }
> >
> >  bool Browser::IsFullscreenForTabOrPending() const {
> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
> >   tab_handler_->GetTabStripModel()->SelectLastTab();
> >  }
> >
> > +void Browser::SelectNextMRUTab() {
> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
> > +  std::list<TabContentsWrapper*>::iterator it = tabs_mru_list_.begin();
> > +  // Advance to second element in MRU list if there are more than one
> tabs
> > open.
> > +  if (tabs_mru_list_.size() > 1) {
> > +    std::advance(it, 1);
> > +  }
> > +  if (*it) {
> > +
> >
>  ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
> > +                true);
> > +  }
> > +}
> > +
> >  void Browser::DuplicateTab() {
> >   content::RecordAction(UserMetricsAction("Duplicate"));
> >   DuplicateContentsAt(active_index());
> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
> >                                     break;
> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
>  break;
> >
> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
> > break;
> > +
> >     default:
> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
> >       break;
> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
> >
> >
> 
///////////////////////////////////////////////////////////////////////////////
> >  // Browser, TabStripModelObserver implementation:
> > -
> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
> >                             int index,
> >                             bool foreground) {
> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
> > contents,
> >
> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
> >                  content::Source<WebContents>(contents->web_contents()));
> > +
> > +  // Insert the tab at the beginning of the MRU list
> > +  tabs_mru_list_.push_front(contents);
> >  }
> >
> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
> > tab_strip_model,
> >
> >   // Sever the TabContents' connection back to us.
> >   SetAsDelegate(contents, NULL);
> > +
> > +  // Remove the index if found in MRU List
> > +  std::list<TabContentsWrapper*>::iterator it =
> > +      std::find(tabs_mru_list_.begin(),
> > +                tabs_mru_list_.end(),
> > +                contents);
> > +  if (it != tabs_mru_list_.end()) {
> > +     tabs_mru_list_.erase(it);
> > +  }
> >  }
> >
> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index) {
> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
> > +
> > +  // Remove the index if found in MRU List
> > +  std::list<TabContentsWrapper*>::iterator it =
> > +      std::find(tabs_mru_list_.begin(),
> > +                tabs_mru_list_.end(),
> > +                contents);
> > +  if (it != tabs_mru_list_.end()) {
> > +    tabs_mru_list_.erase(it);
> > +  }
> >  }
> >
> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
> > @@ -3603,6 +3641,17 @@ void Browser::ActiveTabChanged(TabContentsWrapper*
> > old_contents,
> >   }
> >
> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
> > +
> > +
> > +  // Bring the activated tab content to the front of the list
> > +  std::list<TabContentsWrapper*>::iterator it =
> > +      std::find(tabs_mru_list_.begin(),
> > +                tabs_mru_list_.end(),
> > +                new_contents);
> > +  if (it != tabs_mru_list_.end()) {
> > +    tabs_mru_list_.push_front(*it);
> > +    tabs_mru_list_.erase(it);
> > +  }
> >  }
> >
> >  void Browser::TabMoved(TabContentsWrapper* contents,
> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
> > tab_strip_model,
> >
> >   content::DevToolsManager::GetInstance()->TabReplaced(
> >       old_contents->web_contents(), new_contents->web_contents());
> > +
> > +  // Replace the tab contents accordingly in the list. If the tab
> contents
> > are
> > +  // not found add to the end of the list.
> > +  std::list<TabContentsWrapper*>::iterator it =
> > +      std::find(tabs_mru_list_.begin(),
> > +                tabs_mru_list_.end(),
> > +                old_contents);
> > +  if (it != tabs_mru_list_.end()) {
> > +    tabs_mru_list_.insert(it,new_contents);
> > +    tabs_mru_list_.erase(it);
> > +  } else {
> > +    tabs_mru_list_.push_back(new_contents);
> > +  }
> >  }
> >
> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents, int
> > index) {
> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
> >   //       update BrowserList::CloseAllBrowsers.
> >   MessageLoop::current()->PostTask(
> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
> > weak_factory_.GetWeakPtr()));
> > +
> > +  // Clear the tabs MRU List
> > +  tabs_mru_list_.clear();
> >  }
> >
> >
> 
///////////////////////////////////////////////////////////////////////////////
> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
> normal_window);
> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
> >                                         normal_window);
> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
> > +                                        normal_window);
> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
> normal_window);
> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
> > normal_window);
> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0, normal_window);
> > Index: chrome/browser/ui/browser.h
> > diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
> > index
> >
>
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
> > 100644
> > --- a/chrome/browser/ui/browser.h
> > +++ b/chrome/browser/ui/browser.h
> > @@ -10,6 +10,7 @@
> >  #include <set>
> >  #include <string>
> >  #include <vector>
> > +#include <list>
> >
> >  #include "base/basictypes.h"
> >  #include "base/compiler_specific.h"
> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
> >                 public ProfileSyncServiceObserver,
> >                 public InstantDelegate {
> >  public:
> > +  // List that maintains the tab indices in the most recently visited
> order
> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
> > +
> >   // SessionService::WindowType mirrors these values.  If you add to this
> >   // enum, look at SessionService::WindowType to see if it needs to be
> >   // updated.
> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
> >   void MoveTabPrevious();
> >   void SelectNumberedTab(int index);
> >   void SelectLastTab();
> > +  void SelectNextMRUTab();
> >   void DuplicateTab();
> >   void WriteCurrentURLToClipboard();
> >   void ConvertPopupToTabbedBrowser();
> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
> >   // before DidEndColorChooser is called.
> >   scoped_ptr<content::ColorChooser> color_chooser_;
> >
> > +  TabsMRUList tabs_mru_list_;
> > +
> >   DISALLOW_COPY_AND_ASSIGN(Browser);
> >  };
> >
> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
> > index
> >
>
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
> > 100644
> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
> >   { GDK_t, IDC_RESTORE_TAB,
> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
> >
> >
>

[sky, 2012-04-19 15:45:01]

We had such a feature early on in Chrome's life time but removed it
because it was unsatisfying.

  -Scott

On Thu, Apr 19, 2012 at 12:27 AM, Naveen Bobbili <qghc36@motorola.com> wrote:
> Thanks for letting me know.
> This patch is mostly back end work .
> I would like to implement a feature which shows a preview of tabs using ctrl
> + tilde in order for the user to jump to any tab. Very similar to what alt +
> tab shows in desktop or ctrl + tab in opera browser.
> Please let me know if it falls inline with chromium browser UX plans.
>
>
> On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Thanks for the patch! Before you embark on ui changes like this you
>> need to run them by the ux folks: Glen, Brian and Ben, which are now
>> cc'd.
>>
>>  -Scott
>>
>> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
>> > Reviewers: sky,
>> >
>> > Message:
>> > Hi Scott,
>> > This feature is very similar to alt+tab feature in desktop. We are
>> > maintaining
>> > an Most Recently Used tabs list to switch to previously used tab. I have
>> > submitted initial implementation which works.
>> > Please provide comments about what else is necessary and any other
>> > optimizations. modularization which is required to finish up this
>> > feature.
>> >
>> > Thanks,
>> > Naveen B
>> >
>> > Description:
>> > First cut implementation of switching between recently used tabs using
>> > ctrl
>> > tilde or quoteleft.
>> >
>> >
>> > BUG=43459
>> > TEST=Done UI Testing only.
>> >
>> >
>> > Please review this at https://chromiumcodereview.appspot.com/10117016/
>> >
>> > SVN Base: http://git.chromium.org/chromium/src.git@master
>> >
>> > Affected files:
>> >  M chrome/app/chrome_command_ids.h
>> >  M chrome/app/chrome_dll.rc
>> >  M chrome/browser/ui/browser.h
>> >  M chrome/browser/ui/browser.cc
>> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
>> >
>> >
>> > Index: chrome/app/chrome_command_ids.h
>> > diff --git a/chrome/app/chrome_command_ids.h
>> > b/chrome/app/chrome_command_ids.h
>> > index
>> >
>> >
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
>> > 100644
>> > --- a/chrome/app/chrome_command_ids.h
>> > +++ b/chrome/app/chrome_command_ids.h
>> > @@ -315,6 +315,9 @@
>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
>> >
>> > +// Select the last visited tab.
>> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
>> > +
>> >  // NOTE: The last valid command value is 57343 (0xDFFF)
>> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
>> >
>> > Index: chrome/app/chrome_dll.rc
>> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
>> > index
>> >
>> >
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
>> > 100644
>> > --- a/chrome/app/chrome_dll.rc
>> > +++ b/chrome/app/chrome_dll.rc
>> > @@ -122,6 +122,7 @@ BEGIN
>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL, SHIFT
>> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
>> >  END
>> >
>> >  IDR_CHROMEFRAME ACCELERATORS
>> > Index: chrome/browser/ui/browser.cc
>> > diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
>> > index
>> >
>> >
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
>> > 100644
>> > --- a/chrome/browser/ui/browser.cc
>> > +++ b/chrome/browser/ui/browser.cc
>> > @@ -531,6 +531,9 @@ Browser::~Browser() {
>> >     select_file_dialog_->ListenerDestroyed();
>> >
>> >   TabRestoreServiceDestroyed(tab_restore_service_);
>> > +
>> > +  // Clear the tabs mru list
>> > +  tabs_mru_list_.clear();
>> >  }
>> >
>> >  bool Browser::IsFullscreenForTabOrPending() const {
>> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
>> >   tab_handler_->GetTabStripModel()->SelectLastTab();
>> >  }
>> >
>> > +void Browser::SelectNextMRUTab() {
>> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
>> > +  std::list<TabContentsWrapper*>::iterator it = tabs_mru_list_.begin();
>> > +  // Advance to second element in MRU list if there are more than one
>> > tabs
>> > open.
>> > +  if (tabs_mru_list_.size() > 1) {
>> > +    std::advance(it, 1);
>> > +  }
>> > +  if (*it) {
>> > +
>> >
>> >
 ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
>> > +                true);
>> > +  }
>> > +}
>> > +
>> >  void Browser::DuplicateTab() {
>> >   content::RecordAction(UserMetricsAction("Duplicate"));
>> >   DuplicateContentsAt(active_index());
>> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
>> >                                     break;
>> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
>> >  break;
>> >
>> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
>> > break;
>> > +
>> >     default:
>> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
>> >       break;
>> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
>> >
>> >
>> >
 ///////////////////////////////////////////////////////////////////////////////
>> >  // Browser, TabStripModelObserver implementation:
>> > -
>> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
>> >                             int index,
>> >                             bool foreground) {
>> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
>> > contents,
>> >
>> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
>> >
>> >  content::Source<WebContents>(contents->web_contents()));
>> > +
>> > +  // Insert the tab at the beginning of the MRU list
>> > +  tabs_mru_list_.push_front(contents);
>> >  }
>> >
>> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
>> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
>> > tab_strip_model,
>> >
>> >   // Sever the TabContents' connection back to us.
>> >   SetAsDelegate(contents, NULL);
>> > +
>> > +  // Remove the index if found in MRU List
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +     tabs_mru_list_.erase(it);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index) {
>> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
>> > +
>> > +  // Remove the index if found in MRU List
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +    tabs_mru_list_.erase(it);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
>> > @@ -3603,6 +3641,17 @@ void
>> > Browser::ActiveTabChanged(TabContentsWrapper*
>> > old_contents,
>> >   }
>> >
>> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
>> > +
>> > +
>> > +  // Bring the activated tab content to the front of the list
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                new_contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +    tabs_mru_list_.push_front(*it);
>> > +    tabs_mru_list_.erase(it);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabMoved(TabContentsWrapper* contents,
>> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
>> > tab_strip_model,
>> >
>> >   content::DevToolsManager::GetInstance()->TabReplaced(
>> >       old_contents->web_contents(), new_contents->web_contents());
>> > +
>> > +  // Replace the tab contents accordingly in the list. If the tab
>> > contents
>> > are
>> > +  // not found add to the end of the list.
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                old_contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +    tabs_mru_list_.insert(it,new_contents);
>> > +    tabs_mru_list_.erase(it);
>> > +  } else {
>> > +    tabs_mru_list_.push_back(new_contents);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents, int
>> > index) {
>> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
>> >   //       update BrowserList::CloseAllBrowsers.
>> >   MessageLoop::current()->PostTask(
>> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
>> > weak_factory_.GetWeakPtr()));
>> > +
>> > +  // Clear the tabs MRU List
>> > +  tabs_mru_list_.clear();
>> >  }
>> >
>> >
>> >
 ///////////////////////////////////////////////////////////////////////////////
>> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
>> > normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
>> >                                         normal_window);
>> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
>> > +                                        normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
>> > normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
>> > normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0,
>> > normal_window);
>> > Index: chrome/browser/ui/browser.h
>> > diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
>> > index
>> >
>> >
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
>> > 100644
>> > --- a/chrome/browser/ui/browser.h
>> > +++ b/chrome/browser/ui/browser.h
>> > @@ -10,6 +10,7 @@
>> >  #include <set>
>> >  #include <string>
>> >  #include <vector>
>> > +#include <list>
>> >
>> >  #include "base/basictypes.h"
>> >  #include "base/compiler_specific.h"
>> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
>> >                 public ProfileSyncServiceObserver,
>> >                 public InstantDelegate {
>> >  public:
>> > +  // List that maintains the tab indices in the most recently visited
>> > order
>> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
>> > +
>> >   // SessionService::WindowType mirrors these values.  If you add to
>> > this
>> >   // enum, look at SessionService::WindowType to see if it needs to be
>> >   // updated.
>> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
>> >   void MoveTabPrevious();
>> >   void SelectNumberedTab(int index);
>> >   void SelectLastTab();
>> > +  void SelectNextMRUTab();
>> >   void DuplicateTab();
>> >   void WriteCurrentURLToClipboard();
>> >   void ConvertPopupToTabbedBrowser();
>> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
>> >   // before DidEndColorChooser is called.
>> >   scoped_ptr<content::ColorChooser> color_chooser_;
>> >
>> > +  TabsMRUList tabs_mru_list_;
>> > +
>> >   DISALLOW_COPY_AND_ASSIGN(Browser);
>> >  };
>> >
>> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
>> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > index
>> >
>> >
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
>> > 100644
>> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
>> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
>> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
>> >   { GDK_t, IDC_RESTORE_TAB,
>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>> >
>> >
>
>

[glen, 2012-04-23 22:21:58]

We shouldn't do a preview (we should just switch the tab, as we do
with Ctrl+Tab), but we're good to have Ctrl+~ be MRU.


On Thu, Apr 19, 2012 at 8:45 AM, Scott Violet <sky@chromium.org> wrote:
> We had such a feature early on in Chrome's life time but removed it
> because it was unsatisfying.
>
>  -Scott
>
> On Thu, Apr 19, 2012 at 12:27 AM, Naveen Bobbili <qghc36@motorola.com> wrote:
>> Thanks for letting me know.
>> This patch is mostly back end work .
>> I would like to implement a feature which shows a preview of tabs using ctrl
>> + tilde in order for the user to jump to any tab. Very similar to what alt +
>> tab shows in desktop or ctrl + tab in opera browser.
>> Please let me know if it falls inline with chromium browser UX plans.
>>
>>
>> On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:
>>>
>>> Thanks for the patch! Before you embark on ui changes like this you
>>> need to run them by the ux folks: Glen, Brian and Ben, which are now
>>> cc'd.
>>>
>>>  -Scott
>>>
>>> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
>>> > Reviewers: sky,
>>> >
>>> > Message:
>>> > Hi Scott,
>>> > This feature is very similar to alt+tab feature in desktop. We are
>>> > maintaining
>>> > an Most Recently Used tabs list to switch to previously used tab. I have
>>> > submitted initial implementation which works.
>>> > Please provide comments about what else is necessary and any other
>>> > optimizations. modularization which is required to finish up this
>>> > feature.
>>> >
>>> > Thanks,
>>> > Naveen B
>>> >
>>> > Description:
>>> > First cut implementation of switching between recently used tabs using
>>> > ctrl
>>> > tilde or quoteleft.
>>> >
>>> >
>>> > BUG=43459
>>> > TEST=Done UI Testing only.
>>> >
>>> >
>>> > Please review this at https://chromiumcodereview.appspot.com/10117016/
>>> >
>>> > SVN Base: http://git.chromium.org/chromium/src.git@master
>>> >
>>> > Affected files:
>>> >  M chrome/app/chrome_command_ids.h
>>> >  M chrome/app/chrome_dll.rc
>>> >  M chrome/browser/ui/browser.h
>>> >  M chrome/browser/ui/browser.cc
>>> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
>>> >
>>> >
>>> > Index: chrome/app/chrome_command_ids.h
>>> > diff --git a/chrome/app/chrome_command_ids.h
>>> > b/chrome/app/chrome_command_ids.h
>>> > index
>>> >
>>> >
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
>>> > 100644
>>> > --- a/chrome/app/chrome_command_ids.h
>>> > +++ b/chrome/app/chrome_command_ids.h
>>> > @@ -315,6 +315,9 @@
>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
>>> >
>>> > +// Select the last visited tab.
>>> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
>>> > +
>>> >  // NOTE: The last valid command value is 57343 (0xDFFF)
>>> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
>>> >
>>> > Index: chrome/app/chrome_dll.rc
>>> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
>>> > index
>>> >
>>> >
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
>>> > 100644
>>> > --- a/chrome/app/chrome_dll.rc
>>> > +++ b/chrome/app/chrome_dll.rc
>>> > @@ -122,6 +122,7 @@ BEGIN
>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL, SHIFT
>>> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
>>> >  END
>>> >
>>> >  IDR_CHROMEFRAME ACCELERATORS
>>> > Index: chrome/browser/ui/browser.cc
>>> > diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
>>> > index
>>> >
>>> >
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
>>> > 100644
>>> > --- a/chrome/browser/ui/browser.cc
>>> > +++ b/chrome/browser/ui/browser.cc
>>> > @@ -531,6 +531,9 @@ Browser::~Browser() {
>>> >     select_file_dialog_->ListenerDestroyed();
>>> >
>>> >   TabRestoreServiceDestroyed(tab_restore_service_);
>>> > +
>>> > +  // Clear the tabs mru list
>>> > +  tabs_mru_list_.clear();
>>> >  }
>>> >
>>> >  bool Browser::IsFullscreenForTabOrPending() const {
>>> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
>>> >   tab_handler_->GetTabStripModel()->SelectLastTab();
>>> >  }
>>> >
>>> > +void Browser::SelectNextMRUTab() {
>>> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
>>> > +  std::list<TabContentsWrapper*>::iterator it = tabs_mru_list_.begin();
>>> > +  // Advance to second element in MRU list if there are more than one
>>> > tabs
>>> > open.
>>> > +  if (tabs_mru_list_.size() > 1) {
>>> > +    std::advance(it, 1);
>>> > +  }
>>> > +  if (*it) {
>>> > +
>>> >
>>> >
 ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
>>> > +                true);
>>> > +  }
>>> > +}
>>> > +
>>> >  void Browser::DuplicateTab() {
>>> >   content::RecordAction(UserMetricsAction("Duplicate"));
>>> >   DuplicateContentsAt(active_index());
>>> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
>>> >                                     break;
>>> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
>>> >  break;
>>> >
>>> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
>>> > break;
>>> > +
>>> >     default:
>>> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
>>> >       break;
>>> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
>>> >
>>> >
>>> >
 ///////////////////////////////////////////////////////////////////////////////
>>> >  // Browser, TabStripModelObserver implementation:
>>> > -
>>> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
>>> >                             int index,
>>> >                             bool foreground) {
>>> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
>>> > contents,
>>> >
>>> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
>>> >
>>> >  content::Source<WebContents>(contents->web_contents()));
>>> > +
>>> > +  // Insert the tab at the beginning of the MRU list
>>> > +  tabs_mru_list_.push_front(contents);
>>> >  }
>>> >
>>> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
>>> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
>>> > tab_strip_model,
>>> >
>>> >   // Sever the TabContents' connection back to us.
>>> >   SetAsDelegate(contents, NULL);
>>> > +
>>> > +  // Remove the index if found in MRU List
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +     tabs_mru_list_.erase(it);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index) {
>>> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
>>> > +
>>> > +  // Remove the index if found in MRU List
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +    tabs_mru_list_.erase(it);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
>>> > @@ -3603,6 +3641,17 @@ void
>>> > Browser::ActiveTabChanged(TabContentsWrapper*
>>> > old_contents,
>>> >   }
>>> >
>>> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
>>> > +
>>> > +
>>> > +  // Bring the activated tab content to the front of the list
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                new_contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +    tabs_mru_list_.push_front(*it);
>>> > +    tabs_mru_list_.erase(it);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabMoved(TabContentsWrapper* contents,
>>> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
>>> > tab_strip_model,
>>> >
>>> >   content::DevToolsManager::GetInstance()->TabReplaced(
>>> >       old_contents->web_contents(), new_contents->web_contents());
>>> > +
>>> > +  // Replace the tab contents accordingly in the list. If the tab
>>> > contents
>>> > are
>>> > +  // not found add to the end of the list.
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                old_contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +    tabs_mru_list_.insert(it,new_contents);
>>> > +    tabs_mru_list_.erase(it);
>>> > +  } else {
>>> > +    tabs_mru_list_.push_back(new_contents);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents, int
>>> > index) {
>>> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
>>> >   //       update BrowserList::CloseAllBrowsers.
>>> >   MessageLoop::current()->PostTask(
>>> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
>>> > weak_factory_.GetWeakPtr()));
>>> > +
>>> > +  // Clear the tabs MRU List
>>> > +  tabs_mru_list_.clear();
>>> >  }
>>> >
>>> >
>>> >
 ///////////////////////////////////////////////////////////////////////////////
>>> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
>>> > normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
>>> >                                         normal_window);
>>> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
>>> > +                                        normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
>>> > normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
>>> > normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0,
>>> > normal_window);
>>> > Index: chrome/browser/ui/browser.h
>>> > diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
>>> > index
>>> >
>>> >
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
>>> > 100644
>>> > --- a/chrome/browser/ui/browser.h
>>> > +++ b/chrome/browser/ui/browser.h
>>> > @@ -10,6 +10,7 @@
>>> >  #include <set>
>>> >  #include <string>
>>> >  #include <vector>
>>> > +#include <list>
>>> >
>>> >  #include "base/basictypes.h"
>>> >  #include "base/compiler_specific.h"
>>> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
>>> >                 public ProfileSyncServiceObserver,
>>> >                 public InstantDelegate {
>>> >  public:
>>> > +  // List that maintains the tab indices in the most recently visited
>>> > order
>>> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
>>> > +
>>> >   // SessionService::WindowType mirrors these values.  If you add to
>>> > this
>>> >   // enum, look at SessionService::WindowType to see if it needs to be
>>> >   // updated.
>>> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
>>> >   void MoveTabPrevious();
>>> >   void SelectNumberedTab(int index);
>>> >   void SelectLastTab();
>>> > +  void SelectNextMRUTab();
>>> >   void DuplicateTab();
>>> >   void WriteCurrentURLToClipboard();
>>> >   void ConvertPopupToTabbedBrowser();
>>> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
>>> >   // before DidEndColorChooser is called.
>>> >   scoped_ptr<content::ColorChooser> color_chooser_;
>>> >
>>> > +  TabsMRUList tabs_mru_list_;
>>> > +
>>> >   DISALLOW_COPY_AND_ASSIGN(Browser);
>>> >  };
>>> >
>>> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > index
>>> >
>>> >
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
>>> > 100644
>>> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
>>> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
>>> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
>>> >   { GDK_t, IDC_RESTORE_TAB,
>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>> >
>>> >
>>
>>

[NaveenBobbili (Motorola), 2012-04-24 05:02:31]

Hi Glen,
Thanks for your comments.
Regards,
Naveen.B

Hi Scott,
Can you please review this change during your free cycles?

Thanks,
Naveen.B


On Tue, Apr 24, 2012 at 3:51 AM, Glen Murphy <glen@google.com> wrote:

> We shouldn't do a preview (we should just switch the tab, as we do
> with Ctrl+Tab), but we're good to have Ctrl+~ be MRU.
>
>
> On Thu, Apr 19, 2012 at 8:45 AM, Scott Violet <sky@chromium.org> wrote:
> > We had such a feature early on in Chrome's life time but removed it
> > because it was unsatisfying.
> >
> >  -Scott
> >
> > On Thu, Apr 19, 2012 at 12:27 AM, Naveen Bobbili <qghc36@motorola.com>
> wrote:
> >> Thanks for letting me know.
> >> This patch is mostly back end work .
> >> I would like to implement a feature which shows a preview of tabs using
> ctrl
> >> + tilde in order for the user to jump to any tab. Very similar to what
> alt +
> >> tab shows in desktop or ctrl + tab in opera browser.
> >> Please let me know if it falls inline with chromium browser UX plans.
> >>
> >>
> >> On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:
> >>>
> >>> Thanks for the patch! Before you embark on ui changes like this you
> >>> need to run them by the ux folks: Glen, Brian and Ben, which are now
> >>> cc'd.
> >>>
> >>>  -Scott
> >>>
> >>> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
> >>> > Reviewers: sky,
> >>> >
> >>> > Message:
> >>> > Hi Scott,
> >>> > This feature is very similar to alt+tab feature in desktop. We are
> >>> > maintaining
> >>> > an Most Recently Used tabs list to switch to previously used tab. I
> have
> >>> > submitted initial implementation which works.
> >>> > Please provide comments about what else is necessary and any other
> >>> > optimizations. modularization which is required to finish up this
> >>> > feature.
> >>> >
> >>> > Thanks,
> >>> > Naveen B
> >>> >
> >>> > Description:
> >>> > First cut implementation of switching between recently used tabs
> using
> >>> > ctrl
> >>> > tilde or quoteleft.
> >>> >
> >>> >
> >>> > BUG=43459
> >>> > TEST=Done UI Testing only.
> >>> >
> >>> >
> >>> > Please review this at
> https://chromiumcodereview.appspot.com/10117016/
> >>> >
> >>> > SVN Base: http://git.chromium.org/chromium/src.git@master
> >>> >
> >>> > Affected files:
> >>> >  M chrome/app/chrome_command_ids.h
> >>> >  M chrome/app/chrome_dll.rc
> >>> >  M chrome/browser/ui/browser.h
> >>> >  M chrome/browser/ui/browser.cc
> >>> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
> >>> >
> >>> >
> >>> > Index: chrome/app/chrome_command_ids.h
> >>> > diff --git a/chrome/app/chrome_command_ids.h
> >>> > b/chrome/app/chrome_command_ids.h
> >>> > index
> >>> >
> >>> >
>
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
> >>> > 100644
> >>> > --- a/chrome/app/chrome_command_ids.h
> >>> > +++ b/chrome/app/chrome_command_ids.h
> >>> > @@ -315,6 +315,9 @@
> >>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
> >>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
> >>> >
> >>> > +// Select the last visited tab.
> >>> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
> >>> > +
> >>> >  // NOTE: The last valid command value is 57343 (0xDFFF)
> >>> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
> >>> >
> >>> > Index: chrome/app/chrome_dll.rc
> >>> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
> >>> > index
> >>> >
> >>> >
>
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
> >>> > 100644
> >>> > --- a/chrome/app/chrome_dll.rc
> >>> > +++ b/chrome/app/chrome_dll.rc
> >>> > @@ -122,6 +122,7 @@ BEGIN
> >>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
> >>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL,
> SHIFT
> >>> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
> >>> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
> >>> >  END
> >>> >
> >>> >  IDR_CHROMEFRAME ACCELERATORS
> >>> > Index: chrome/browser/ui/browser.cc
> >>> > diff --git a/chrome/browser/ui/browser.cc
> b/chrome/browser/ui/browser.cc
> >>> > index
> >>> >
> >>> >
>
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
> >>> > 100644
> >>> > --- a/chrome/browser/ui/browser.cc
> >>> > +++ b/chrome/browser/ui/browser.cc
> >>> > @@ -531,6 +531,9 @@ Browser::~Browser() {
> >>> >     select_file_dialog_->ListenerDestroyed();
> >>> >
> >>> >   TabRestoreServiceDestroyed(tab_restore_service_);
> >>> > +
> >>> > +  // Clear the tabs mru list
> >>> > +  tabs_mru_list_.clear();
> >>> >  }
> >>> >
> >>> >  bool Browser::IsFullscreenForTabOrPending() const {
> >>> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
> >>> >   tab_handler_->GetTabStripModel()->SelectLastTab();
> >>> >  }
> >>> >
> >>> > +void Browser::SelectNextMRUTab() {
> >>> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
> >>> > +  std::list<TabContentsWrapper*>::iterator it =
> tabs_mru_list_.begin();
> >>> > +  // Advance to second element in MRU list if there are more than
> one
> >>> > tabs
> >>> > open.
> >>> > +  if (tabs_mru_list_.size() > 1) {
> >>> > +    std::advance(it, 1);
> >>> > +  }
> >>> > +  if (*it) {
> >>> > +
> >>> >
> >>> >
>  ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
> >>> > +                true);
> >>> > +  }
> >>> > +}
> >>> > +
> >>> >  void Browser::DuplicateTab() {
> >>> >   content::RecordAction(UserMetricsAction("Duplicate"));
> >>> >   DuplicateContentsAt(active_index());
> >>> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
> >>> >                                     break;
> >>> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
> >>> >  break;
> >>> >
> >>> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
> >>> > break;
> >>> > +
> >>> >     default:
> >>> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
> >>> >       break;
> >>> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
> >>> >
> >>> >
> >>> >
> 
///////////////////////////////////////////////////////////////////////////////
> >>> >  // Browser, TabStripModelObserver implementation:
> >>> > -
> >>> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
> >>> >                             int index,
> >>> >                             bool foreground) {
> >>> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
> >>> > contents,
> >>> >
> >>> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
> >>> >
> >>> >  content::Source<WebContents>(contents->web_contents()));
> >>> > +
> >>> > +  // Insert the tab at the beginning of the MRU list
> >>> > +  tabs_mru_list_.push_front(contents);
> >>> >  }
> >>> >
> >>> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
> >>> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
> >>> > tab_strip_model,
> >>> >
> >>> >   // Sever the TabContents' connection back to us.
> >>> >   SetAsDelegate(contents, NULL);
> >>> > +
> >>> > +  // Remove the index if found in MRU List
> >>> > +  std::list<TabContentsWrapper*>::iterator it =
> >>> > +      std::find(tabs_mru_list_.begin(),
> >>> > +                tabs_mru_list_.end(),
> >>> > +                contents);
> >>> > +  if (it != tabs_mru_list_.end()) {
> >>> > +     tabs_mru_list_.erase(it);
> >>> > +  }
> >>> >  }
> >>> >
> >>> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int
> index) {
> >>> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
> >>> > +
> >>> > +  // Remove the index if found in MRU List
> >>> > +  std::list<TabContentsWrapper*>::iterator it =
> >>> > +      std::find(tabs_mru_list_.begin(),
> >>> > +                tabs_mru_list_.end(),
> >>> > +                contents);
> >>> > +  if (it != tabs_mru_list_.end()) {
> >>> > +    tabs_mru_list_.erase(it);
> >>> > +  }
> >>> >  }
> >>> >
> >>> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
> >>> > @@ -3603,6 +3641,17 @@ void
> >>> > Browser::ActiveTabChanged(TabContentsWrapper*
> >>> > old_contents,
> >>> >   }
> >>> >
> >>> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
> >>> > +
> >>> > +
> >>> > +  // Bring the activated tab content to the front of the list
> >>> > +  std::list<TabContentsWrapper*>::iterator it =
> >>> > +      std::find(tabs_mru_list_.begin(),
> >>> > +                tabs_mru_list_.end(),
> >>> > +                new_contents);
> >>> > +  if (it != tabs_mru_list_.end()) {
> >>> > +    tabs_mru_list_.push_front(*it);
> >>> > +    tabs_mru_list_.erase(it);
> >>> > +  }
> >>> >  }
> >>> >
> >>> >  void Browser::TabMoved(TabContentsWrapper* contents,
> >>> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
> >>> > tab_strip_model,
> >>> >
> >>> >   content::DevToolsManager::GetInstance()->TabReplaced(
> >>> >       old_contents->web_contents(), new_contents->web_contents());
> >>> > +
> >>> > +  // Replace the tab contents accordingly in the list. If the tab
> >>> > contents
> >>> > are
> >>> > +  // not found add to the end of the list.
> >>> > +  std::list<TabContentsWrapper*>::iterator it =
> >>> > +      std::find(tabs_mru_list_.begin(),
> >>> > +                tabs_mru_list_.end(),
> >>> > +                old_contents);
> >>> > +  if (it != tabs_mru_list_.end()) {
> >>> > +    tabs_mru_list_.insert(it,new_contents);
> >>> > +    tabs_mru_list_.erase(it);
> >>> > +  } else {
> >>> > +    tabs_mru_list_.push_back(new_contents);
> >>> > +  }
> >>> >  }
> >>> >
> >>> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents,
> int
> >>> > index) {
> >>> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
> >>> >   //       update BrowserList::CloseAllBrowsers.
> >>> >   MessageLoop::current()->PostTask(
> >>> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
> >>> > weak_factory_.GetWeakPtr()));
> >>> > +
> >>> > +  // Clear the tabs MRU List
> >>> > +  tabs_mru_list_.clear();
> >>> >  }
> >>> >
> >>> >
> >>> >
> 
///////////////////////////////////////////////////////////////////////////////
> >>> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
> >>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
> >>> > normal_window);
> >>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
> >>> >                                         normal_window);
> >>> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
> >>> > +                                        normal_window);
> >>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
> >>> > normal_window);
> >>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
> >>> > normal_window);
> >>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0,
> >>> > normal_window);
> >>> > Index: chrome/browser/ui/browser.h
> >>> > diff --git a/chrome/browser/ui/browser.h
> b/chrome/browser/ui/browser.h
> >>> > index
> >>> >
> >>> >
>
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
> >>> > 100644
> >>> > --- a/chrome/browser/ui/browser.h
> >>> > +++ b/chrome/browser/ui/browser.h
> >>> > @@ -10,6 +10,7 @@
> >>> >  #include <set>
> >>> >  #include <string>
> >>> >  #include <vector>
> >>> > +#include <list>
> >>> >
> >>> >  #include "base/basictypes.h"
> >>> >  #include "base/compiler_specific.h"
> >>> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
> >>> >                 public ProfileSyncServiceObserver,
> >>> >                 public InstantDelegate {
> >>> >  public:
> >>> > +  // List that maintains the tab indices in the most recently
> visited
> >>> > order
> >>> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
> >>> > +
> >>> >   // SessionService::WindowType mirrors these values.  If you add to
> >>> > this
> >>> >   // enum, look at SessionService::WindowType to see if it needs to
> be
> >>> >   // updated.
> >>> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
> >>> >   void MoveTabPrevious();
> >>> >   void SelectNumberedTab(int index);
> >>> >   void SelectLastTab();
> >>> > +  void SelectNextMRUTab();
> >>> >   void DuplicateTab();
> >>> >   void WriteCurrentURLToClipboard();
> >>> >   void ConvertPopupToTabbedBrowser();
> >>> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
> >>> >   // before DidEndColorChooser is called.
> >>> >   scoped_ptr<content::ColorChooser> color_chooser_;
> >>> >
> >>> > +  TabsMRUList tabs_mru_list_;
> >>> > +
> >>> >   DISALLOW_COPY_AND_ASSIGN(Browser);
> >>> >  };
> >>> >
> >>> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
> >>> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
> >>> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
> >>> > index
> >>> >
> >>> >
>
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
> >>> > 100644
> >>> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
> >>> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
> >>> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
> >>> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
> >>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
> >>> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
> >>> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
> >>> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
> >>> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
> >>> >   { GDK_t, IDC_RESTORE_TAB,
> >>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
> >>> >
> >>> >
> >>
> >>
>

[sky, 2012-04-24 15:44:04]

We should track this in the TabStripModel. It's a more natural place.
Additionally add unit tests for coverage.

[NaveenBobbili (Motorola), 2012-05-21 09:26:46]

Hi,
Please review the new patchset.

Thanks,
Naveen.B

[sky, 2012-05-22 15:39:40]

On second thought I think we should move this into a standalone class

[NaveenBobbili (Motorola), 2012-05-25 09:19:24]

Please review patchset 2
Added a seperate class for handling MRU list management.

[sky, 2012-05-25 17:56:52]

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/app/chrome_...
File chrome/app/chrome_command_ids.h (right):

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/app/chrome_...
chrome/app/chrome_command_ids.h:318: #define IDC_SELECT_NEXT_MRU_TAB 52201
This should be in window management command close by the SELECT_TAB ones.

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/app/chrome_...
File chrome/app/chrome_dll.rc (right):

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/app/chrome_...
chrome/app/chrome_dll.rc:125: VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,   
VIRTKEY, CONTROL
We shouldn't use the key see
http://code.google.com/p/chromium/issues/detail?id=3696 . Either way though,
this should be behind a flag.

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/browser/tab...
File chrome/browser/tabs/tab_mru_list_manager.h (right):

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/browser/tab...
chrome/browser/tabs/tab_mru_list_manager.h:5: #ifndef
CHROME_BROWSER_TABS_TAB_MRU_LIST_MANAGER_H_
This should be in chrome/browser/ui/tabs.

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/browser/tab...
chrome/browser/tabs/tab_mru_list_manager.h:20: class TabMRUListManager {
TabMRUListManager should be a TabStripModelObserver.

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/browser/tab...
chrome/browser/tabs/tab_mru_list_manager.h:22: virtual ~TabMRUListManager();
no virtual

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/browser/tab...
chrome/browser/tabs/tab_mru_list_manager.h:36: };
DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/10117016/diff/12002/chrome/browser/tab...
chrome/browser/tabs/tab_mru_list_manager.h:37: #endif //
CHROME_BROWSER_TABS_TAB_MRU_LIST_MANAGER_H_
newline between 36/67.

[NaveenBobbili (Motorola), 2012-05-29 11:41:05]

Please review patch 4.
Now that I have removed all my changes from tab_strip_model pls recommend a
place where I could add my unit tests.

http://codereview.chromium.org/10117016/diff/12002/chrome/app/chrome_command_...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/10117016/diff/12002/chrome/app/chrome_command_...
chrome/app/chrome_command_ids.h:318: #define IDC_SELECT_NEXT_MRU_TAB 52201
On 2012/05/25 17:56:52, sky wrote:
> This should be in window management command close by the SELECT_TAB ones.

Done.

http://codereview.chromium.org/10117016/diff/12002/chrome/app/chrome_dll.rc
File chrome/app/chrome_dll.rc (right):

http://codereview.chromium.org/10117016/diff/12002/chrome/app/chrome_dll.rc#n...
chrome/app/chrome_dll.rc:125: VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,   
VIRTKEY, CONTROL
On 2012/05/25 17:56:52, sky wrote:
> We shouldn't use the key see
> http://code.google.com/p/chromium/issues/detail?id=3696 . Either way though,
> this should be behind a flag.

But the CR is for Alt + VK_OEM_3. Is it same as Ctrl + VK_OEM_3. If so do I need
to use another key combination?
How to protect using a flag??

http://codereview.chromium.org/10117016/diff/12002/chrome/browser/tabs/tab_mr...
File chrome/browser/tabs/tab_mru_list_manager.h (right):

http://codereview.chromium.org/10117016/diff/12002/chrome/browser/tabs/tab_mr...
chrome/browser/tabs/tab_mru_list_manager.h:5: #ifndef
CHROME_BROWSER_TABS_TAB_MRU_LIST_MANAGER_H_
On 2012/05/25 17:56:52, sky wrote:
> This should be in chrome/browser/ui/tabs.

Done.

http://codereview.chromium.org/10117016/diff/12002/chrome/browser/tabs/tab_mr...
chrome/browser/tabs/tab_mru_list_manager.h:5: #ifndef
CHROME_BROWSER_TABS_TAB_MRU_LIST_MANAGER_H_
On 2012/05/25 17:56:52, sky wrote:
> This should be in chrome/browser/ui/tabs.

Done.

http://codereview.chromium.org/10117016/diff/12002/chrome/browser/tabs/tab_mr...
chrome/browser/tabs/tab_mru_list_manager.h:20: class TabMRUListManager {
On 2012/05/25 17:56:52, sky wrote:
> TabMRUListManager should be a TabStripModelObserver.

Done.

[sky, 2012-05-29 19:48:27]

You'll want to add tests to tab_mru_list_manager_unittest

http://codereview.chromium.org/10117016/diff/17002/chrome/app/chrome_command_...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/app/chrome_command_...
chrome/app/chrome_command_ids.h:61: #define IDC_SELECT_NEXT_MRU_TAB        
34040
Put between SELECT_LAST_TAB and DUPLICATE_TAB

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:351: ALLOW_THIS_IN_INITIALIZER_LIST(
No need for ALLOW_THIS_IN_INITIALIAZER_LIST here.

As indicated in last review, this should be off by default and turned on via
about:flags.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:380:
tab_strip_model_->AddObserver(tab_mru_list_manager_.get());
TabMRUListManager should take care of adding/removing itself from the model.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1425: TabContentsWrapper* contents =
tab_mru_list_manager_->GetNextMRUTab();
Don't change selectnexttab.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:4097:
command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
Only enable if enabled in about:flags.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.cc (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:25:
tabs_mru_list_.push_front(contents);
You only want to add to the front if foreground is true.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:31: // Remove the index if found
in MRU List
You shouldn't need to override this. Instead put logic in TabDetachedAt.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:58: tabs_mru_list_.erase(it);
You should always push_front, right?

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:74:
tabs_mru_list_.push_back(new_contents);
This should be NOTREACHED

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:89: return *it;
This will crash if empty.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.h (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:31: bool foreground);
OVERRIDE on all of these.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:46: // Public Methods
Move GetNextMRUTab above TabInsertedTab and add a description. Its in the public
section, so no need to say 'Public Methods.'

[NaveenBobbili (Motorola), 2012-06-05 05:34:16]

http://codereview.chromium.org/10117016/diff/17002/chrome/app/chrome_command_...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/app/chrome_command_...
chrome/app/chrome_command_ids.h:61: #define IDC_SELECT_NEXT_MRU_TAB        
34040
On 2012/05/29 19:48:27, sky wrote:
> Put between SELECT_LAST_TAB and DUPLICATE_TAB

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:351: ALLOW_THIS_IN_INITIALIZER_LIST(
On 2012/05/29 19:48:27, sky wrote:
> No need for ALLOW_THIS_IN_INITIALIAZER_LIST here.
> 
> As indicated in last review, this should be off by default and turned on via
> about:flags.

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:380:
tab_strip_model_->AddObserver(tab_mru_list_manager_.get());
On 2012/05/29 19:48:28, sky wrote:
> TabMRUListManager should take care of adding/removing itself from the model.

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1425: TabContentsWrapper* contents =
tab_mru_list_manager_->GetNextMRUTab();
On 2012/05/29 19:48:28, sky wrote:
> Don't change selectnexttab.

This creeped in by mistake.Done

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:4097:
command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
On 2012/05/29 19:48:28, sky wrote:
> Only enable if enabled in about:flags.

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.cc (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:25:
tabs_mru_list_.push_front(contents);
On 2012/05/29 19:48:28, sky wrote:
> You only want to add to the front if foreground is true.

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:31: // Remove the index if found
in MRU List
On 2012/05/29 19:48:28, sky wrote:
> You shouldn't need to override this. Instead put logic in TabDetachedAt.

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:58: tabs_mru_list_.erase(it);
On 2012/05/29 19:48:28, sky wrote:
> You should always push_front, right?

This case would arrive if the tab is manually activated by the user. Then MRU
list item corresponding to this has to be brought to the front.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:74:
tabs_mru_list_.push_back(new_contents);
On 2012/05/29 19:48:28, sky wrote:
> This should be NOTREACHED

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:74:
tabs_mru_list_.push_back(new_contents);
On 2012/05/29 19:48:28, sky wrote:
> This should be NOTREACHED

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:89: return *it;
On 2012/05/29 19:48:28, sky wrote:
> This will crash if empty.

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.h (right):

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:31: bool foreground);
On 2012/05/29 19:48:28, sky wrote:
> OVERRIDE on all of these.

Done.

http://codereview.chromium.org/10117016/diff/17002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:46: // Public Methods
On 2012/05/29 19:48:28, sky wrote:
> Move GetNextMRUTab above TabInsertedTab and add a description. Its in the
public
> section, so no need to say 'Public Methods.'

Done.

[NaveenBobbili (Motorola), 2012-06-05 12:32:01]

Please review patchset6. I have considered all your previous review comments.

[NaveenBobbili (Motorola), 2012-06-11 09:45:31]

Hi Scott,
Can you please review patchset 6 during your free cycles?

Thanks,
Naveen.B

[sky, 2012-06-11 18:21:51]

I think you have the interaction a bit wrong. In particular as long as the user
has the control key down activating a tab shouldn't effect the current stack.
It's only when they release it that you want to effectively commit the switch
and update the stack.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/about_flags...
chrome/browser/about_flags.cc:762: kOsAll,
It looks like you only wired this up on linux.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:380: tab_mru_list_manager_.reset(new
TabMRUListManager(tab_strip_model_.get()));
spacing is wrong here.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1468: void Browser::SelectNextMRUTab() {
Isn't this more of selecting the previous mru tab, not the next mru tab?

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.cc (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:17: if (tab_strip_model_)
When would the model ever be NULL?

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:54:
tabs_mru_list_.push_front(*it);
Why is the push conditional?

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:68:
tabs_mru_list_.insert(it,new_contents);
space after ','

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:79: // Public Methods
remove this comment.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:80: TabContentsWrapper*
TabMRUListManager::GetNextMRUTab() {
Order should match header.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.h (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:27:
TabMRUListManager(TabStripModel* tab_strip_model);
explicit

[NaveenBobbili (Motorola), 2012-06-13 06:07:32]

Hi,
I am not able to find a specific use case/ code to check if ctrl key is still
pressed. I tried looking at overriding AcceleratorTarget but that doesn't notify
about key releases. Please let me know if there is a way to check if ctrl is
pressed or not. I can only see windows API in base as of now. Not for linux.

Thanks,
Naveen.B

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/about_flags...
chrome/browser/about_flags.cc:762: kOsAll,
On 2012/06/11 18:21:51, sky wrote:
> It looks like you only wired this up on linux.

Done.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:380: tab_mru_list_manager_.reset(new
TabMRUListManager(tab_strip_model_.get()));
On 2012/06/11 18:21:51, sky wrote:
> spacing is wrong here.

Done.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1468: void Browser::SelectNextMRUTab() {
On 2012/06/11 18:21:51, sky wrote:
> Isn't this more of selecting the previous mru tab, not the next mru tab?

Done.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.cc (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:17: if (tab_strip_model_)
On 2012/06/11 18:21:51, sky wrote:
> When would the model ever be NULL?
Protection code. Unit tests would need this check.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:54:
tabs_mru_list_.push_front(*it);
On 2012/06/11 18:21:51, sky wrote:
> Why is the push conditional?

ActiveTabChanged should ideally be called after TabInsertTabAt. So I am double
checking if active tab is already in my list or not.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:68:
tabs_mru_list_.insert(it,new_contents);
On 2012/06/11 18:21:51, sky wrote:
> space after ','

Done.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:79: // Public Methods
On 2012/06/11 18:21:51, sky wrote:
> remove this comment.

Done.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:80: TabContentsWrapper*
TabMRUListManager::GetNextMRUTab() {
On 2012/06/11 18:21:51, sky wrote:
> Order should match header.

Done.

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.h (right):

http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:27:
TabMRUListManager(TabStripModel* tab_strip_model);
On 2012/06/11 18:21:51, sky wrote:
> explicit

Done.

[NaveenBobbili (Motorola), 2012-06-15 06:48:01]

Hi Scott,
Can you please review the change. Got the interaction right this time as per our
discussion.
I am pausing stack updates while ctrl key is pressed and commit the change once
I have ctrl key released.

Regards,
Naveen.B

[sky, 2012-06-15 19:35:21]

I'm not sure on the linux side. You'll have to ping some one who knows
more about linux or ask chormium-dev.

  -Scott

On Tue, Jun 12, 2012 at 11:07 PM,  <qghc36@motorola.com> wrote:
> Hi,
> I am not able to find a specific use case/ code to check if ctrl key is
> still
> pressed. I tried looking at overriding AcceleratorTarget but that doesn't
> notify
> about key releases. Please let me know if there is a way to check if ctrl is
> pressed or not. I can only see windows API in base as of now. Not for linux.
>
> Thanks,
> Naveen.B
>
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/about_flags.cc
> File chrome/browser/about_flags.cc (right):
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/about_flags...
> chrome/browser/about_flags.cc:762: kOsAll,
> On 2012/06/11 18:21:51, sky wrote:
>>
>> It looks like you only wired this up on linux.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser....
> chrome/browser/ui/browser.cc:380: tab_mru_list_manager_.reset(new
> TabMRUListManager(tab_strip_model_.get()));
> On 2012/06/11 18:21:51, sky wrote:
>>
>> spacing is wrong here.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/browser....
> chrome/browser/ui/browser.cc:1468: void Browser::SelectNextMRUTab() {
> On 2012/06/11 18:21:51, sky wrote:
>>
>> Isn't this more of selecting the previous mru tab, not the next mru
>
> tab?
>
> Done.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> File chrome/browser/ui/tabs/tab_mru_list_manager.cc (right):
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> chrome/browser/ui/tabs/tab_mru_list_manager.cc:17: if (tab_strip_model_)
> On 2012/06/11 18:21:51, sky wrote:
>>
>> When would the model ever be NULL?
>
> Protection code. Unit tests would need this check.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> chrome/browser/ui/tabs/tab_mru_list_manager.cc:54:
> tabs_mru_list_.push_front(*it);
> On 2012/06/11 18:21:51, sky wrote:
>>
>> Why is the push conditional?
>
>
> ActiveTabChanged should ideally be called after TabInsertTabAt. So I am
> double checking if active tab is already in my list or not.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> chrome/browser/ui/tabs/tab_mru_list_manager.cc:68:
> tabs_mru_list_.insert(it,new_contents);
> On 2012/06/11 18:21:51, sky wrote:
>>
>> space after ','
>
>
> Done.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> chrome/browser/ui/tabs/tab_mru_list_manager.cc:79: // Public Methods
> On 2012/06/11 18:21:51, sky wrote:
>>
>> remove this comment.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> chrome/browser/ui/tabs/tab_mru_list_manager.cc:80: TabContentsWrapper*
> TabMRUListManager::GetNextMRUTab() {
> On 2012/06/11 18:21:51, sky wrote:
>>
>> Order should match header.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> File chrome/browser/ui/tabs/tab_mru_list_manager.h (right):
>
>
http://codereview.chromium.org/10117016/diff/27002/chrome/browser/ui/tabs/tab...
> chrome/browser/ui/tabs/tab_mru_list_manager.h:27:
> TabMRUListManager(TabStripModel* tab_strip_model);
> On 2012/06/11 18:21:51, sky wrote:
>>
>> explicit
>
>
> Done.
>
> http://codereview.chromium.org/10117016/

[sky, 2012-06-15 19:47:39]

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:4094: if
(CommandLine::ForCurrentProcess()->HasSwitch(
This should be conditionally enabled.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:2229:
switches::kEnableTabMRUSwitch);
spacing is off. Rather than checking switch value, check for non-null
MRUTabController.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:2267:
switches::kEnableTabMRUSwitch);
spacing is off.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.cc (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:18: if (tab_strip_model_)
There is no reason to support a NULL model.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:89: std::advance(it, 1);
++it;

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.h (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:13: #include
"chrome/browser/ui/tabs/tab_strip_model.h"
You shouldn't need this include.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:17:
////////////////////////////////////////////////////////////////////////////////
newline between 16 and 17.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:25: class TabMRUListManager :
public TabStripModelObserver {
Name this MRUTabController.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:30: // Overridden from
TabStripModelObserver:
overriden methods should be grouped at the end of a section.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:45: TabContentsWrapper*
GetPreviousMRUTab();
You need to sync, TCW is now TabContents. Also, add comments to all this.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager_unittest.cc (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager_unittest.cc:5: #include
"chrome/browser/ui/tabs/tab_mru_list_manager.h"
newline between 5 and 6.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager_unittest.cc:21: TabContentsWrapper*
contents1 = (TabContentsWrapper*) 0xfffffff1;
Don't do this.

You can create TabContents in unit tests. Search for other examples to see how
its done.

[NaveenBobbili (Motorola), 2012-06-25 09:44:22]

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:4094: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/06/15 19:47:39, sky wrote:
> This should be conditionally enabled.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:2229:
switches::kEnableTabMRUSwitch);
On 2012/06/15 19:47:39, sky wrote:
> spacing is off. Rather than checking switch value, check for non-null
> MRUTabController.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:2267:
switches::kEnableTabMRUSwitch);
On 2012/06/15 19:47:39, sky wrote:
> spacing is off.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.cc (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:18: if (tab_strip_model_)
On 2012/06/15 19:47:39, sky wrote:
> There is no reason to support a NULL model.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.cc:89: std::advance(it, 1);
On 2012/06/15 19:47:39, sky wrote:
> ++it;

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_mru_list_manager.h (right):

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:13: #include
"chrome/browser/ui/tabs/tab_strip_model.h"
On 2012/06/15 19:47:39, sky wrote:
> You shouldn't need this include.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:17:
////////////////////////////////////////////////////////////////////////////////
On 2012/06/15 19:47:39, sky wrote:
> newline between 16 and 17.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:25: class TabMRUListManager :
public TabStripModelObserver {
On 2012/06/15 19:47:39, sky wrote:
> Name this MRUTabController.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:30: // Overridden from
TabStripModelObserver:
On 2012/06/15 19:47:39, sky wrote:
> overriden methods should be grouped at the end of a section.

Done.

http://codereview.chromium.org/10117016/diff/28031/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_mru_list_manager.h:45: TabContentsWrapper*
GetPreviousMRUTab();
On 2012/06/15 19:47:39, sky wrote:
> You need to sync, TCW is now TabContents. Also, add comments to all this.

Done.

[NaveenBobbili (Motorola), 2012-06-27 05:54:30]

Hi Scott,
I have addressed all the previous review comments.
Can you please take a look at the patch?

Thanks,
Naveen.B

[alexfh, 2012-07-02 15:16:08]

On 2012/04/23 22:21:58, glen wrote:
> We shouldn't do a preview (we should just switch the tab, as we do
> with Ctrl+Tab), but we're good to have Ctrl+~ be MRU.
I believe it's not a good idea to use Ctrl+~, as tilde is a keyboard
layout-dependent key, which means that users will not be able to use it when
they switch layout to German, Russian or any other layout which has local
characters bound to this key.
Why would it be bad to have an option to just switch the behavior of Ctrl+Tab?
We have Ctrl+PgUp/PgDn to switch to the previous/next tab. Why can't we make
Ctrl+Tab and Ctrl+Shift+Tab use MRU order (or at least have an option for this)?

[NaveenBobbili (Motorola), 2012-07-02 15:20:06]

On 2012/07/02 15:16:08, alexfh wrote:
> On 2012/04/23 22:21:58, glen wrote:
> > We shouldn't do a preview (we should just switch the tab, as we do
> > with Ctrl+Tab), but we're good to have Ctrl+~ be MRU.
> I believe it's not a good idea to use Ctrl+~, as tilde is a keyboard
> layout-dependent key, which means that users will not be able to use it when
> they switch layout to German, Russian or any other layout which has local
> characters bound to this key.
> Why would it be bad to have an option to just switch the behavior of Ctrl+Tab?
> We have Ctrl+PgUp/PgDn to switch to the previous/next tab. Why can't we make
> Ctrl+Tab and Ctrl+Shift+Tab use MRU order (or at least have an option for
this)?

Sounds Good. I will wait for Scott and Glen to post their opinions too before
making necessary changes.

[NaveenBobbili (Motorola), 2012-07-10 12:25:14]

On 2012/07/02 15:20:06, NaveenBobbili (Motorola) wrote:
> On 2012/07/02 15:16:08, alexfh wrote:
> > On 2012/04/23 22:21:58, glen wrote:
> > > We shouldn't do a preview (we should just switch the tab, as we do
> > > with Ctrl+Tab), but we're good to have Ctrl+~ be MRU.
> > I believe it's not a good idea to use Ctrl+~, as tilde is a keyboard
> > layout-dependent key, which means that users will not be able to use it when
> > they switch layout to German, Russian or any other layout which has local
> > characters bound to this key.
> > Why would it be bad to have an option to just switch the behavior of
Ctrl+Tab?
> > We have Ctrl+PgUp/PgDn to switch to the previous/next tab. Why can't we make
> > Ctrl+Tab and Ctrl+Shift+Tab use MRU order (or at least have an option for
> this)?
> 
> Sounds Good. I will wait for Scott and Glen to post their opinions too before
> making necessary changes.

Hi Scott,
Can you please post your comments during your free cycles?
Thanks,
Naveen.B

[sky, 2012-07-17 17:46:31]

browser has changed around quite substantially. Please rebase and I'll take
another look.

[NaveenBobbili (Motorola), 2012-07-19 07:24:11]

Hi Scott,
I have rebased and done necessary changes . Please take a look and let me know
your comments.

Thanks,
Naveen.B

[sky, 2012-07-19 16:13:00]

Before continuing with this work we need to resolve the keyboard
accelerator question. The ball is in Glens court.

  -Scott

On Thu, Jul 19, 2012 at 12:24 AM,  <qghc36@motorola.com> wrote:
> Hi Scott,
> I have rebased and done necessary changes . Please take a look and let me
> know
> your comments.
>
> Thanks,
> Naveen.B
>
> http://codereview.chromium.org/10117016/

[NaveenBobbili (Motorola), 2012-07-20 11:26:28]

Hi,
How about ctrl + backspace . Does it signify going back to the previous MRU
Tab ??

Thanks,
Naveen.B

On Thu, Jul 19, 2012 at 9:42 PM, Scott Violet <sky@chromium.org> wrote:

> Before continuing with this work we need to resolve the keyboard
> accelerator question. The ball is in Glens court.
>
>   -Scott
>
> On Thu, Jul 19, 2012 at 12:24 AM,  <qghc36@motorola.com> wrote:
> > Hi Scott,
> > I have rebased and done necessary changes . Please take a look and let me
> > know
> > your comments.
> >
> > Thanks,
> > Naveen.B
> >
> > http://codereview.chromium.org/10117016/
>

[sky, 2012-07-20 16:18:02]

You need both a back and forward key. ctrl+backspace is no good since
it does something in the omnbox and page (delete word).

  -Scott

On Fri, Jul 20, 2012 at 4:26 AM, Naveen Bobbili <qghc36@motorola.com> wrote:
> Hi,
> How about ctrl + backspace . Does it signify going back to the previous MRU
> Tab ??
>
> Thanks,
> Naveen.B
>
>
> On Thu, Jul 19, 2012 at 9:42 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Before continuing with this work we need to resolve the keyboard
>> accelerator question. The ball is in Glens court.
>>
>>   -Scott
>>
>> On Thu, Jul 19, 2012 at 12:24 AM,  <qghc36@motorola.com> wrote:
>> > Hi Scott,
>> > I have rebased and done necessary changes . Please take a look and let
>> > me
>> > know
>> > your comments.
>> >
>> > Thanks,
>> > Naveen.B
>> >
>> > http://codereview.chromium.org/10117016/
>
>

[NaveenBobbili (Motorola), 2012-07-21 06:53:25]

I think the current implementation could suffice one key very similar to
alt+tab in desktop. Whats the take on using something like a ctrl+alt+tab
or ctrl+shift+tab?


On Fri, Jul 20, 2012 at 9:47 PM, Scott Violet <sky@chromium.org> wrote:

> You need both a back and forward key. ctrl+backspace is no good since
> it does something in the omnbox and page (delete word).
>
>   -Scott
>
> On Fri, Jul 20, 2012 at 4:26 AM, Naveen Bobbili <qghc36@motorola.com>
> wrote:
> > Hi,
> > How about ctrl + backspace . Does it signify going back to the previous
> MRU
> > Tab ??
> >
> > Thanks,
> > Naveen.B
> >
> >
> > On Thu, Jul 19, 2012 at 9:42 PM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> Before continuing with this work we need to resolve the keyboard
> >> accelerator question. The ball is in Glens court.
> >>
> >>   -Scott
> >>
> >> On Thu, Jul 19, 2012 at 12:24 AM,  <qghc36@motorola.com> wrote:
> >> > Hi Scott,
> >> > I have rebased and done necessary changes . Please take a look and let
> >> > me
> >> > know
> >> > your comments.
> >> >
> >> > Thanks,
> >> > Naveen.B
> >> >
> >> > http://codereview.chromium.org/10117016/
> >
> >
>

[NaveenBobbili (Motorola), 2012-08-29 07:22:03]

Hi Glen,
I couldn't think anything other than ctrl + tilde . All the keys seem to be
taken away.
Do you have any inputs as I believe this feature is very useful for
keyboard users ?

Thanks,
Naveen.B

On Sat, Jul 21, 2012 at 12:23 PM, Naveen Bobbili <qghc36@motorola.com>wrote:

> I think the current implementation could suffice one key very similar to
> alt+tab in desktop. Whats the take on using something like a ctrl+alt+tab
> or ctrl+shift+tab?
>
>
>
> On Fri, Jul 20, 2012 at 9:47 PM, Scott Violet <sky@chromium.org> wrote:
>
>> You need both a back and forward key. ctrl+backspace is no good since
>> it does something in the omnbox and page (delete word).
>>
>>   -Scott
>>
>> On Fri, Jul 20, 2012 at 4:26 AM, Naveen Bobbili <qghc36@motorola.com>
>> wrote:
>> > Hi,
>> > How about ctrl + backspace . Does it signify going back to the previous
>> MRU
>> > Tab ??
>> >
>> > Thanks,
>> > Naveen.B
>> >
>> >
>> > On Thu, Jul 19, 2012 at 9:42 PM, Scott Violet <sky@chromium.org> wrote:
>> >>
>> >> Before continuing with this work we need to resolve the keyboard
>> >> accelerator question. The ball is in Glens court.
>> >>
>> >>   -Scott
>> >>
>> >> On Thu, Jul 19, 2012 at 12:24 AM,  <qghc36@motorola.com> wrote:
>> >> > Hi Scott,
>> >> > I have rebased and done necessary changes . Please take a look and
>> let
>> >> > me
>> >> > know
>> >> > your comments.
>> >> >
>> >> > Thanks,
>> >> > Naveen.B
>> >> >
>> >> > http://codereview.chromium.org/10117016/
>> >
>> >
>>
>
>

[epere4, 2012-08-29 07:37:41]

On 2012/07/02 15:20:06, NaveenBobbili (Motorola) wrote:
> On 2012/07/02 15:16:08, alexfh wrote:
> > On 2012/04/23 22:21:58, glen wrote:
> > > We shouldn't do a preview (we should just switch the tab, as we do
> > > with Ctrl+Tab), but we're good to have Ctrl+~ be MRU.
> > I believe it's not a good idea to use Ctrl+~, as tilde is a keyboard
> > layout-dependent key, which means that users will not be able to use it when
> > they switch layout to German, Russian or any other layout which has local
> > characters bound to this key.
> > Why would it be bad to have an option to just switch the behavior of
Ctrl+Tab?
> > We have Ctrl+PgUp/PgDn to switch to the previous/next tab. Why can't we make
> > Ctrl+Tab and Ctrl+Shift+Tab use MRU order (or at least have an option for
> this)?
> 
> Sounds Good. I will wait for Scott and Glen to post their opinions too before
> making necessary changes.

+1 for alexfh's idea of letting Ctrl+PgUp/PdDn be the only ones used for next
and previous tab and switch the behavior of the Ctrl+Tab and Ctrl+Shift+Tab
combination to be MRU.

[NaveenBobbili (Motorola), 2012-09-05 09:05:34]

On 2012/08/29 07:37:41, epere4 wrote:
> On 2012/07/02 15:20:06, NaveenBobbili (Motorola) wrote:
> > On 2012/07/02 15:16:08, alexfh wrote:
> > > On 2012/04/23 22:21:58, glen wrote:
> > > > We shouldn't do a preview (we should just switch the tab, as we do
> > > > with Ctrl+Tab), but we're good to have Ctrl+~ be MRU.
> > > I believe it's not a good idea to use Ctrl+~, as tilde is a keyboard
> > > layout-dependent key, which means that users will not be able to use it
when
> > > they switch layout to German, Russian or any other layout which has local
> > > characters bound to this key.
> > > Why would it be bad to have an option to just switch the behavior of
> Ctrl+Tab?
> > > We have Ctrl+PgUp/PgDn to switch to the previous/next tab. Why can't we
make
> > > Ctrl+Tab and Ctrl+Shift+Tab use MRU order (or at least have an option for
> > this)?
> > 
> > Sounds Good. I will wait for Scott and Glen to post their opinions too
before
> > making necessary changes.
> 
> +1 for alexfh's idea of letting Ctrl+PgUp/PdDn be the only ones used for next
> and previous tab and switch the behavior of the Ctrl+Tab and Ctrl+Shift+Tab
> combination to be MRU.


Since Glen is not keen on replacing the ctrl+PgUp/PgDn behaviour how about using
ctrl + leftarrow to switch to previous MRU tab?

[NaveenBobbili (Motorola), 2012-09-05 09:17:30]

Hi Glen/Scott,
How about using ctrl+left arrow to move to the previous MRU tab?

Thanks,
Naveen Bobbili

On Thu, Apr 19, 2012 at 12:57 PM, Naveen Bobbili <qghc36@motorola.com>wrote:

> Thanks for letting me know.
> This patch is mostly back end work .
> I would like to implement a feature which shows a preview of tabs using
> ctrl + tilde in order for the user to jump to any tab. Very similar to what
> alt + tab shows in desktop or ctrl + tab in opera browser.
> Please let me know if it falls inline with chromium browser UX plans.
>
>
> On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:
>
>> Thanks for the patch! Before you embark on ui changes like this you
>> need to run them by the ux folks: Glen, Brian and Ben, which are now
>> cc'd.
>>
>>  -Scott
>>
>> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
>> > Reviewers: sky,
>> >
>> > Message:
>> > Hi Scott,
>> > This feature is very similar to alt+tab feature in desktop. We are
>> > maintaining
>> > an Most Recently Used tabs list to switch to previously used tab. I have
>> > submitted initial implementation which works.
>> > Please provide comments about what else is necessary and any other
>> > optimizations. modularization which is required to finish up this
>> feature.
>> >
>> > Thanks,
>> > Naveen B
>> >
>> > Description:
>> > First cut implementation of switching between recently used tabs using
>> ctrl
>> > tilde or quoteleft.
>> >
>> >
>> > BUG=43459
>> > TEST=Done UI Testing only.
>> >
>> >
>> > Please review this at https://chromiumcodereview.appspot.com/10117016/
>> >
>> > SVN Base: http://git.chromium.org/chromium/src.git@master
>> >
>> > Affected files:
>> >  M chrome/app/chrome_command_ids.h
>> >  M chrome/app/chrome_dll.rc
>> >  M chrome/browser/ui/browser.h
>> >  M chrome/browser/ui/browser.cc
>> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
>> >
>> >
>> > Index: chrome/app/chrome_command_ids.h
>> > diff --git a/chrome/app/chrome_command_ids.h
>> > b/chrome/app/chrome_command_ids.h
>> > index
>> >
>>
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
>> > 100644
>> > --- a/chrome/app/chrome_command_ids.h
>> > +++ b/chrome/app/chrome_command_ids.h
>> > @@ -315,6 +315,9 @@
>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
>> >
>> > +// Select the last visited tab.
>> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
>> > +
>> >  // NOTE: The last valid command value is 57343 (0xDFFF)
>> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
>> >
>> > Index: chrome/app/chrome_dll.rc
>> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
>> > index
>> >
>>
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
>> > 100644
>> > --- a/chrome/app/chrome_dll.rc
>> > +++ b/chrome/app/chrome_dll.rc
>> > @@ -122,6 +122,7 @@ BEGIN
>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL, SHIFT
>> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
>> >  END
>> >
>> >  IDR_CHROMEFRAME ACCELERATORS
>> > Index: chrome/browser/ui/browser.cc
>> > diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
>> > index
>> >
>>
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
>> > 100644
>> > --- a/chrome/browser/ui/browser.cc
>> > +++ b/chrome/browser/ui/browser.cc
>> > @@ -531,6 +531,9 @@ Browser::~Browser() {
>> >     select_file_dialog_->ListenerDestroyed();
>> >
>> >   TabRestoreServiceDestroyed(tab_restore_service_);
>> > +
>> > +  // Clear the tabs mru list
>> > +  tabs_mru_list_.clear();
>> >  }
>> >
>> >  bool Browser::IsFullscreenForTabOrPending() const {
>> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
>> >   tab_handler_->GetTabStripModel()->SelectLastTab();
>> >  }
>> >
>> > +void Browser::SelectNextMRUTab() {
>> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
>> > +  std::list<TabContentsWrapper*>::iterator it = tabs_mru_list_.begin();
>> > +  // Advance to second element in MRU list if there are more than one
>> tabs
>> > open.
>> > +  if (tabs_mru_list_.size() > 1) {
>> > +    std::advance(it, 1);
>> > +  }
>> > +  if (*it) {
>> > +
>> >
>>  ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
>> > +                true);
>> > +  }
>> > +}
>> > +
>> >  void Browser::DuplicateTab() {
>> >   content::RecordAction(UserMetricsAction("Duplicate"));
>> >   DuplicateContentsAt(active_index());
>> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
>> >                                     break;
>> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
>>  break;
>> >
>> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
>> > break;
>> > +
>> >     default:
>> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
>> >       break;
>> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
>> >
>> >
>> 
///////////////////////////////////////////////////////////////////////////////
>> >  // Browser, TabStripModelObserver implementation:
>> > -
>> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
>> >                             int index,
>> >                             bool foreground) {
>> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
>> > contents,
>> >
>> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
>> >
>>  content::Source<WebContents>(contents->web_contents()));
>> > +
>> > +  // Insert the tab at the beginning of the MRU list
>> > +  tabs_mru_list_.push_front(contents);
>> >  }
>> >
>> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
>> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
>> > tab_strip_model,
>> >
>> >   // Sever the TabContents' connection back to us.
>> >   SetAsDelegate(contents, NULL);
>> > +
>> > +  // Remove the index if found in MRU List
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +     tabs_mru_list_.erase(it);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index) {
>> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
>> > +
>> > +  // Remove the index if found in MRU List
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +    tabs_mru_list_.erase(it);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
>> > @@ -3603,6 +3641,17 @@ void
>> Browser::ActiveTabChanged(TabContentsWrapper*
>> > old_contents,
>> >   }
>> >
>> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
>> > +
>> > +
>> > +  // Bring the activated tab content to the front of the list
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                new_contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +    tabs_mru_list_.push_front(*it);
>> > +    tabs_mru_list_.erase(it);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabMoved(TabContentsWrapper* contents,
>> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
>> > tab_strip_model,
>> >
>> >   content::DevToolsManager::GetInstance()->TabReplaced(
>> >       old_contents->web_contents(), new_contents->web_contents());
>> > +
>> > +  // Replace the tab contents accordingly in the list. If the tab
>> contents
>> > are
>> > +  // not found add to the end of the list.
>> > +  std::list<TabContentsWrapper*>::iterator it =
>> > +      std::find(tabs_mru_list_.begin(),
>> > +                tabs_mru_list_.end(),
>> > +                old_contents);
>> > +  if (it != tabs_mru_list_.end()) {
>> > +    tabs_mru_list_.insert(it,new_contents);
>> > +    tabs_mru_list_.erase(it);
>> > +  } else {
>> > +    tabs_mru_list_.push_back(new_contents);
>> > +  }
>> >  }
>> >
>> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents, int
>> > index) {
>> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
>> >   //       update BrowserList::CloseAllBrowsers.
>> >   MessageLoop::current()->PostTask(
>> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
>> > weak_factory_.GetWeakPtr()));
>> > +
>> > +  // Clear the tabs MRU List
>> > +  tabs_mru_list_.clear();
>> >  }
>> >
>> >
>> 
///////////////////////////////////////////////////////////////////////////////
>> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
>> normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
>> >                                         normal_window);
>> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
>> > +                                        normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
>> normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
>> > normal_window);
>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0,
>> normal_window);
>> > Index: chrome/browser/ui/browser.h
>> > diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
>> > index
>> >
>>
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
>> > 100644
>> > --- a/chrome/browser/ui/browser.h
>> > +++ b/chrome/browser/ui/browser.h
>> > @@ -10,6 +10,7 @@
>> >  #include <set>
>> >  #include <string>
>> >  #include <vector>
>> > +#include <list>
>> >
>> >  #include "base/basictypes.h"
>> >  #include "base/compiler_specific.h"
>> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
>> >                 public ProfileSyncServiceObserver,
>> >                 public InstantDelegate {
>> >  public:
>> > +  // List that maintains the tab indices in the most recently visited
>> order
>> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
>> > +
>> >   // SessionService::WindowType mirrors these values.  If you add to
>> this
>> >   // enum, look at SessionService::WindowType to see if it needs to be
>> >   // updated.
>> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
>> >   void MoveTabPrevious();
>> >   void SelectNumberedTab(int index);
>> >   void SelectLastTab();
>> > +  void SelectNextMRUTab();
>> >   void DuplicateTab();
>> >   void WriteCurrentURLToClipboard();
>> >   void ConvertPopupToTabbedBrowser();
>> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
>> >   // before DidEndColorChooser is called.
>> >   scoped_ptr<content::ColorChooser> color_chooser_;
>> >
>> > +  TabsMRUList tabs_mru_list_;
>> > +
>> >   DISALLOW_COPY_AND_ASSIGN(Browser);
>> >  };
>> >
>> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
>> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > index
>> >
>>
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
>> > 100644
>> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
>> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
>> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
>> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
>> >   { GDK_t, IDC_RESTORE_TAB,
>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>> >
>> >
>>
>
>

[glen, 2012-09-06 00:58:56]

Ctrl+Arrows are heavily used in text editing, so it wouldn't work whenever
the omnibox was focused.


On Wed, Sep 5, 2012 at 2:07 AM, Naveen Bobbili <qghc36@motorola.com> wrote:

> Hi Glen/Scott,
> How about using ctrl+left arrow to move to the previous MRU tab?
>
> Thanks,
> Naveen Bobbili
>
>
> On Thu, Apr 19, 2012 at 12:57 PM, Naveen Bobbili <qghc36@motorola.com>wrote:
>
>> Thanks for letting me know.
>> This patch is mostly back end work .
>> I would like to implement a feature which shows a preview of tabs using
>> ctrl + tilde in order for the user to jump to any tab. Very similar to what
>> alt + tab shows in desktop or ctrl + tab in opera browser.
>> Please let me know if it falls inline with chromium browser UX plans.
>>
>>
>> On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:
>>
>>> Thanks for the patch! Before you embark on ui changes like this you
>>> need to run them by the ux folks: Glen, Brian and Ben, which are now
>>> cc'd.
>>>
>>>  -Scott
>>>
>>> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
>>> > Reviewers: sky,
>>> >
>>> > Message:
>>> > Hi Scott,
>>> > This feature is very similar to alt+tab feature in desktop. We are
>>> > maintaining
>>> > an Most Recently Used tabs list to switch to previously used tab. I
>>> have
>>> > submitted initial implementation which works.
>>> > Please provide comments about what else is necessary and any other
>>> > optimizations. modularization which is required to finish up this
>>> feature.
>>> >
>>> > Thanks,
>>> > Naveen B
>>> >
>>> > Description:
>>> > First cut implementation of switching between recently used tabs using
>>> ctrl
>>> > tilde or quoteleft.
>>> >
>>> >
>>> > BUG=43459
>>> > TEST=Done UI Testing only.
>>> >
>>> >
>>> > Please review this at https://chromiumcodereview.appspot.com/10117016/
>>> >
>>> > SVN Base: http://git.chromium.org/chromium/src.git@master
>>> >
>>> > Affected files:
>>> >  M chrome/app/chrome_command_ids.h
>>> >  M chrome/app/chrome_dll.rc
>>> >  M chrome/browser/ui/browser.h
>>> >  M chrome/browser/ui/browser.cc
>>> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
>>> >
>>> >
>>> > Index: chrome/app/chrome_command_ids.h
>>> > diff --git a/chrome/app/chrome_command_ids.h
>>> > b/chrome/app/chrome_command_ids.h
>>> > index
>>> >
>>>
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
>>> > 100644
>>> > --- a/chrome/app/chrome_command_ids.h
>>> > +++ b/chrome/app/chrome_command_ids.h
>>> > @@ -315,6 +315,9 @@
>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
>>> >
>>> > +// Select the last visited tab.
>>> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
>>> > +
>>> >  // NOTE: The last valid command value is 57343 (0xDFFF)
>>> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
>>> >
>>> > Index: chrome/app/chrome_dll.rc
>>> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
>>> > index
>>> >
>>>
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
>>> > 100644
>>> > --- a/chrome/app/chrome_dll.rc
>>> > +++ b/chrome/app/chrome_dll.rc
>>> > @@ -122,6 +122,7 @@ BEGIN
>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL, SHIFT
>>> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
>>> >  END
>>> >
>>> >  IDR_CHROMEFRAME ACCELERATORS
>>> > Index: chrome/browser/ui/browser.cc
>>> > diff --git a/chrome/browser/ui/browser.cc
>>> b/chrome/browser/ui/browser.cc
>>> > index
>>> >
>>>
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
>>> > 100644
>>> > --- a/chrome/browser/ui/browser.cc
>>> > +++ b/chrome/browser/ui/browser.cc
>>> > @@ -531,6 +531,9 @@ Browser::~Browser() {
>>> >     select_file_dialog_->ListenerDestroyed();
>>> >
>>> >   TabRestoreServiceDestroyed(tab_restore_service_);
>>> > +
>>> > +  // Clear the tabs mru list
>>> > +  tabs_mru_list_.clear();
>>> >  }
>>> >
>>> >  bool Browser::IsFullscreenForTabOrPending() const {
>>> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
>>> >   tab_handler_->GetTabStripModel()->SelectLastTab();
>>> >  }
>>> >
>>> > +void Browser::SelectNextMRUTab() {
>>> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> tabs_mru_list_.begin();
>>> > +  // Advance to second element in MRU list if there are more than one
>>> tabs
>>> > open.
>>> > +  if (tabs_mru_list_.size() > 1) {
>>> > +    std::advance(it, 1);
>>> > +  }
>>> > +  if (*it) {
>>> > +
>>> >
>>> 
ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
>>> > +                true);
>>> > +  }
>>> > +}
>>> > +
>>> >  void Browser::DuplicateTab() {
>>> >   content::RecordAction(UserMetricsAction("Duplicate"));
>>> >   DuplicateContentsAt(active_index());
>>> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
>>> >                                     break;
>>> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
>>>  break;
>>> >
>>> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
>>> > break;
>>> > +
>>> >     default:
>>> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
>>> >       break;
>>> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
>>> >
>>> >
>>> 
///////////////////////////////////////////////////////////////////////////////
>>> >  // Browser, TabStripModelObserver implementation:
>>> > -
>>> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
>>> >                             int index,
>>> >                             bool foreground) {
>>> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
>>> > contents,
>>> >
>>> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
>>> >
>>>  content::Source<WebContents>(contents->web_contents()));
>>> > +
>>> > +  // Insert the tab at the beginning of the MRU list
>>> > +  tabs_mru_list_.push_front(contents);
>>> >  }
>>> >
>>> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
>>> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
>>> > tab_strip_model,
>>> >
>>> >   // Sever the TabContents' connection back to us.
>>> >   SetAsDelegate(contents, NULL);
>>> > +
>>> > +  // Remove the index if found in MRU List
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +     tabs_mru_list_.erase(it);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index) {
>>> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
>>> > +
>>> > +  // Remove the index if found in MRU List
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +    tabs_mru_list_.erase(it);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
>>> > @@ -3603,6 +3641,17 @@ void
>>> Browser::ActiveTabChanged(TabContentsWrapper*
>>> > old_contents,
>>> >   }
>>> >
>>> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
>>> > +
>>> > +
>>> > +  // Bring the activated tab content to the front of the list
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                new_contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +    tabs_mru_list_.push_front(*it);
>>> > +    tabs_mru_list_.erase(it);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabMoved(TabContentsWrapper* contents,
>>> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
>>> > tab_strip_model,
>>> >
>>> >   content::DevToolsManager::GetInstance()->TabReplaced(
>>> >       old_contents->web_contents(), new_contents->web_contents());
>>> > +
>>> > +  // Replace the tab contents accordingly in the list. If the tab
>>> contents
>>> > are
>>> > +  // not found add to the end of the list.
>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>> > +      std::find(tabs_mru_list_.begin(),
>>> > +                tabs_mru_list_.end(),
>>> > +                old_contents);
>>> > +  if (it != tabs_mru_list_.end()) {
>>> > +    tabs_mru_list_.insert(it,new_contents);
>>> > +    tabs_mru_list_.erase(it);
>>> > +  } else {
>>> > +    tabs_mru_list_.push_back(new_contents);
>>> > +  }
>>> >  }
>>> >
>>> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents, int
>>> > index) {
>>> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
>>> >   //       update BrowserList::CloseAllBrowsers.
>>> >   MessageLoop::current()->PostTask(
>>> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
>>> > weak_factory_.GetWeakPtr()));
>>> > +
>>> > +  // Clear the tabs MRU List
>>> > +  tabs_mru_list_.clear();
>>> >  }
>>> >
>>> >
>>> 
///////////////////////////////////////////////////////////////////////////////
>>> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
>>> normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
>>> >                                         normal_window);
>>> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
>>> > +                                        normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
>>> normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
>>> > normal_window);
>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0,
>>> normal_window);
>>> > Index: chrome/browser/ui/browser.h
>>> > diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
>>> > index
>>> >
>>>
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
>>> > 100644
>>> > --- a/chrome/browser/ui/browser.h
>>> > +++ b/chrome/browser/ui/browser.h
>>> > @@ -10,6 +10,7 @@
>>> >  #include <set>
>>> >  #include <string>
>>> >  #include <vector>
>>> > +#include <list>
>>> >
>>> >  #include "base/basictypes.h"
>>> >  #include "base/compiler_specific.h"
>>> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
>>> >                 public ProfileSyncServiceObserver,
>>> >                 public InstantDelegate {
>>> >  public:
>>> > +  // List that maintains the tab indices in the most recently visited
>>> order
>>> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
>>> > +
>>> >   // SessionService::WindowType mirrors these values.  If you add to
>>> this
>>> >   // enum, look at SessionService::WindowType to see if it needs to be
>>> >   // updated.
>>> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
>>> >   void MoveTabPrevious();
>>> >   void SelectNumberedTab(int index);
>>> >   void SelectLastTab();
>>> > +  void SelectNextMRUTab();
>>> >   void DuplicateTab();
>>> >   void WriteCurrentURLToClipboard();
>>> >   void ConvertPopupToTabbedBrowser();
>>> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
>>> >   // before DidEndColorChooser is called.
>>> >   scoped_ptr<content::ColorChooser> color_chooser_;
>>> >
>>> > +  TabsMRUList tabs_mru_list_;
>>> > +
>>> >   DISALLOW_COPY_AND_ASSIGN(Browser);
>>> >  };
>>> >
>>> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > index
>>> >
>>>
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
>>> > 100644
>>> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
>>> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
>>> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
>>> >   { GDK_t, IDC_RESTORE_TAB,
>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>> >
>>> >
>>>
>>
>>
>

[alexfh, 2012-09-10 15:33:58]

Naveen, can you try your patch with keyboard layouts that change tilde key
behavior? Maybe there's no problem with keyboard layout sensitivity?

http://codereview.chromium.org/10117016/diff/64003/chrome/browser/ui/gtk/acce...
File chrome/browser/ui/gtk/accelerators_gtk.cc (right):

http://codereview.chromium.org/10117016/diff/64003/chrome/browser/ui/gtk/acce...
chrome/browser/ui/gtk/accelerators_gtk.cc:38: { GDK_quoteleft,
IDC_SELECT_PREVIOUS_MRU_TAB, GDK_CONTROL_MASK },
BTW, is this way of handling hot-keys really keyboard layout-sensitive? Maybe
you try to install a couple of European keyboard layouts (e.g. Russian and
French) and try if it works with them?

And if it doesn't, maybe there's a way to handle the 'tilde' key regardless of
the layout?

[NaveenBobbili (Motorola), 2013-02-25 18:49:55]

Hi Scott/Glen,
Can we have ctrl+tab as an experimental override over its basic
implementation?
I see a lot of traction on the bug to use ctrl+tab and make it experimental
feature.

Thanks,
Naveen Bobbili

On Wed, Sep 5, 2012 at 5:58 PM, Glen Murphy <glen@google.com> wrote:

> Ctrl+Arrows are heavily used in text editing, so it wouldn't work whenever
> the omnibox was focused.
>
>
> On Wed, Sep 5, 2012 at 2:07 AM, Naveen Bobbili <qghc36@motorola.com>wrote:
>
>> Hi Glen/Scott,
>> How about using ctrl+left arrow to move to the previous MRU tab?
>>
>> Thanks,
>> Naveen Bobbili
>>
>>
>> On Thu, Apr 19, 2012 at 12:57 PM, Naveen Bobbili <qghc36@motorola.com>wrote:
>>
>>> Thanks for letting me know.
>>> This patch is mostly back end work .
>>> I would like to implement a feature which shows a preview of tabs using
>>> ctrl + tilde in order for the user to jump to any tab. Very similar to what
>>> alt + tab shows in desktop or ctrl + tab in opera browser.
>>> Please let me know if it falls inline with chromium browser UX plans.
>>>
>>>
>>> On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:
>>>
>>>> Thanks for the patch! Before you embark on ui changes like this you
>>>> need to run them by the ux folks: Glen, Brian and Ben, which are now
>>>> cc'd.
>>>>
>>>>  -Scott
>>>>
>>>> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
>>>> > Reviewers: sky,
>>>> >
>>>> > Message:
>>>> > Hi Scott,
>>>> > This feature is very similar to alt+tab feature in desktop. We are
>>>> > maintaining
>>>> > an Most Recently Used tabs list to switch to previously used tab. I
>>>> have
>>>> > submitted initial implementation which works.
>>>> > Please provide comments about what else is necessary and any other
>>>> > optimizations. modularization which is required to finish up this
>>>> feature.
>>>> >
>>>> > Thanks,
>>>> > Naveen B
>>>> >
>>>> > Description:
>>>> > First cut implementation of switching between recently used tabs
>>>> using ctrl
>>>> > tilde or quoteleft.
>>>> >
>>>> >
>>>> > BUG=43459
>>>> > TEST=Done UI Testing only.
>>>> >
>>>> >
>>>> > Please review this at
>>>> https://chromiumcodereview.appspot.com/10117016/
>>>> >
>>>> > SVN Base: http://git.chromium.org/chromium/src.git@master
>>>> >
>>>> > Affected files:
>>>> >  M chrome/app/chrome_command_ids.h
>>>> >  M chrome/app/chrome_dll.rc
>>>> >  M chrome/browser/ui/browser.h
>>>> >  M chrome/browser/ui/browser.cc
>>>> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
>>>> >
>>>> >
>>>> > Index: chrome/app/chrome_command_ids.h
>>>> > diff --git a/chrome/app/chrome_command_ids.h
>>>> > b/chrome/app/chrome_command_ids.h
>>>> > index
>>>> >
>>>>
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
>>>> > 100644
>>>> > --- a/chrome/app/chrome_command_ids.h
>>>> > +++ b/chrome/app/chrome_command_ids.h
>>>> > @@ -315,6 +315,9 @@
>>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
>>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
>>>> >
>>>> > +// Select the last visited tab.
>>>> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
>>>> > +
>>>> >  // NOTE: The last valid command value is 57343 (0xDFFF)
>>>> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
>>>> >
>>>> > Index: chrome/app/chrome_dll.rc
>>>> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
>>>> > index
>>>> >
>>>>
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
>>>> > 100644
>>>> > --- a/chrome/app/chrome_dll.rc
>>>> > +++ b/chrome/app/chrome_dll.rc
>>>> > @@ -122,6 +122,7 @@ BEGIN
>>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL,
>>>> SHIFT
>>>> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>>> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
>>>> >  END
>>>> >
>>>> >  IDR_CHROMEFRAME ACCELERATORS
>>>> > Index: chrome/browser/ui/browser.cc
>>>> > diff --git a/chrome/browser/ui/browser.cc
>>>> b/chrome/browser/ui/browser.cc
>>>> > index
>>>> >
>>>>
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
>>>> > 100644
>>>> > --- a/chrome/browser/ui/browser.cc
>>>> > +++ b/chrome/browser/ui/browser.cc
>>>> > @@ -531,6 +531,9 @@ Browser::~Browser() {
>>>> >     select_file_dialog_->ListenerDestroyed();
>>>> >
>>>> >   TabRestoreServiceDestroyed(tab_restore_service_);
>>>> > +
>>>> > +  // Clear the tabs mru list
>>>> > +  tabs_mru_list_.clear();
>>>> >  }
>>>> >
>>>> >  bool Browser::IsFullscreenForTabOrPending() const {
>>>> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
>>>> >   tab_handler_->GetTabStripModel()->SelectLastTab();
>>>> >  }
>>>> >
>>>> > +void Browser::SelectNextMRUTab() {
>>>> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>> tabs_mru_list_.begin();
>>>> > +  // Advance to second element in MRU list if there are more than
>>>> one tabs
>>>> > open.
>>>> > +  if (tabs_mru_list_.size() > 1) {
>>>> > +    std::advance(it, 1);
>>>> > +  }
>>>> > +  if (*it) {
>>>> > +
>>>> >
>>>> 
ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
>>>> > +                true);
>>>> > +  }
>>>> > +}
>>>> > +
>>>> >  void Browser::DuplicateTab() {
>>>> >   content::RecordAction(UserMetricsAction("Duplicate"));
>>>> >   DuplicateContentsAt(active_index());
>>>> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
>>>> >                                     break;
>>>> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
>>>>  break;
>>>> >
>>>> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
>>>> > break;
>>>> > +
>>>> >     default:
>>>> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
>>>> >       break;
>>>> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
>>>> >
>>>> >
>>>> 
///////////////////////////////////////////////////////////////////////////////
>>>> >  // Browser, TabStripModelObserver implementation:
>>>> > -
>>>> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
>>>> >                             int index,
>>>> >                             bool foreground) {
>>>> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
>>>> > contents,
>>>> >
>>>> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
>>>> >
>>>>  content::Source<WebContents>(contents->web_contents()));
>>>> > +
>>>> > +  // Insert the tab at the beginning of the MRU list
>>>> > +  tabs_mru_list_.push_front(contents);
>>>> >  }
>>>> >
>>>> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
>>>> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
>>>> > tab_strip_model,
>>>> >
>>>> >   // Sever the TabContents' connection back to us.
>>>> >   SetAsDelegate(contents, NULL);
>>>> > +
>>>> > +  // Remove the index if found in MRU List
>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>> > +      std::find(tabs_mru_list_.begin(),
>>>> > +                tabs_mru_list_.end(),
>>>> > +                contents);
>>>> > +  if (it != tabs_mru_list_.end()) {
>>>> > +     tabs_mru_list_.erase(it);
>>>> > +  }
>>>> >  }
>>>> >
>>>> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index)
>>>> {
>>>> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
>>>> > +
>>>> > +  // Remove the index if found in MRU List
>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>> > +      std::find(tabs_mru_list_.begin(),
>>>> > +                tabs_mru_list_.end(),
>>>> > +                contents);
>>>> > +  if (it != tabs_mru_list_.end()) {
>>>> > +    tabs_mru_list_.erase(it);
>>>> > +  }
>>>> >  }
>>>> >
>>>> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
>>>> > @@ -3603,6 +3641,17 @@ void
>>>> Browser::ActiveTabChanged(TabContentsWrapper*
>>>> > old_contents,
>>>> >   }
>>>> >
>>>> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
>>>> > +
>>>> > +
>>>> > +  // Bring the activated tab content to the front of the list
>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>> > +      std::find(tabs_mru_list_.begin(),
>>>> > +                tabs_mru_list_.end(),
>>>> > +                new_contents);
>>>> > +  if (it != tabs_mru_list_.end()) {
>>>> > +    tabs_mru_list_.push_front(*it);
>>>> > +    tabs_mru_list_.erase(it);
>>>> > +  }
>>>> >  }
>>>> >
>>>> >  void Browser::TabMoved(TabContentsWrapper* contents,
>>>> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
>>>> > tab_strip_model,
>>>> >
>>>> >   content::DevToolsManager::GetInstance()->TabReplaced(
>>>> >       old_contents->web_contents(), new_contents->web_contents());
>>>> > +
>>>> > +  // Replace the tab contents accordingly in the list. If the tab
>>>> contents
>>>> > are
>>>> > +  // not found add to the end of the list.
>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>> > +      std::find(tabs_mru_list_.begin(),
>>>> > +                tabs_mru_list_.end(),
>>>> > +                old_contents);
>>>> > +  if (it != tabs_mru_list_.end()) {
>>>> > +    tabs_mru_list_.insert(it,new_contents);
>>>> > +    tabs_mru_list_.erase(it);
>>>> > +  } else {
>>>> > +    tabs_mru_list_.push_back(new_contents);
>>>> > +  }
>>>> >  }
>>>> >
>>>> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents, int
>>>> > index) {
>>>> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
>>>> >   //       update BrowserList::CloseAllBrowsers.
>>>> >   MessageLoop::current()->PostTask(
>>>> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
>>>> > weak_factory_.GetWeakPtr()));
>>>> > +
>>>> > +  // Clear the tabs MRU List
>>>> > +  tabs_mru_list_.clear();
>>>> >  }
>>>> >
>>>> >
>>>> 
///////////////////////////////////////////////////////////////////////////////
>>>> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
>>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
>>>> normal_window);
>>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
>>>> >                                         normal_window);
>>>> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
>>>> > +                                        normal_window);
>>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
>>>> normal_window);
>>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
>>>> > normal_window);
>>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0,
>>>> normal_window);
>>>> > Index: chrome/browser/ui/browser.h
>>>> > diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
>>>> > index
>>>> >
>>>>
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
>>>> > 100644
>>>> > --- a/chrome/browser/ui/browser.h
>>>> > +++ b/chrome/browser/ui/browser.h
>>>> > @@ -10,6 +10,7 @@
>>>> >  #include <set>
>>>> >  #include <string>
>>>> >  #include <vector>
>>>> > +#include <list>
>>>> >
>>>> >  #include "base/basictypes.h"
>>>> >  #include "base/compiler_specific.h"
>>>> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
>>>> >                 public ProfileSyncServiceObserver,
>>>> >                 public InstantDelegate {
>>>> >  public:
>>>> > +  // List that maintains the tab indices in the most recently
>>>> visited order
>>>> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
>>>> > +
>>>> >   // SessionService::WindowType mirrors these values.  If you add to
>>>> this
>>>> >   // enum, look at SessionService::WindowType to see if it needs to be
>>>> >   // updated.
>>>> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
>>>> >   void MoveTabPrevious();
>>>> >   void SelectNumberedTab(int index);
>>>> >   void SelectLastTab();
>>>> > +  void SelectNextMRUTab();
>>>> >   void DuplicateTab();
>>>> >   void WriteCurrentURLToClipboard();
>>>> >   void ConvertPopupToTabbedBrowser();
>>>> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
>>>> >   // before DidEndColorChooser is called.
>>>> >   scoped_ptr<content::ColorChooser> color_chooser_;
>>>> >
>>>> > +  TabsMRUList tabs_mru_list_;
>>>> > +
>>>> >   DISALLOW_COPY_AND_ASSIGN(Browser);
>>>> >  };
>>>> >
>>>> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
>>>> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>> > index
>>>> >
>>>>
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
>>>> > 100644
>>>> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
>>>> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
>>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>>> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
>>>> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>>> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>>> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
>>>> >   { GDK_t, IDC_RESTORE_TAB,
>>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>>> >
>>>> >
>>>>
>>>
>>>
>>
>

[sky, 2013-02-25 21:42:51]

See Glen's earlier comment:

"I could've sworn I'd responded to this already - we're not interested
in replacing the Ctrl+Tab behavior or adding a pref; if we can't find
an alternate shortcut for this, we may have to cancel."

  -Scott

On Mon, Feb 25, 2013 at 10:49 AM, Naveen Bobbili <qghc36@motorola.com> wrote:
> Hi Scott/Glen,
> Can we have ctrl+tab as an experimental override over its basic
> implementation?
> I see a lot of traction on the bug to use ctrl+tab and make it experimental
> feature.
>
> Thanks,
> Naveen Bobbili
>
>
> On Wed, Sep 5, 2012 at 5:58 PM, Glen Murphy <glen@google.com> wrote:
>>
>> Ctrl+Arrows are heavily used in text editing, so it wouldn't work whenever
>> the omnibox was focused.
>>
>>
>> On Wed, Sep 5, 2012 at 2:07 AM, Naveen Bobbili <qghc36@motorola.com>
>> wrote:
>>>
>>> Hi Glen/Scott,
>>> How about using ctrl+left arrow to move to the previous MRU tab?
>>>
>>> Thanks,
>>> Naveen Bobbili
>>>
>>>
>>> On Thu, Apr 19, 2012 at 12:57 PM, Naveen Bobbili <qghc36@motorola.com>
>>> wrote:
>>>>
>>>> Thanks for letting me know.
>>>> This patch is mostly back end work .
>>>> I would like to implement a feature which shows a preview of tabs using
>>>> ctrl + tilde in order for the user to jump to any tab. Very similar to
what
>>>> alt + tab shows in desktop or ctrl + tab in opera browser.
>>>> Please let me know if it falls inline with chromium browser UX plans.
>>>>
>>>>
>>>> On Wed, Apr 18, 2012 at 8:05 PM, Scott Violet <sky@chromium.org> wrote:
>>>>>
>>>>> Thanks for the patch! Before you embark on ui changes like this you
>>>>> need to run them by the ux folks: Glen, Brian and Ben, which are now
>>>>> cc'd.
>>>>>
>>>>>  -Scott
>>>>>
>>>>> On Wed, Apr 18, 2012 at 3:38 AM,  <qghc36@motorola.com> wrote:
>>>>> > Reviewers: sky,
>>>>> >
>>>>> > Message:
>>>>> > Hi Scott,
>>>>> > This feature is very similar to alt+tab feature in desktop. We are
>>>>> > maintaining
>>>>> > an Most Recently Used tabs list to switch to previously used tab. I
>>>>> > have
>>>>> > submitted initial implementation which works.
>>>>> > Please provide comments about what else is necessary and any other
>>>>> > optimizations. modularization which is required to finish up this
>>>>> > feature.
>>>>> >
>>>>> > Thanks,
>>>>> > Naveen B
>>>>> >
>>>>> > Description:
>>>>> > First cut implementation of switching between recently used tabs
>>>>> > using ctrl
>>>>> > tilde or quoteleft.
>>>>> >
>>>>> >
>>>>> > BUG=43459
>>>>> > TEST=Done UI Testing only.
>>>>> >
>>>>> >
>>>>> > Please review this at
>>>>> > https://chromiumcodereview.appspot.com/10117016/
>>>>> >
>>>>> > SVN Base: http://git.chromium.org/chromium/src.git@master
>>>>> >
>>>>> > Affected files:
>>>>> >  M chrome/app/chrome_command_ids.h
>>>>> >  M chrome/app/chrome_dll.rc
>>>>> >  M chrome/browser/ui/browser.h
>>>>> >  M chrome/browser/ui/browser.cc
>>>>> >  M chrome/browser/ui/gtk/accelerators_gtk.cc
>>>>> >
>>>>> >
>>>>> > Index: chrome/app/chrome_command_ids.h
>>>>> > diff --git a/chrome/app/chrome_command_ids.h
>>>>> > b/chrome/app/chrome_command_ids.h
>>>>> > index
>>>>> >
>>>>> >
5eca7a3a29ba4adaa3c6cf112bf67dc70596c077..a48a797269dcdc16c96490eddec87a2bb345df40
>>>>> > 100644
>>>>> > --- a/chrome/app/chrome_command_ids.h
>>>>> > +++ b/chrome/app/chrome_command_ids.h
>>>>> > @@ -315,6 +315,9 @@
>>>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST      52199
>>>>> >  #define IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_SETTINGS  52200
>>>>> >
>>>>> > +// Select the last visited tab.
>>>>> > +#define IDC_SELECT_NEXT_MRU_TAB 52201
>>>>> > +
>>>>> >  // NOTE: The last valid command value is 57343 (0xDFFF)
>>>>> >  // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
>>>>> >
>>>>> > Index: chrome/app/chrome_dll.rc
>>>>> > diff --git a/chrome/app/chrome_dll.rc b/chrome/app/chrome_dll.rc
>>>>> > index
>>>>> >
>>>>> >
879579a5c605a594c8fa3a53602839090fc2442e..1747effab40b06fa50842625cb6e24fc65e117f5
>>>>> > 100644
>>>>> > --- a/chrome/app/chrome_dll.rc
>>>>> > +++ b/chrome/app/chrome_dll.rc
>>>>> > @@ -122,6 +122,7 @@ BEGIN
>>>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>>>> >     VK_OEM_PLUS,    IDC_ZOOM_PLUS,              VIRTKEY, CONTROL,
>>>>> > SHIFT
>>>>> >     VK_ADD,         IDC_ZOOM_PLUS,              VIRTKEY, CONTROL
>>>>> > +    VK_OEM_3,       IDC_SELECT_NEXT_MRU_TAB,    VIRTKEY, CONTROL
>>>>> >  END
>>>>> >
>>>>> >  IDR_CHROMEFRAME ACCELERATORS
>>>>> > Index: chrome/browser/ui/browser.cc
>>>>> > diff --git a/chrome/browser/ui/browser.cc
>>>>> > b/chrome/browser/ui/browser.cc
>>>>> > index
>>>>> >
>>>>> >
9c3a807a265ca4e3aa79e5d68f9b1be623b6ee5e..06695c12f4df8e2aedf8892c9b4e481080340d25
>>>>> > 100644
>>>>> > --- a/chrome/browser/ui/browser.cc
>>>>> > +++ b/chrome/browser/ui/browser.cc
>>>>> > @@ -531,6 +531,9 @@ Browser::~Browser() {
>>>>> >     select_file_dialog_->ListenerDestroyed();
>>>>> >
>>>>> >   TabRestoreServiceDestroyed(tab_restore_service_);
>>>>> > +
>>>>> > +  // Clear the tabs mru list
>>>>> > +  tabs_mru_list_.clear();
>>>>> >  }
>>>>> >
>>>>> >  bool Browser::IsFullscreenForTabOrPending() const {
>>>>> > @@ -1884,6 +1887,19 @@ void Browser::SelectLastTab() {
>>>>> >   tab_handler_->GetTabStripModel()->SelectLastTab();
>>>>> >  }
>>>>> >
>>>>> > +void Browser::SelectNextMRUTab() {
>>>>> > +  content::RecordAction(UserMetricsAction("SelectNextMRUTab"));
>>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>>> > tabs_mru_list_.begin();
>>>>> > +  // Advance to second element in MRU list if there are more than
>>>>> > one tabs
>>>>> > open.
>>>>> > +  if (tabs_mru_list_.size() > 1) {
>>>>> > +    std::advance(it, 1);
>>>>> > +  }
>>>>> > +  if (*it) {
>>>>> > +
>>>>> >
>>>>> >
ActivateTabAt(tab_handler_->GetTabStripModel()->GetIndexOfTabContents(*it),
>>>>> > +                true);
>>>>> > +  }
>>>>> > +}
>>>>> > +
>>>>> >  void Browser::DuplicateTab() {
>>>>> >   content::RecordAction(UserMetricsAction("Duplicate"));
>>>>> >   DuplicateContentsAt(active_index());
>>>>> > @@ -3261,6 +3277,8 @@ void Browser::ExecuteCommandWithDisposition(
>>>>> >                                     break;
>>>>> >     case IDC_TOGGLE_SPEECH_INPUT:   ToggleSpeechInput();
>>>>> > break;
>>>>> >
>>>>> > +    case IDC_SELECT_NEXT_MRU_TAB:   SelectNextMRUTab();
>>>>> > break;
>>>>> > +
>>>>> >     default:
>>>>> >       LOG(WARNING) << "Received Unimplemented Command: " << id;
>>>>> >       break;
>>>>> > @@ -3487,7 +3505,6 @@ bool Browser::LargeIconsPermitted() const {
>>>>> >
>>>>> >
>>>>> >
///////////////////////////////////////////////////////////////////////////////
>>>>> >  // Browser, TabStripModelObserver implementation:
>>>>> > -
>>>>> >  void Browser::TabInsertedAt(TabContentsWrapper* contents,
>>>>> >                             int index,
>>>>> >                             bool foreground) {
>>>>> > @@ -3510,6 +3527,9 @@ void Browser::TabInsertedAt(TabContentsWrapper*
>>>>> > contents,
>>>>> >
>>>>> >   registrar_.Add(this, content::NOTIFICATION_INTERSTITIAL_DETACHED,
>>>>> >
>>>>> > content::Source<WebContents>(contents->web_contents()));
>>>>> > +
>>>>> > +  // Insert the tab at the beginning of the MRU list
>>>>> > +  tabs_mru_list_.push_front(contents);
>>>>> >  }
>>>>> >
>>>>> >  void Browser::TabClosingAt(TabStripModel* tab_strip_model,
>>>>> > @@ -3524,10 +3544,28 @@ void Browser::TabClosingAt(TabStripModel*
>>>>> > tab_strip_model,
>>>>> >
>>>>> >   // Sever the TabContents' connection back to us.
>>>>> >   SetAsDelegate(contents, NULL);
>>>>> > +
>>>>> > +  // Remove the index if found in MRU List
>>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>>> > +      std::find(tabs_mru_list_.begin(),
>>>>> > +                tabs_mru_list_.end(),
>>>>> > +                contents);
>>>>> > +  if (it != tabs_mru_list_.end()) {
>>>>> > +     tabs_mru_list_.erase(it);
>>>>> > +  }
>>>>> >  }
>>>>> >
>>>>> >  void Browser::TabDetachedAt(TabContentsWrapper* contents, int index)
>>>>> > {
>>>>> >   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
>>>>> > +
>>>>> > +  // Remove the index if found in MRU List
>>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>>> > +      std::find(tabs_mru_list_.begin(),
>>>>> > +                tabs_mru_list_.end(),
>>>>> > +                contents);
>>>>> > +  if (it != tabs_mru_list_.end()) {
>>>>> > +    tabs_mru_list_.erase(it);
>>>>> > +  }
>>>>> >  }
>>>>> >
>>>>> >  void Browser::TabDeactivated(TabContentsWrapper* contents) {
>>>>> > @@ -3603,6 +3641,17 @@ void
>>>>> > Browser::ActiveTabChanged(TabContentsWrapper*
>>>>> > old_contents,
>>>>> >   }
>>>>> >
>>>>> >   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH);
>>>>> > +
>>>>> > +
>>>>> > +  // Bring the activated tab content to the front of the list
>>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>>> > +      std::find(tabs_mru_list_.begin(),
>>>>> > +                tabs_mru_list_.end(),
>>>>> > +                new_contents);
>>>>> > +  if (it != tabs_mru_list_.end()) {
>>>>> > +    tabs_mru_list_.push_front(*it);
>>>>> > +    tabs_mru_list_.erase(it);
>>>>> > +  }
>>>>> >  }
>>>>> >
>>>>> >  void Browser::TabMoved(TabContentsWrapper* contents,
>>>>> > @@ -3640,6 +3689,19 @@ void Browser::TabReplacedAt(TabStripModel*
>>>>> > tab_strip_model,
>>>>> >
>>>>> >   content::DevToolsManager::GetInstance()->TabReplaced(
>>>>> >       old_contents->web_contents(), new_contents->web_contents());
>>>>> > +
>>>>> > +  // Replace the tab contents accordingly in the list. If the tab
>>>>> > contents
>>>>> > are
>>>>> > +  // not found add to the end of the list.
>>>>> > +  std::list<TabContentsWrapper*>::iterator it =
>>>>> > +      std::find(tabs_mru_list_.begin(),
>>>>> > +                tabs_mru_list_.end(),
>>>>> > +                old_contents);
>>>>> > +  if (it != tabs_mru_list_.end()) {
>>>>> > +    tabs_mru_list_.insert(it,new_contents);
>>>>> > +    tabs_mru_list_.erase(it);
>>>>> > +  } else {
>>>>> > +    tabs_mru_list_.push_back(new_contents);
>>>>> > +  }
>>>>> >  }
>>>>> >
>>>>> >  void Browser::TabPinnedStateChanged(TabContentsWrapper* contents,
>>>>> > int
>>>>> > index) {
>>>>> > @@ -3664,6 +3726,9 @@ void Browser::TabStripEmpty() {
>>>>> >   //       update BrowserList::CloseAllBrowsers.
>>>>> >   MessageLoop::current()->PostTask(
>>>>> >       FROM_HERE, base::Bind(&Browser::CloseFrame,
>>>>> > weak_factory_.GetWeakPtr()));
>>>>> > +
>>>>> > +  // Clear the tabs MRU List
>>>>> > +  tabs_mru_list_.clear();
>>>>> >  }
>>>>> >
>>>>> >
>>>>> >
///////////////////////////////////////////////////////////////////////////////
>>>>> > @@ -4779,6 +4844,8 @@ void Browser::InitCommandState() {
>>>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB,
>>>>> > normal_window);
>>>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
>>>>> >                                         normal_window);
>>>>> > +  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_MRU_TAB,
>>>>> > +                                        normal_window);
>>>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_NEXT,
>>>>> > normal_window);
>>>>> >   command_updater_.UpdateCommandEnabled(IDC_MOVE_TAB_PREVIOUS,
>>>>> > normal_window);
>>>>> >   command_updater_.UpdateCommandEnabled(IDC_SELECT_TAB_0,
>>>>> > normal_window);
>>>>> > Index: chrome/browser/ui/browser.h
>>>>> > diff --git a/chrome/browser/ui/browser.h
>>>>> > b/chrome/browser/ui/browser.h
>>>>> > index
>>>>> >
>>>>> >
90c6fc5f11430ffe9640bf57a041a12913152d02..b51bea50126d0237299ca4ba5d86c53983cf0b45
>>>>> > 100644
>>>>> > --- a/chrome/browser/ui/browser.h
>>>>> > +++ b/chrome/browser/ui/browser.h
>>>>> > @@ -10,6 +10,7 @@
>>>>> >  #include <set>
>>>>> >  #include <string>
>>>>> >  #include <vector>
>>>>> > +#include <list>
>>>>> >
>>>>> >  #include "base/basictypes.h"
>>>>> >  #include "base/compiler_specific.h"
>>>>> > @@ -98,6 +99,9 @@ class Browser : public TabHandlerDelegate,
>>>>> >                 public ProfileSyncServiceObserver,
>>>>> >                 public InstantDelegate {
>>>>> >  public:
>>>>> > +  // List that maintains the tab indices in the most recently
>>>>> > visited order
>>>>> > +  typedef std::list<TabContentsWrapper*> TabsMRUList;
>>>>> > +
>>>>> >   // SessionService::WindowType mirrors these values.  If you add to
>>>>> > this
>>>>> >   // enum, look at SessionService::WindowType to see if it needs to
>>>>> > be
>>>>> >   // updated.
>>>>> > @@ -566,6 +570,7 @@ class Browser : public TabHandlerDelegate,
>>>>> >   void MoveTabPrevious();
>>>>> >   void SelectNumberedTab(int index);
>>>>> >   void SelectLastTab();
>>>>> > +  void SelectNextMRUTab();
>>>>> >   void DuplicateTab();
>>>>> >   void WriteCurrentURLToClipboard();
>>>>> >   void ConvertPopupToTabbedBrowser();
>>>>> > @@ -1514,6 +1519,8 @@ class Browser : public TabHandlerDelegate,
>>>>> >   // before DidEndColorChooser is called.
>>>>> >   scoped_ptr<content::ColorChooser> color_chooser_;
>>>>> >
>>>>> > +  TabsMRUList tabs_mru_list_;
>>>>> > +
>>>>> >   DISALLOW_COPY_AND_ASSIGN(Browser);
>>>>> >  };
>>>>> >
>>>>> > Index: chrome/browser/ui/gtk/accelerators_gtk.cc
>>>>> > diff --git a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>>> > b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>>> > index
>>>>> >
>>>>> >
509c24872ae41746bb3214ac59e351cf5dc315df..3988f0bdf9615d8048edc7399646d9d7c9bf9917
>>>>> > 100644
>>>>> > --- a/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>>> > +++ b/chrome/browser/ui/gtk/accelerators_gtk.cc
>>>>> > @@ -39,6 +39,8 @@ const struct AcceleratorMapping {
>>>>> >   { GDK_Page_Up, IDC_MOVE_TAB_PREVIOUS,
>>>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>>>> >   { GDK_Page_Up, IDC_SELECT_PREVIOUS_TAB, GDK_CONTROL_MASK },
>>>>> > +  { GDK_asciitilde, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>>>> > +  { GDK_quoteleft, IDC_SELECT_NEXT_MRU_TAB, GDK_CONTROL_MASK },
>>>>> >   { GDK_w, IDC_CLOSE_TAB, GDK_CONTROL_MASK },
>>>>> >   { GDK_t, IDC_RESTORE_TAB,
>>>>> >     GdkModifierType(GDK_CONTROL_MASK | GDK_SHIFT_MASK) },
>>>>> >
>>>>> >
>>>>
>>>>
>>>
>>
>

[deadduck169, 2013-03-09 05:36:57]

On 2013/02/25 21:42:51, sky wrote:
> See Glen's earlier comment:
> 
> "I could've sworn I'd responded to this already - we're not interested
> in replacing the Ctrl+Tab behavior or adding a pref; if we can't find
> an alternate shortcut for this, we may have to cancel."
> 
>   -Scott
> 

How could adding a preference to toggle the ctrl-tab behavior between
next/previous tab and MRU possibly affect anybody negatively under any
circumstances? I honestly can't think of any. It seems mind-boggling to me to
leave the decision of something like this up to one single person. This is
obviously a large show-stopper for many people. Don't believe me? Try looking at
this: http://code.google.com/p/chromium/issues/detail?id=5569 Nearly 600 stars
and counting plus another 60 people here:
http://code.google.com/p/chromium/issues/detail?id=43459. Listen to the people.
Add a vote or whatever you want, but don't just dismiss this because one single
person is happy enough with the current behavior and feels that anybody who
thinks differently is wrong. You have your way to switch tabs, other people have
theirs. Catering to everyone's desires is not a bad thing. That's why there is a
"settings" dialog to begin with. Let's put it to use. Leave the default behavior
how it is right now but at least add an option SOMEWHERE (even in the command
line if that's what it takes) to change this to MRU. Personally this is THE
single biggest annoyance with Chrome for me right now and I know I'm not alone.

[Dave Myron, 2013-08-06 20:38:51]

On 2013/03/09 05:36:57, deadduck169 wrote:
> On 2013/02/25 21:42:51, sky wrote:
> > See Glen's earlier comment:
> > 
> > "I could've sworn I'd responded to this already - we're not interested
> > in replacing the Ctrl+Tab behavior or adding a pref; if we can't find
> > an alternate shortcut for this, we may have to cancel."
> > 
> >   -Scott
> > 
> 
> How could adding a preference to toggle the ctrl-tab behavior between
> next/previous tab and MRU possibly affect anybody negatively under any
> circumstances? I honestly can't think of any. It seems mind-boggling to me to
> leave the decision of something like this up to one single person. 

Agreed. "We're not interested" is among the sorriest excuses seen in a while to
not change something that A LOT of people consider either a) a major annoyance
with Chrome or b) a complete switch-stopper from other browsers that understand
the better UX provided by MRU. (Imagine if Alt+Tab on Windows or Cmd+Tab on OSX
made you cycle through all application windows instead of using MRU!)

We have a patch, above, that gets us this highly desired functionality. Please,
put the dang thing as an option, at least!

[deadduck169, 2013-11-18 22:18:18]

On 2013/02/25 21:42:51, sky wrote:
> See Glen's earlier comment:
> 
> "I could've sworn I'd responded to this already - we're not interested
> in replacing the Ctrl+Tab behavior or adding a pref; if we can't find
> an alternate shortcut for this, we may have to cancel."
> 
>   -Scott
> 

Weird. I don't see this comment anywhere. Did he change his mind and delete it?
Anyway, if the only thing that is preventing this from being included is
deciding on a keyboard shortcut. I have a few suggestions:

* Ideally, add a setting to toggle the Ctrl-Tab/Ctrl-Shift-Tab behavior between
prev/next and MRU. If not, I would love if someone could elaborate more on
Glen's comment above. There are obviously many people interested in this
feature, so there must be some valid reason to not include it beyond one dev
simply not being interested. I'm not interested in painting my house pink, but
it doesn't mean that my paint store should eliminate all pink paint.

If overriding Ctrl-Tab is absolutely not possible, then perhaps any combination
of the following could be worth exploring:

* Set the shortcut to something logical based on the current keyboard layout, or
the layout at browser start up. For example, use Ctrl+{key in the English
keyboard ~ location}. For Spanish, this would be something like Ctrl+º.
* Similar to the above, set the shortcut to something logical based on the
current browser language. For each language, assume the most common layout for
that language
* Make this default to a specific shortcut (like Ctrl+~) and allow the user to
override it in the settings to whatever key combination they want. If this
combination is already assigned to something else, either prevent the change or
prompt the user with a dialog box to override the other shortcut or cancel.