Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6933037:
Multi-tab selection for Linux. (Closed)
|
Created:
9 years, 7 months ago by dpapad Modified:
9 years, 5 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMulti-tab selection for Linux.
This CL enables multi-tab selection on Linux (shift+click, ctrl+click and context menu commands). Dragging multiple tabs in not implemented yet (probably on a follow up CL).
BUG=30572
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90698
Patch Set 1 #Patch Set 2 : Enabling crtl click #Patch Set 3 : Enabling shift click #Patch Set 4 : Minor renaming #Patch Set 5 : Fixing selected, not active tab backgroung rendering #Patch Set 6 : Fixing context menu commands #
Total comments: 1
Patch Set 7 : Refactoring ContextMenuController, context menu commands still work. #Patch Set 8 : Removing unnecessary includes #
Total comments: 16
Patch Set 9 : Addressing comments #Patch Set 10 : Removing check, addressing comments #
Total comments: 22
Patch Set 11 : Addressing comments #Patch Set 12 : Addressing comments #Patch Set 13 : Addressing comments, rebasing #
Total comments: 8
Patch Set 14 : Nits, fixing comments #Patch Set 15 : Adding TabSelectionChanged callback and removing unnecessary method #
Total comments: 15
Patch Set 16 : Addressing comments #Patch Set 17 : Rebasing, modifying TabSelectionChanged arguments, notifying observers when necessary. #
Total comments: 4
Patch Set 18 : Fixing nits #
Total comments: 10
Patch Set 19 : Removing unnecessary check, using shorthand to populate set. #Patch Set 20 : Rebasing, resolving conflicts #Patch Set 21 : Removing unused include #
Total comments: 13
Patch Set 22 : Addressing comments #Patch Set 23 : Addressing comments #Patch Set 24 : Removing ActiveTabChanged from TabStripGtk #
Total comments: 1
Patch Set 25 : Repainting active and previously active tab explicitly #
Total comments: 1
Patch Set 26 : Addressing comments #Patch Set 27 : Fixing clang errors #Messages
Total messages: 44 (0 generated)
http://codereview.chromium.org/6933037/diff/11001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/11001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:40: tab->delegate()->GetIndexOfTab(const_cast<const TabGtk*>(tab))) { This is the hacky part. In order to invoke the desired constructor of TabMenuModel (model_ in this case) I need a reference to the TabStripModel and this can only be done by adding an extra getter method to the TabDelegate.
probably best to match what sky did on Views, which was to refactor the context menu out of the tab and into the tabstrip
On 2011/05/10 16:40:46, Evan Stade wrote: > probably best to match what sky did on Views, which was to refactor the context > menu out of the tab and into the tabstrip Ok, this sounds better and it is what I had in mind yesterday when I was referring to some refactoring being needed.
http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/context_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/context_menu_controller.cc:12: ContextMenuController::ContextMenuController(TabGtk* tab, this class should probably be TabStripContextMenuController now that there is no TabMenu:: namespace. (And you could drop Context to reduce the length.) http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/context_menu_controller.cc:56: if (!tab_) when would this be null? http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:107: if (!IsActive() && (event->state & 5) == 0) { don't use magic constants like 5 http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:111: NOTIMPLEMENTED() << " Multi tab selection ctrl click under construction"; it looks implemented http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:114: NOTIMPLEMENTED() << " Multi tab selection shift click under construction"; ditto http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1136: if (model_->ContainsIndex(index)) when could this check be false? why isn't it used in IsTabPinned or GetContextMenuControllerForTab?
http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/context_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/context_menu_controller.cc:12: ContextMenuController::ContextMenuController(TabGtk* tab, On 2011/05/11 20:58:27, Evan Stade wrote: > this class should probably be TabStripContextMenuController now that there is no > TabMenu:: namespace. (And you could drop Context to reduce the length.) Done. http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/context_menu_controller.cc:56: if (!tab_) On 2011/05/11 20:58:27, Evan Stade wrote: > when would this be null? Again, I did not change these checks. tab_ is set to null in Cancel(). I am guessing that it cant harm to have this check and avoid dereferencing a null pointer in some corner case. http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:107: if (!IsActive() && (event->state & 5) == 0) { On 2011/05/11 20:58:27, Evan Stade wrote: > don't use magic constants like 5 Done. http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:111: NOTIMPLEMENTED() << " Multi tab selection ctrl click under construction"; On 2011/05/11 20:58:27, Evan Stade wrote: > it looks implemented Done. http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:114: NOTIMPLEMENTED() << " Multi tab selection shift click under construction"; On 2011/05/11 20:58:27, Evan Stade wrote: > ditto Done. http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1136: if (model_->ContainsIndex(index)) On 2011/05/11 20:58:27, Evan Stade wrote: > when could this check be false? why isn't it used in IsTabPinned or > GetContextMenuControllerForTab? I din not change these checks and I do not know if it is needed. I had no indication to remove it either (documentation of ContainsIndex does not give any examples).
http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/context_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/context_menu_controller.cc:56: if (!tab_) so I guess it depends on whether you could call Cancel() and still get an ExecuteCommand(). If this is possible, there should at least be a comment to that effect which explains the conditions under which it might happen, otherwise, this check shouldn't be here or should be a DCHECK. http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1136: if (model_->ContainsIndex(index)) On 2011/05/11 23:30:20, dpapad wrote: > On 2011/05/11 20:58:27, Evan Stade wrote: > > when could this check be false? why isn't it used in IsTabPinned or > > GetContextMenuControllerForTab? > > I din not change these checks and I do not know if it is needed. I had no > indication to remove it either (documentation of ContainsIndex does not give any > examples). you didn't change this check but you did duplicate it. It would be nice to know why you are deciding to duplicate it when you might have decided to leave it off. As far as whether it's better to proof your code against unexpected failure, I think it's not: this will just cover up other programming errors. "It is far better to receive detailed crash reports than vague anecdotes about unreproducible hangs, dead clicks, or other misbehaving non-crash conditions."[1] [1] http://www.chromium.org/developers/coding-style
http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/context_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/context_menu_controller.cc:56: if (!tab_) On 2011/05/12 00:05:27, Evan Stade wrote: > so I guess it depends on whether you could call Cancel() and still get an > ExecuteCommand(). If this is possible, there should at least be a comment to > that effect which explains the conditions under which it might happen, > otherwise, this check shouldn't be here or should be a DCHECK. I will try and see and also ask James, since svn blame returns his name. http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1136: if (model_->ContainsIndex(index)) On 2011/05/12 00:05:27, Evan Stade wrote: > On 2011/05/11 23:30:20, dpapad wrote: > > On 2011/05/11 20:58:27, Evan Stade wrote: > > > when could this check be false? why isn't it used in IsTabPinned or > > > GetContextMenuControllerForTab? > > > > I din not change these checks and I do not know if it is needed. I had no > > indication to remove it either (documentation of ContainsIndex does not give > any > > examples). > > you didn't change this check but you did duplicate it. It would be nice to know > why you are deciding to duplicate it when you might have decided to leave it > off. As far as whether it's better to proof your code against unexpected > failure, I think it's not: this will just cover up other programming errors. > > "It is far better to receive detailed crash reports than vague anecdotes about > unreproducible hangs, dead clicks, or other misbehaving non-crash > conditions."[1] > > [1] http://www.chromium.org/developers/coding-style I am removing it from ToggleSelection(), since according to my understanding this is not possible (cant control+click on a tab that is not in the strip). Also a DCHECK is not needed since it is performed already within TabStripModel::ToggleSelectionAt. Do you know whether unit tests cover these cases?
On 2011/05/12 00:34:25, dpapad wrote: > http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... > File chrome/browser/ui/gtk/tabs/context_menu_controller.cc (right): > > http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... > chrome/browser/ui/gtk/tabs/context_menu_controller.cc:56: if (!tab_) > On 2011/05/12 00:05:27, Evan Stade wrote: > > so I guess it depends on whether you could call Cancel() and still get an > > ExecuteCommand(). If this is possible, there should at least be a comment to > > that effect which explains the conditions under which it might happen, > > otherwise, this check shouldn't be here or should be a DCHECK. > > I will try and see and also ask James, since svn blame returns his name. > > http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... > File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): > > http://codereview.chromium.org/6933037/diff/19001/chrome/browser/ui/gtk/tabs/... > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1136: if > (model_->ContainsIndex(index)) > On 2011/05/12 00:05:27, Evan Stade wrote: > > On 2011/05/11 23:30:20, dpapad wrote: > > > On 2011/05/11 20:58:27, Evan Stade wrote: > > > > when could this check be false? why isn't it used in IsTabPinned or > > > > GetContextMenuControllerForTab? > > > > > > I din not change these checks and I do not know if it is needed. I had no > > > indication to remove it either (documentation of ContainsIndex does not give > > any > > > examples). > > > > you didn't change this check but you did duplicate it. It would be nice to > know > > why you are deciding to duplicate it when you might have decided to leave it > > off. As far as whether it's better to proof your code against unexpected > > failure, I think it's not: this will just cover up other programming errors. > > > > "It is far better to receive detailed crash reports than vague anecdotes about > > unreproducible hangs, dead clicks, or other misbehaving non-crash > > conditions."[1] > > > > [1] http://www.chromium.org/developers/coding-style > > I am removing it from ToggleSelection(), since according to my understanding > this is not possible (cant control+click on a tab that is not in the strip). the comparable functions in Views make similar checks. You might ask sky why since I can't find documentation anyhwere. > Also a DCHECK is not needed since it is performed already within > TabStripModel::ToggleSelectionAt. Do you know whether unit tests cover these > cases? I doubt it.
http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:107: if (!IsActive() && (event->state & && !(event->state & (GDK_SHIFT_MASK | GDK_CONTROL_MASK)) http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:110: } else if (!IsActive() && (event->state & GDK_CONTROL_MASK) == It's merely enough to check `event->state & GDK_CONTROL_MASK`. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.h (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.h:96: virtual TabStripMenuController* GetContextMenuControllerForTab( Should this be called GetTabStripMenuControllerForTab now? http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:911: tab_id = IDR_THEME_TAB_BACKGROUND_V; What does the 'V' stand for? http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:56: if (!tab_) Under what circumstances is |tab_| NULL here? http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:66: MenuGtk::Delegate::GetDefaultImageForCommandId(browser_cmd_id) : nit: This is somewhat hard to read. I'd not use a ternary here. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:23: class TabStripMenuController : public ui::SimpleMenuModel::Delegate, Document the class. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:26: TabStripMenuController(TabGtk* tab, TabStripModel* model, int index); Document the parameters. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:39: GtkWidget* GetImageForCommandId(int command_id) const; // Overridden from ... http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:46: TabGtk* tab_; Document ownership.
http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:107: if (!IsActive() && (event->state & On 2011/05/12 00:53:54, James Hawkins wrote: > && !(event->state & (GDK_SHIFT_MASK | GDK_CONTROL_MASK)) Done. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:110: } else if (!IsActive() && (event->state & GDK_CONTROL_MASK) == On 2011/05/12 00:53:54, James Hawkins wrote: > It's merely enough to check `event->state & GDK_CONTROL_MASK`. Done. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.h (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.h:96: virtual TabStripMenuController* GetContextMenuControllerForTab( On 2011/05/12 00:53:54, James Hawkins wrote: > Should this be called GetTabStripMenuControllerForTab now? Done. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:911: tab_id = IDR_THEME_TAB_BACKGROUND_V; On 2011/05/12 00:53:54, James Hawkins wrote: > What does the 'V' stand for? I do not know what V stands for and also it does not seem to be documented. It is defined in chrome/browser/themes/theme_service.cc and corresponds to file chrome/app/themes/theme_tab_background_glass.png. I am using it to render a selected, not active tab which seems to be what it should be used for (but again I did not see this documented). http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:56: if (!tab_) On 2011/05/12 00:53:54, James Hawkins wrote: > Under what circumstances is |tab_| NULL here? That was Evan's question I was deferring to you. I have to investigate to see if NULL is possible. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:66: MenuGtk::Delegate::GetDefaultImageForCommandId(browser_cmd_id) : On 2011/05/12 00:53:54, James Hawkins wrote: > nit: This is somewhat hard to read. I'd not use a ternary here. Done. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:23: class TabStripMenuController : public ui::SimpleMenuModel::Delegate, On 2011/05/12 00:53:54, James Hawkins wrote: > Document the class. Done. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:26: TabStripMenuController(TabGtk* tab, TabStripModel* model, int index); On 2011/05/12 00:53:54, James Hawkins wrote: > Document the parameters. Done. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:39: GtkWidget* GetImageForCommandId(int command_id) const; On 2011/05/12 00:53:54, James Hawkins wrote: > // Overridden from ... Done. http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:46: TabGtk* tab_; On 2011/05/12 00:53:54, James Hawkins wrote: > Document ownership. No ownership comment implies that it is not owned by this class's objects, right? I have only seen ownership comments in the reverse case.
http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:46: TabGtk* tab_; On 2011/05/12 16:37:08, dpapad wrote: > On 2011/05/12 00:53:54, James Hawkins wrote: > > Document ownership. > > No ownership comment implies that it is not owned by this class's objects, > right? I have only seen ownership comments in the reverse case. No. "Absence of evidence..." The correct documentation for this is 'weak reference' or 'weak pointer' depending on your preference.
http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/27001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:46: TabGtk* tab_; On 2011/05/12 16:49:48, James Hawkins wrote: > On 2011/05/12 16:37:08, dpapad wrote: > > On 2011/05/12 00:53:54, James Hawkins wrote: > > > Document ownership. > > > > No ownership comment implies that it is not owned by this class's objects, > > right? I have only seen ownership comments in the reverse case. > > No. "Absence of evidence..." The correct documentation for this is 'weak > reference' or 'weak pointer' depending on your preference. Done.
lgtm http://codereview.chromium.org/6933037/diff/28011/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/28011/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:107: if (!IsActive() && !(event->state & (GDK_SHIFT_MASK | GDK_CONTROL_MASK))) { could you pull the !IsActive() check out into an outer if
http://codereview.chromium.org/6933037/diff/28011/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/28011/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:56: if (!tab_) I looked into this. This logic was apparently to guard against the user clicking on an opened context menu of a tab after the tab has closed (you can test this w/ some JS that closes the window). I think it's highly unlikely that this could happen, but I can't verify that it's impossible to race here. I'd add a comment to this effect.
http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:107: if (!IsActive()) { Done. http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:58: // through javascript. Done.
LGTM. http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:108: if (!(event->state & (GDK_SHIFT_MASK | GDK_CONTROL_MASK))) This logic can be removed and just make this block the else block. http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:911: tab_id = IDR_THEME_TAB_BACKGROUND_V; Does this also work for incognito windows? http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:58: // through javascript. nit: Tabs can be closed various ways that don't require user interaction; just note that it's possible the context menu may be open as the tab is closing.
http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.cc:108: if (!(event->state & (GDK_SHIFT_MASK | GDK_CONTROL_MASK))) On 2011/05/12 21:59:52, James Hawkins wrote: > This logic can be removed and just make this block the else block. Done. http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:911: tab_id = IDR_THEME_TAB_BACKGROUND_V; On 2011/05/12 21:59:52, James Hawkins wrote: > Does this also work for incognito windows? It seems to me that a new theme should be added for not active, selected incognito tabs in theme_service.cc, probably called IDR_THEME_TAB_BACKGROUND_INCOGNITO_V and then this if statement should be slightly modified. For the moment any not active, selected tabs use the same theme. I will ask rsesek and sky about the themes. http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/23009/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:58: // through javascript. On 2011/05/12 21:59:52, James Hawkins wrote: > nit: Tabs can be closed various ways that don't require user interaction; just > note that it's possible the context menu may be open as the tab is closing. Done.
http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model.cc (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model.cc:572: std::vector<int> old_selected_indices( nit: You use indices here and indexes in the NotifyMultipleSelectionChanged parameter; please pick one for consistency. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model.h (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model.h:528: // Notifies the observers the selection changed. |old_selected_indexes| gives nit: The last sentence is a bit redundant. Something like "|old_selected_indexes| contains the list of indexes that were previously selected." would be more useful. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model_observer.h (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model_observer.h:86: virtual void TabSelectionChanged(std::set<int>& indices_affected); Document this method. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.h (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.h:45: // Activate the specified Tab. s/Activate/Activates/ http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.h:48: // Toggle selection of the specified Tab. s/Toggle/Toggles/ http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1041: LOG(INFO) << "tab selection changed"; Remove logging, here and elsewhere. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1043: for (it = indices_affected.begin() ; it != indices_affected.end(); it++) { ++it http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a... Personally I use preincrement for all types now in for loops so I don't have to remember this. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:24: // TabGtk::OnButtonPressEvent every a time right click occurs on a tab. Don't specify implementation details of interfaces (this class), i.e., just leave of "in TabGtk::OnButtonPressEvent".
http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model.cc (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model.cc:572: std::vector<int> old_selected_indices( On 2011/05/18 18:03:10, James Hawkins wrote: > nit: You use indices here and indexes in the NotifyMultipleSelectionChanged > parameter; please pick one for consistency. Done. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model.h (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model.h:528: // Notifies the observers the selection changed. |old_selected_indexes| gives On 2011/05/18 18:03:10, James Hawkins wrote: > nit: The last sentence is a bit redundant. Something like > "|old_selected_indexes| contains the list of indexes that were previously > selected." would be more useful. Done. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model_observer.h (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model_observer.h:86: virtual void TabSelectionChanged(std::set<int>& indices_affected); On 2011/05/18 18:03:10, James Hawkins wrote: > Document this method. Done. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_gtk.h (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.h:45: // Activate the specified Tab. On 2011/05/18 18:03:10, James Hawkins wrote: > s/Activate/Activates/ Done. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_gtk.h:48: // Toggle selection of the specified Tab. On 2011/05/18 18:03:10, James Hawkins wrote: > s/Toggle/Toggles/ Done. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1041: LOG(INFO) << "tab selection changed"; On 2011/05/18 18:03:10, James Hawkins wrote: > Remove logging, here and elsewhere. Done. http://codereview.chromium.org/6933037/diff/26025/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1043: for (it = indices_affected.begin() ; it != indices_affected.end(); it++) { On 2011/05/18 18:03:10, James Hawkins wrote: > ++it > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a... > > Personally I use preincrement for all types now in for loops so I don't have to > remember this. Done.
LGTM with nits. http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1044: size_t i = 0; nit: Move loop variables into the loop declaration, even though you're using the same loop var twice (especially so). http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:23: // Controls the context menu of a specific tab. It is created in every a time s/in //
http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1044: size_t i = 0; On 2011/06/01 18:20:40, James Hawkins wrote: > nit: Move loop variables into the loop declaration, even though you're using the > same loop var twice (especially so). Done. http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/44002/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:23: // Controls the context menu of a specific tab. It is created in every a time On 2011/06/01 18:20:40, James Hawkins wrote: > s/in // Done.
Addressing estade's comments (he commented them in person).
http://codereview.chromium.org/6933037/diff/40013/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model_observer.h (right): http://codereview.chromium.org/6933037/diff/40013/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model_observer.h:84: virtual void TabSelectionChanged(const std::vector<int>& previously_selected, Add test coverage of this. Also, it seems like you don't always invoke this when the selected indices change. The case I think you don't send out notification are add/remove. For example, if you add/remove tabs, the selection in terms of indices changes. http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1040: void TabStripGtk::TabSelectionChanged( Seems like you should only have to override one of the selection change methods. http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1048: for (size_t i = 0; i < currently_selected.size(); i++) Can this be indices_affected.insert(currently_selected.begin(), currently_selected.end())? http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1051: for (it = indices_affected.begin(); it != indices_affected.end(); ++it) move definition of iterator into for loop. http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1052: GetTabAt(*it)->SchedulePaint(); The indices passed to this method are from the model. GetTabAt() accesses tab_data_. The two differ when a tab is being deleted.
http://codereview.chromium.org/6933037/diff/40013/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model_observer.h (right): http://codereview.chromium.org/6933037/diff/40013/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model_observer.h:84: virtual void TabSelectionChanged(const std::vector<int>& previously_selected, On 2011/06/01 20:56:04, sky wrote: > Add test coverage of this. > Also, it seems like you don't always invoke this when the selected indices > change. The case I think you don't send out notification are add/remove. For > example, if you add/remove tabs, the selection in terms of indices changes. Yes, currently TabSelectionChanged() is called from ActivateTabAt(), ExtendSelectionTo(), ToggleSelectionAt(), AddSelectionFromAnchorTo() and SetSelectionFromModel(). It is intentionally not called from InsertTabContentsAt() and CloseTabContentsAt(), for 2 reasons, 1st) These events seem to be handled fine by existing callbacks (in terms of repainting) and 2nd) to avoid sending indices of tabs that no longer exist (as noted in one of your comments). I see two approaches 1) Add some more comments in TabSelectionChanged explaining that it is not always fired when the selection changes. 2) Call TabSelectionChanged() from InsertTabContentsAt() and CloseTabContentsAt() too and find out how to deal with differences in model_ and tab_data_ in TabStripGtk (maybe send the TabContentsWrapper* instead of the indices?). http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1040: void TabStripGtk::TabSelectionChanged( On 2011/06/01 20:56:04, sky wrote: > Seems like you should only have to override one of the selection change methods. TabStripGtk::ActiveTabChanged() seems to me that is doing more than just scheduling a repaint and since I dont understand exactly what it does I kept it as is instead of replacing with TabSelectionChanged(). http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1048: for (size_t i = 0; i < currently_selected.size(); i++) On 2011/06/01 20:56:04, sky wrote: > Can this be indices_affected.insert(currently_selected.begin(), > currently_selected.end())? Done. http://codereview.chromium.org/6933037/diff/40013/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1051: for (it = indices_affected.begin(); it != indices_affected.end(); ++it) On 2011/06/01 20:56:04, sky wrote: > move definition of iterator into for loop. Done.
http://codereview.chromium.org/6933037/diff/40013/chrome/browser/tabs/tab_str... File chrome/browser/tabs/tab_strip_model_observer.h (right): http://codereview.chromium.org/6933037/diff/40013/chrome/browser/tabs/tab_str... chrome/browser/tabs/tab_strip_model_observer.h:84: virtual void TabSelectionChanged(const std::vector<int>& previously_selected, On 2011/06/02 00:28:22, dpapad wrote: > On 2011/06/01 20:56:04, sky wrote: > > Add test coverage of this. > > Also, it seems like you don't always invoke this when the selected indices > > change. The case I think you don't send out notification are add/remove. For > > example, if you add/remove tabs, the selection in terms of indices changes. > > Yes, currently TabSelectionChanged() is called from ActivateTabAt(), > ExtendSelectionTo(), ToggleSelectionAt(), AddSelectionFromAnchorTo() and > SetSelectionFromModel(). It is intentionally not called from > InsertTabContentsAt() and CloseTabContentsAt(), for 2 reasons, 1st) These events > seem to be handled fine by existing callbacks (in terms of repainting) and 2nd) > to avoid sending indices of tabs that no longer exist (as noted in one of your > comments). > I see two approaches > 1) Add some more comments in TabSelectionChanged explaining that it is not > always fired when the selection changes. > 2) Call TabSelectionChanged() from InsertTabContentsAt() and > CloseTabContentsAt() too and find out how to deal with differences in model_ and > tab_data_ in TabStripGtk (maybe send the TabContentsWrapper* instead of the > indices?). You're right on 1. My intention with the two methods is the following: . ActiveTabChanged: only sent when the active tab changes. Currently it isn't and most implementations check before doing anything. At the time I added multi-selection I went this route, rather than a new method, as we weren't sure if we were going to keep the feature. . TabSelectionChanged: sent when the selection changes, but not necessarily the active tab, and not when tabs are inserted/removed. Since TabSelectionChanged will be sent when ActiveTabChanged is sent now that means it should be enough for the TabStrip implementations to only listen to TabSelectionChanged. I think it would be best to separate out the model changes into a patch doing the following: . Only send ActiveTabChanged when the active tab really changes. You'll then be able to remove a bunch of checks I added. See http://src.chromium.org/viewvc/chrome?view=rev&revision=76619 . . Add TabSelectionChanged and change the places that care about any selection change to override TabSelectionChanged instead of ActiveTabChanged. . For TabSelectionChanged, make it take const TabStripSelectionModel& as the only argument. It'll be the selection before the change. If folks care about the new selection, they can query the model. . Add/update tests.
> I think it would be best to separate out the model changes into a patch doing > the following: > . Only send ActiveTabChanged when the active tab really changes. You'll then be > able to remove a bunch of checks I added. See > http://src.chromium.org/viewvc/chrome?view=rev&revision=76619 . > . Add TabSelectionChanged and change the places that care about any selection > change to override TabSelectionChanged instead of ActiveTabChanged. > . For TabSelectionChanged, make it take const TabStripSelectionModel& as the > only argument. It'll be the selection before the change. If folks care about the > new selection, they can query the model. > . Add/update tests. Thanks for the instructions. I agree that these changes to the model should be on a separate patch. One question though: How would the argument const TabStripSelectionModel& contain the selected tabs before the change, since all notifications (including TabSelectionChanged) are sent after any action has taken place. Am I missing something?
On 2011/06/02 17:02:10, dpapad wrote: > > I think it would be best to separate out the model changes into a patch doing > > the following: > > . Only send ActiveTabChanged when the active tab really changes. You'll then > be > > able to remove a bunch of checks I added. See > > http://src.chromium.org/viewvc/chrome?view=rev&revision=76619 . > > . Add TabSelectionChanged and change the places that care about any selection > > change to override TabSelectionChanged instead of ActiveTabChanged. > > . For TabSelectionChanged, make it take const TabStripSelectionModel& as the > > only argument. It'll be the selection before the change. If folks care about > the > > new selection, they can query the model. > > . Add/update tests. > > Thanks for the instructions. I agree that these changes to the model should be > on a separate patch. One question though: How would the argument const > TabStripSelectionModel& contain the selected tabs before the change, since all > notifications (including TabSelectionChanged) are sent after any action has > taken place. Am I missing something? You'll have to do it similar to how you were copying the indices in your current patch. TabStripSelectionModel has a Copy method for this sort of thing.
> All remaining occurrences that did not
> have this check were accessing the parameters of
> ActiveTabChanged (with one exception
> chrome/browser/ui/toolbar/wrench_menu_model.cc), so for the
> moment there is no site overriding TabSelectionChanged.
Doesn't this mean those sites need to now override both?
And this is what I'm worried about. I really want folks that care about any
selection change to be able to override TabSelectionChanged. The reason you
raised for not doing this is selection changes that are the result of
insert/remove and what the indices mean. For that case, we should send
TabInsertedAt to observers, then notify the selection model changed (if it did).
That way, the indices still work. Eg:
selection_model_.IncrementFrom(index);
FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
TabInsertedAt(contents, index, active));
TabStripSelectionModel old_model;
old_model.Copy(selection_model_);
if (active) {
selection_model_.SetSelectedIndex(index);
NotifyIfActiveOrSelectionChanged(old_model);
} else {
NotifyIfActiveOrSelectionChanged(old_model);
}
On Thu, Jun 2, 2011 at 4:57 PM, <sky@chromium.org> wrote: > All remaining occurrences that did not >> have this check were accessing the parameters of >> ActiveTabChanged (with one exception >> chrome/browser/ui/toolbar/**wrench_menu_model.cc), so for the >> moment there is no site overriding TabSelectionChanged. >> > > Doesn't this mean those sites need to now override both? > It could but not necessarily. Some authors might just have omitted to add the check you added, even though they were only interested for changes of the active index. In any case what should be done when overriding it is not clear, since the args of ActiveTabChanged are used in almost all cases. Could take a look on those files individually and provide some guidelines? > And this is what I'm worried about. I really want folks that care about any > selection change to be able to override TabSelectionChanged. The reason you > raised for not doing this is selection changes that are the result of > insert/remove and what the indices mean. For that case, we should send > TabInsertedAt to observers, then notify the selection model changed (if it > did). > That way, the indices still work. Eg: > > selection_model_.**IncrementFrom(index); > FOR_EACH_OBSERVER(**TabStripModelObserver, observers_, > TabInsertedAt(contents, index, active)); > TabStripSelectionModel old_model; > old_model.Copy(selection_**model_); > if (active) { > selection_model_.**SetSelectedIndex(index); > I think it would be even better to call ActivateTabAt(index) here (instead of SetSelectedIndex()) and let this take care of sending any notification. > NotifyIfActiveOrSelectionChang**ed(old_model); > } else { > NotifyIfActiveOrSelectionChang**ed(old_model); > > } > > > http://codereview.chromium.**org/6933037/<http://codereview.chromium.org/6933... >
After landing http://codereview.chromium.org/7215003/, I rebased this CL and made it use the new notification TabStripModelObserver::TabSelectionChanged().
SLGTM http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1051: it != indices_affected.end(); ++it) nit: Indentation looks off by one here. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.cc:46: return false; nit: This would be more readable if you add a blank line after this line. In general I'm a fan of blank lines after one-line if blocks, but it's up to your discretion.
LGTM http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:442: // The selected tab clips favicon before close button. selected -> active http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:829: SkColor title_color = IsActive() ? selected_title_color_ On windows we use IsSelected for this one. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:8: #include <vector> This is already included by the header. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1050: for (std::vector<int>::iterator it = indices_affected.begin(); If there is a deletion in progress then the indices from the model don't line up with the indices of the TabStrip. This is true of all the methods in this class though, so you might just want to add a TODO some where that it needs to be straightened out. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:38: virtual bool IsCommandIdChecked(int command_id) const; OVERRIDE
http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:442: // The selected tab clips favicon before close button. On 2011/06/23 18:14:03, sky wrote: > selected -> active Done. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:829: SkColor title_color = IsActive() ? selected_title_color_ On 2011/06/23 18:14:03, sky wrote: > On windows we use IsSelected for this one. Done. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:8: #include <vector> On 2011/06/23 18:14:03, sky wrote: > This is already included by the header. Done. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1050: for (std::vector<int>::iterator it = indices_affected.begin(); On 2011/06/23 18:14:03, sky wrote: > If there is a deletion in progress then the indices from the model don't line up > with the indices of the TabStrip. This is true of all the methods in this class > though, so you might just want to add a TODO some where that it needs to be > straightened out. Why is that happening? In TabStripModel::DetachTabContentsAt() we store the |old_model| after calling DecrementFrom. Also observers should get a TabClosingAt notification before getting notified of any TabSelectionChanged, right? http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1051: it != indices_affected.end(); ++it) On 2011/06/23 18:08:26, James Hawkins wrote: > nit: Indentation looks off by one here. Done. http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:38: virtual bool IsCommandIdChecked(int command_id) const; On 2011/06/23 18:14:03, sky wrote: > OVERRIDE Done.
> http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1050: for > (std::vector<int>::iterator it = indices_affected.begin(); > On 2011/06/23 18:14:03, sky wrote: >> >> If there is a deletion in progress then the indices from the model > > don't line up >> >> with the indices of the TabStrip. This is true of all the methods in > > this class >> >> though, so you might just want to add a TODO some where that it needs > > to be >> >> straightened out. > > Why is that happening? In TabStripModel::DetachTabContentsAt() we store > the |old_model| after calling DecrementFrom. Also observers should get a > TabClosingAt notification before getting notified of any > TabSelectionChanged, right? There are two coordinate systems at play. There's the coordinates of the model and the coordinates of the tabstrip. Normally they are the same. The one exception to that is during a deletion. This happens because the model is updated immediately, but the tabstrip animates the deletion. During that animation the indices no longer line up. I was wrong about the problem being prevalent here. You should use GetTabAtAdjustForAnimation here and then it'll be good. Also, TabStripGtk shouldn't need to override ActiveTabChanged. It should be like you changed TabStrip::SetSelection. -Scott > > http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1051: it != > indices_affected.end(); ++it) > On 2011/06/23 18:08:26, James Hawkins wrote: >> >> nit: Indentation looks off by one here. > > Done. > > http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... > File chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h (right): > > http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... > chrome/browser/ui/gtk/tabs/tab_strip_menu_controller.h:38: virtual bool > IsCommandIdChecked(int command_id) const; > On 2011/06/23 18:14:03, sky wrote: >> >> OVERRIDE > > Done. > > http://codereview.chromium.org/6933037/ >
On 2011/06/23 20:19:41, sky wrote: > > > http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... > > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1050: for > > (std::vector<int>::iterator it = indices_affected.begin(); > > On 2011/06/23 18:14:03, sky wrote: > >> > >> If there is a deletion in progress then the indices from the model > > > > don't line up > >> > >> with the indices of the TabStrip. This is true of all the methods in > > > > this class > >> > >> though, so you might just want to add a TODO some where that it needs > > > > to be > >> > >> straightened out. > > > > Why is that happening? In TabStripModel::DetachTabContentsAt() we store > > the |old_model| after calling DecrementFrom. Also observers should get a > > TabClosingAt notification before getting notified of any > > TabSelectionChanged, right? > > There are two coordinate systems at play. There's the coordinates of > the model and the coordinates of the tabstrip. Normally they are the > same. The one exception to that is during a deletion. This happens > because the model is updated immediately, but the tabstrip animates > the deletion. During that animation the indices no longer line up. I > was wrong about the problem being prevalent here. You should use > GetTabAtAdjustForAnimation here and then it'll be good. > Done. > Also, TabStripGtk shouldn't need to override ActiveTabChanged. It > should be like you changed TabStrip::SetSelection. > > -Scott TabStripGtk implements directly the TabStripModelObserver, while TabStrip does not (even though it sais so in the class description). Instead, BrowserTabStripController implements TabStripModelObserver and calls TabStrip::SetSelection when a TabSelectionChanged is received. So in this case I dont think it is necessary to add a new method (who would call it?), I can directly use TabSelectionChanged, right? If that is the case, can you provide some help in moving the contents of TabStripGtk::ActiveTabChanged() within TabStripGtk::TabSelectionChanged()?
On 2011/06/24 16:01:24, dpapad wrote: > On 2011/06/23 20:19:41, sky wrote: > > > > > > http://codereview.chromium.org/6933037/diff/55001/chrome/browser/ui/gtk/tabs/... > > > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1050: for > > > (std::vector<int>::iterator it = indices_affected.begin(); > > > On 2011/06/23 18:14:03, sky wrote: > > >> > > >> If there is a deletion in progress then the indices from the model > > > > > > don't line up > > >> > > >> with the indices of the TabStrip. This is true of all the methods in > > > > > > this class > > >> > > >> though, so you might just want to add a TODO some where that it needs > > > > > > to be > > >> > > >> straightened out. > > > > > > Why is that happening? In TabStripModel::DetachTabContentsAt() we store > > > the |old_model| after calling DecrementFrom. Also observers should get a > > > TabClosingAt notification before getting notified of any > > > TabSelectionChanged, right? > > > > There are two coordinate systems at play. There's the coordinates of > > the model and the coordinates of the tabstrip. Normally they are the > > same. The one exception to that is during a deletion. This happens > > because the model is updated immediately, but the tabstrip animates > > the deletion. During that animation the indices no longer line up. I > > was wrong about the problem being prevalent here. You should use > > GetTabAtAdjustForAnimation here and then it'll be good. > > > Done. > > > Also, TabStripGtk shouldn't need to override ActiveTabChanged. It > > should be like you changed TabStrip::SetSelection. > > > > -Scott > > TabStripGtk implements directly the TabStripModelObserver, while TabStrip does > not (even though it sais so in the class description). Instead, > BrowserTabStripController implements TabStripModelObserver and calls > TabStrip::SetSelection when a TabSelectionChanged is received. So in this case I > dont think it is necessary to add a new method (who would call it?), I can > directly use TabSelectionChanged, right? > > If that is the case, can you provide some help in moving the contents of > TabStripGtk::ActiveTabChanged() within TabStripGtk::TabSelectionChanged()? Sorry for not being clear. What I'm saying is change TabStripGtk::TabSelectionChanged to match that of TabStrip::SetSelection and remove TabStripGtk::ActiveTabChanged. I believe you'll want something like: void TabStripGtk::TabSelectionChanged(const TabStripSelectionModel& old_model) { bool tiny_tabs = current_unselected_width_ != current_selected_width_; if (!IsAnimating() && (!needs_resize_layout_ || tiny_tabs)) Layout(); std::vector<int> indices_affected; std::insert_iterator<std::vector<int> > it(indices_affected, indices_affected.begin()); std::set_symmetric_difference( old_model.selected_indices().begin(), old_model.selected_indices().end(), model_->selection_model().selected_indices().begin(), model_->selection_model().selected_indices().end(), it); for (std::vector<int>::iterator it = indices_affected.begin(); it != indices_affected.end(); ++it) GetTabAtAdjustForAnimation(*it)->SchedulePaint(); TabStripSelectionModel::SelectedIndices no_longer_selected; std::insert_iterator<TabStripSelectionModel::SelectedIndices> it(no_longer_selected, no_longer_selected.begin()); std::set_difference(old_selection.selected_indices().begin(), old_selection.selected_indices().end(), new_selection.selected_indices().begin(), new_selection.selected_indices().end(), it); for (size_t i = 0; i < no_longer_selected.size(); ++i) { GetTabAtTabDataIndex(ModelIndexToTabIndex(no_longer_selected[i]))-> StopMiniTabTitleAnimation(); } }
http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1028: std::set_symmetric_difference( Done. I see one issue here. Since there is no longer "GetTabAt(index)->SchedulePaint();" and |index| will not end up necessarily in the |indices_affected| (for example: before: selected,not-active, now: selected and active), should it explicitly be repainted?
On Fri, Jun 24, 2011 at 1:17 PM, <dpapad@chromium.org> wrote: > > http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... > File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): > > http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1028: > std::set_symmetric_difference( > Done. > I see one issue here. Since there is no longer > "GetTabAt(index)->SchedulePaint();" > and |index| will not end up necessarily in the |indices_affected| (for > example: before: selected,not-active, now: selected and active), should > it explicitly be repainted? I think you're right. If the active_index changes you'll want to paint both the old/new active index. -Scott
On 2011/06/24 20:46:31, sky wrote: > On Fri, Jun 24, 2011 at 1:17 PM, <mailto:dpapad@chromium.org> wrote: > > > > > http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... > > File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): > > > > > http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... > > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1028: > > std::set_symmetric_difference( > > Done. > > I see one issue here. Since there is no longer > > "GetTabAt(index)->SchedulePaint();" > > and |index| will not end up necessarily in the |indices_affected| (for > > example: before: selected,not-active, now: selected and active), should > > it explicitly be repainted? > > I think you're right. If the active_index changes you'll want to paint > both the old/new active index. > > -Scott Should I do this within TabSelectionChanged? It seems that it is exactly what ActiveTabChanged was doing before removing it.
On 2011/06/24 22:12:04, dpapad wrote: > On 2011/06/24 20:46:31, sky wrote: > > On Fri, Jun 24, 2011 at 1:17 PM, <mailto:dpapad@chromium.org> wrote: > > > > > > > > > http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... > > > File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): > > > > > > > > > http://codereview.chromium.org/6933037/diff/62001/chrome/browser/ui/gtk/tabs/... > > > chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1028: > > > std::set_symmetric_difference( > > > Done. > > > I see one issue here. Since there is no longer > > > "GetTabAt(index)->SchedulePaint();" > > > and |index| will not end up necessarily in the |indices_affected| (for > > > example: before: selected,not-active, now: selected and active), should > > > it explicitly be repainted? > > > > I think you're right. If the active_index changes you'll want to paint > > both the old/new active index. > > > > -Scott > > Should I do this within TabSelectionChanged? It seems that it is exactly what > ActiveTabChanged was doing before removing it. True. But remember that we send both when the active changes, which would mean double the SchedulePaint (for the old/new active tabs). I can't imagine that is a big deal. So, I leave it to you to decide which to go with. -Scott
I chose to perform the task in TabSelectionChanged. It is always triggered when ActiveTabChanged triggers, so there should not be any problems because of this.
LGTM http://codereview.chromium.org/6933037/diff/66004/chrome/browser/ui/gtk/tabs/... File chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc (right): http://codereview.chromium.org/6933037/diff/66004/chrome/browser/ui/gtk/tabs/... chrome/browser/ui/gtk/tabs/tab_strip_gtk.cc:1025: GetTabAt(model_->active_index())->SchedulePaint(); I think you should verify this is >= 0 too. |
