The tab scroll buttons (tab_scroll_button_left and tab_scroll_button_right) are 7x10 px, but are placed into 16x16 frames. We should probably just make the images be 16x16, which should eliminate problems like Bug 355558 and Bug 355790. At the very least, the images should be an even number of pixels, so that they even *can* be centered. When we fix bug 355490, we should also make sure that its image is the right size for its frame.

I'd actually like to see this go the other way: I think we should remove the hard-coded things-called-margins-that-actually-aren't and use the actual image widths to determine the layout.

I should really probably sleep before submitting this, since I'm in no state to see if I'm doing anything insane. This patch: - Loses most of the hardcoded values (yay), and bases layout on the actual image sizes. As such, the scroll buttons look way thin. Button images with some border should fix that. - General cleanup/factoring/code-what-have-you. Sorry, I couldn't resist. - Moves the divider image that we draw to the left of the leftmost tab (with no overflow) to the right a little. It's always looked weird to me that that tab grows and shrinks when you select/deselect it, and this solution (drawing the divider on top of the left of the tab) makes it look much more consistent IMO.

Comment on attachment 261158 [details] [diff] [review] Patch v1 Really nice work froodian. As we mentioned on irc, I can jump in first and we'll rely on smorgan for the sr. Despite the late night coding, everything looks cool; r=me with a handful of considerations: > if (!mOverflowMenuButton) { > ... > + mOverflowMenuButton = [self newOverflowButtonForImageNamed:@"tab_menu_button"]; > [mOverflowMenuButton setAction:@selector(showOverflowMenu:)]; > - [(NSButtonCell *)[mOverflowMenuButton cell] sendActionOn:NSLeftMouseDownMask]; I think it was better to have the overflow menu to appear immediately, and not wait until mouse up. If agreed upon, we can add back the removed line as: [mOverflowMenuButton sendActionOn:NSLeftMouseDownMask]. > -(BOOL)tabIndexIsVisible:(int)index; > -(void)drawOverflowButtons; > +- (float)verticalOriginForButtonWithHeight:(float)height; [Style only] Extra space before "(float)vert...". This declaration spacing would be fine in just about every other file in the project, but BrowserTabBarView.mm's private category does not feature any padding between the instance/class method indicator and the return type. > +- (float)verticalOriginForButtonWithHeight:(float)height > +{ > + // buttons that don't necessarily take up the whole height are > + // drawn starting half-way up the available space. Err on the side > + // of being too high, since the bevel at the bottom makes anything > + // appear lower than it is > + return ([self tabBarHeight] - height) / 2 + 1; > +} Not a big deal, but this assumes that the bottom bevel height will always roughly equal 1. If, for some reason, we change/remove this bevel in the future, the buttons might look slightly off-centered (kTabBottomPad seems too large, not sure what that's for). > // Allocate the left scroll button and the right scroll button using a helper > -(void)initOverflow This comment wasn't added in this patch, but since we're refactoring a bit it might deserve a look. It is out of sync with the code, since |initOverflow| creates the menu button as well. Moreover, it doesn't offer any information that cannot be discerned by glancing at the code itself, and it hints at implementation detail ("using a helper"). We should either remove or change it. -- The tracking area for which these buttons respond to a mouse click consists only of the triangle itself (i.e. no grace rectangle around it). This can make them difficult to press since you have to precisely pinpoint the cursor directly over the triangle. Adding some border to the images will probably fix this, but we should probably keep an eye on it. I don't want to turn this bug into a general code cleanup one, but I can't help but mention that we might also want to #pragma mark and reorganize the method implementations to make this file easier to navigate. I found myself jumping all over the place while reviewing and often lost track of where I was. Possible groups might consist of the following: - Creation/Disposal - Drawing - Overflow - Actions - Events - Helpers (or utilities) - Dragging While we're visiting this file, there is an existing compiler warning which needs fixing: > BrowserTabBarView.mm: In function `void -[BrowserTabBarView showOverflowMenu:](BrowserTabBarView*, objc_selector*, objc_object*)': > BrowserTabBarView.mm:605: warning: `NSTabViewItem' may not respond to `-menuItem' A (BrowserTabViewItem*) cast is necessary: [overflowMenu addItem:[[mTabView tabViewItemAtIndex:i] menuItem]]; If these last two items should be left for other bugs, I don't mind taking care of it.

(In reply to comment #3) > I don't want to turn this bug into a general code cleanup one, but I can't help > but mention that we might also want to #pragma mark and reorganize the method > implementations to make this file easier to navigate. Substantial method reorganization should never be done with any behavioral changes. It makes reviewing much harder, and really screws up tools like blame.

No method reorg, it was very silly of me to even consider it. (In reply to comment #3) > I think it was better to have the overflow menu to appear immediately, and not > wait until mouse up. If agreed upon, we can add back the removed line as: > [mOverflowMenuButton sendActionOn:NSLeftMouseDownMask]. Yeah, good catch. > Not a big deal, but this assumes that the bottom bevel height will always > roughly equal 1. If, for some reason, we change/remove this bevel in the > future, the buttons might look slightly off-centered (kTabBottomPad seems too > large, not sure what that's for). OK, I've reorganized this logic some. Specifically, it assumes overflow buttons that are 18 px high (the 22px of the tab bar, - the 5px of kTabBottomPad). I did this because it allows us to have both the icon centered completely and have the button's click area take up the most space possible. This also does a little bit of tab size tweaking so that the overflow button on the right will stay the same size as you resize the window (ie tabs will always be of a width to perfectly fill the bar when the scroll buttons are showing) Another addition is that this also fixes bug 360854. I'm sorry, I promise I didn't mean to, I just noticed funky bits of code, and when I'd finished cleaning them up, I'd done it. What was happening was that what we set |mOverflowTabs| to relied on |tabsRect|, which in turn relied on |mOverflowTabs|. The solution is to use the width of the whole tab bar to decide whether or not we overflow. murph, this has significant enough changes that I'm r?ing you again, if you don't mind. Thanks! :)

Uploading new images of reasonable sizes.

Since I had to make a new one of these anyway (to be 18 px high) i used the chevron + 90 clockwise. This can be discussed/changed/whatever in the other bug - I still consider it a placeholder, but it's a slightly nicer placeholder than what we have now.

Comment on attachment 261430 [details] [diff] [review] Patch v2 >+ // whether we overflow is determined by the actual frame's width, not |tabsRect| This statement (and thus the change) is wrong. tabsRect is *always* smaller than the frame, so the frame width is irrelevant. While it looks like there is a circular dependency (I agree that code isn't very clear), there isn't, because overflow is always set to NO before the first call to tabsRect. I.e., we always see first if the tabs would fit without any overflow buttons.

(In reply to comment #9) > (From update of attachment 261430 [details] [diff] [review]) > >+ // whether we overflow is determined by the actual frame's width, not |tabsRect| > > This statement (and thus the change) is wrong. tabsRect is *always* smaller > than the frame, so the frame width is irrelevant. While it looks like there is > a circular dependency (I agree that code isn't very clear), there isn't, > because overflow is always set to NO before the first call to tabsRect. I.e., > we always see first if the tabs would fit without any overflow buttons. Yeah, you're right of course. I've removed the change and commented a bit more heavily to make it clear what's going on. I also took out a change I'd made in the previous version to make tab widths not round to integer values. (which I'd done essentially as a partial fix to bug 356119, though I didn't realize it). The solution is to actually fix bug 356119 in bug 356119.

Comment on attachment 261471 [details] [diff] [review] Patch v2.1 The frame rects where the tab scroll and menu buttons are now drawn seem perfectly calculated to me. I modified around the three images to contain simply a filled color and a stroked border to determine if they took up (and fit just inside) the entire area in which they're drawn. The ultimate responsibility of this bug is solved. >+ // store a 0-origined sizeless rect for invisible tabs. >+ NSRect overflowTabRect = NSMakeRect(nextTabXOrigin, 0, 0, 0); Would renaming the NSRect variable to something like |invisibleTabRect| disambiguate enough to eliminate the need for a comment describing its use? >+ // Order matters here - calculate widthOfATab with mOverflowTabs set to NO, >+ // so that we're really checking wether or not the tabs will fit without the overflows > mOverflowTabs = NO; >- float widthOfTabBar = NSWidth([self tabsRect]); >- float widthOfATab = widthOfTabBar / numberOfTabs; ... >+ else { // Overflow >+ // Order matters here - calculate widthOfTabBar after setting mOverflowTabs >+ // to YES, since that affects the available rect >+ mOverflowTabs = YES; >+ float widthOfTabBar = NSWidth([self tabsRect]); Hmm, I wish there was a way to indicate this dependency more obviously in the code itself. Statements that necessitate a specific order but keep this fact hidden in their implementation can be fragile. But, when there's no better choice, a comment will have to suffice. Again, since we have fixed the issue called out by this bug report, I'd say not to worry about changing this any further. > for (int i = 0; i < numberOfTabs; i++) { > TabButtonCell* tabButtonCell = [(BrowserTabViewItem*)[mTabView tabViewItemAtIndex:i] tabButtonCell]; > NSRect frameRect; > > // tabButtonCell is off-screen, to the left or right > if (i < mLeftMostVisibleTabIndex || i >= mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) { > frameRect = overflowTabRect; > [tabButtonCell hideCloseButton]; > [tabButtonCell setDrawDivider:NO]; > } > else { // Regular visible tab > frameRect = NSMakeRect(nextTabXOrigin, 0, widthOfATab, [self tabBarHeight]); > [tabButtonCell setDrawDivider:YES]; > nextTabXOrigin += (int)widthOfATab; > } > > [tabButtonCell setFrame:frameRect]; A change made from Patch v1 to this one, you introduced |frameRect| and temporarily store the pending frame and apply it on a single line later on. I actually preferred the prior technique of dropping the need for an additional variable and just doing a setFrame: directly inside each if-else block, as in Patch v1: > for (int i = 0; i < numberOfTabs; i++) { > TabButtonCell* tabButtonCell = [(BrowserTabViewItem*)[mTabView tabViewItemAtIndex:i] tabButtonCell]; > > // the tab at i is off-screen, to the left or right > if (i < mLeftMostVisibleTabIndex || i >= mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) { > // Use a sizeless rect to make these tabs invisible > [tabButtonCell setFrame:overflowTabRect]; > [tabButtonCell hideCloseButton]; > [tabButtonCell setDrawDivider:NO]; > } > else { > [tabButtonCell setFrame:NSMakeRect(nextTabXOrigin, 0, widthOfATab, [self tabBarHeight])]; > [tabButtonCell setDrawDivider:YES]; > nextTabXOrigin += (int)widthOfATab; > } > } If you feel the v2 approach is more clear and wanted to avoid two different setFrame: calls, I can live with that too ;) Again, nice job; r=me with the above comments.

I took out |frameRect| and renamed |overflowTabRect| to |invisibleTabRect|.

Comment on attachment 261706 [details] [diff] [review] Patch v2.2 Looks pretty good; some minor nits, and then a couple larger things since you volunteered for making |layoutButtons| easier to follow :D > if ([mTabView indexOfTabViewItem:[mTabView selectedTabViewItem]] != mLeftMostVisibleTabIndex) { ... >+ [mButtonDividerImage compositeToPoint:NSMakePoint(NSMinX(tabsRect), 0) > operation:NSCompositeSourceOver]; > } Fix the indentation on these lines. >- if (mBackgroundImage) return; >+ if (mBackgroundImage || mButtonDividerImage) >+ return; > > mBackgroundImage = [[NSImage imageNamed:@"tab_bar_bg"] retain]; > mButtonDividerImage = [[NSImage imageNamed:@"tab_button_divider"] retain]; This logic isn't quite right (granted it shouldn't happen that one is true and the other isn't, but if that were assumed then the |if| wouldn't need to change). It should be: if (!mBackgroundImage) mBackgroundImage = ... if (!mButtonDividerImage) mButtonDividerImage = ... > -(void)layoutButtons > { > int numberOfTabs = [mTabView numberOfTabViewItems]; >+ // Order matters here - calculate widthOfATab with mOverflowTabs set to NO, >+ // so that we're really checking wether or not the tabs will fit without the overflows > mOverflowTabs = NO; >+ float widthOfATab = NSWidth([self tabsRect]) / numberOfTabs; One possibility to avoid these comments would be to make a |tabsRectWithOverflow:(BOOL)overflowing|, make |tabsRect| call it with mOverflowTabs, but have this method use the longer version. Then mOverflowTabs can just be set once when we actually know the value. >+ // All the tabs will fit >+ if (widthOfATab >= kMinTabWidth) { Move this comment into the |if| block, since that's what it applies to > mLeftMostVisibleTabIndex = 0; > mNumberOfVisibleTabs = numberOfTabs; > widthOfATab = (widthOfATab > kMaxTabWidth ? kMaxTabWidth : widthOfATab); This line isn't necessary > [mOverflowLeftButton removeFromSuperview]; > [mOverflowRightButton removeFromSuperview]; > [mOverflowMenuButton removeFromSuperview]; The lack of parity between this block and the [self initOverflow] is cumbersome, and makes this less readable. How about making a |setOverflowButtonsVisible:(BOOL)visible| method that does the above or what |drawOverflowButtons| does as appropriate (which you can then use in both blocks here), renaming |initOverflow| to |ensureOveflowButtonsInitted| (since that's what it actually does), and have |setOverflowButtonsVisible:| call that as necessary (rather than the opposite, which is what is done now). (We can move all this and your changes to another bug if you'd like; it's just that if you are going to rewrite this method I'd rather it not have to happen twice). > } >+ else { // Overflow Comment on a new line for parity >+ [mOverflowLeftButton setPeriodicDelay:0.4 interval:0.15]; Make these constants at the top of the file, so we don't have magic numbers in multiple places.

Done and done.

Comment on attachment 264328 [details] [diff] [review] Patch v3 >+ [self setOverflowButtonsVisible:YES]; > } ... >+ [self setOverflowButtonsVisible:NO]; > } Pull these outside into a single [self setOverflowButtonsVisible:mOverflowTabs]; sr=smorgan with that change.

Checked in on trunk.