Updates to clean up default theme and add hover states

[John Grabowski, 2009-08-13 23:34:48]

Drive by comments

Also, no new unit tests anywhere?

http://codereview.chromium.org/165499/diff/89/1017
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/165499/diff/89/1017#newcode764
Line 764: - (void)mouseMoved:(NSEvent *)event {
Please use mouseExited: instead of mouseMoved:
mouseMoved: trivial to fool with normal use

http://codereview.chromium.org/165499/diff/89/1018
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/165499/diff/89/1018#newcode169
Line 169: - (void)mouseMoved:(NSEvent *)theEvent {
Cole, why don't you use mouseEntered: instead?
The use of mouseMoved: like this can trivially be fooled if you do focus
switching et al.

[Avi (use Gerrit), 2009-08-14 14:19:03]

Lots of style. Please:

- two spaces before //
- 80 columns

http://codereview.chromium.org/165499/diff/95/1032
File chrome/app/nibs/Toolbar.xib (right):

http://codereview.chromium.org/165499/diff/95/1032#newcode1
Line 1: <?xml version="1.0" encoding="UTF-8"?>
How does this Toolbar.xib relate to the one you privately mailed me with the
replacement graphics?

http://codereview.chromium.org/165499/diff/95/1038
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/165499/diff/95/1038#newcode327
Line 327: [tabStripController_ userChangedTheme];
Given that all the userChangedTheme on tabstripcontroller does is redraw, I
think we should rename it to applyTheme. That fits here better.

http://codereview.chromium.org/165499/diff/95/1045
File chrome/browser/cocoa/gradient_button_cell.h (right):

http://codereview.chromium.org/165499/diff/95/1045#newcode37
Line 37: CGFloat hoverStrength_; // How strong the mouse hover state is
Two spaces before the //; please make the comment more explanatory. What's a
hover state, and what does it mean for it to be "strong"?

http://codereview.chromium.org/165499/diff/95/1042
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/165499/diff/95/1042#newcode99
Line 99: [self setMouseInside:NO animate:YES];
indent

http://codereview.chromium.org/165499/diff/95/1042#newcode177
Line 177: [[NSColor colorWithCalibratedWhite:1.0 alpha:0.2 * hoverStrength]
setStroke];
80col

http://codereview.chromium.org/165499/diff/95/1042#newcode199
Line 199: NSColor* endColor = [NSColor colorWithCalibratedWhite:1.0 - 0.25 *
hoverStrength
80col

http://codereview.chromium.org/165499/diff/95/1042#newcode214
Line 214: if (showClickedGradient) stroke = [NSColor
colorWithCalibratedWhite:0.0 alpha:0.3];
Can never one-line an if.

http://codereview.chromium.org/165499/diff/95/1039
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/165499/diff/95/1039#newcode13
Line 13: namespace {
Please keep a separator line after the imports.

http://codereview.chromium.org/165499/diff/95/1040
File chrome/browser/cocoa/tab_cell.mm (right):

http://codereview.chromium.org/165499/diff/95/1040#newcode63
Line 63: fromRect:imageBounds operation:NSCompositeSourceOver fraction:1.0];
If you wrap at one method param, wrap at them all.

http://codereview.chromium.org/165499/diff/95/1034
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/165499/diff/95/1034#newcode77
Line 77: TabView *hoveredTab_; // weak. Current hover tab
need two spaces before //

http://codereview.chromium.org/165499/diff/95/1046
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/165499/diff/95/1046#newcode64
Line 64: NSTimeInterval thisUpdate = [NSDate timeIntervalSinceReferenceDate];
This calculation seems familiar. Can we abstract it out with the other one?

http://codereview.chromium.org/165499/diff/95/1037
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/165499/diff/95/1037#newcode170
Line 170: NSButton *targetView = (NSButton *)[[self view] hitTest:[theEvent
locationInWindow]];
80col

[Avi (use Gerrit), 2009-08-17 16:40:36]

In general, LG with issues:

- Please obey the style guide. Specifically, please match your code to the
surroundings.
- How does the Toolbar.xib relate to the one you personally mailed me to fix
some positioning issues?
- Can you please (when altering xibs) provide a brief description of the changes
you made? If they're simple I can grok them from the diff, but these diffs are
so different it's hard to tell.

http://codereview.chromium.org/165499/diff/3038/2019
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/165499/diff/3038/2019#newcode142
Line 142: //  if ([self isEnabled] && !showClickedGradient) {
don't check in commented-out code

http://codereview.chromium.org/165499/diff/3038/2016
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/165499/diff/3038/2016#newcode53
Line 53: NSWindow *parent_;
stars to the left (here and throughout the file). please match existing code.

http://codereview.chromium.org/165499/diff/3038/2017
File chrome/browser/cocoa/tab_cell.mm (right):

http://codereview.chromium.org/165499/diff/3038/2017#newcode55
Line 55: 
blank line?

http://codereview.chromium.org/165499/diff/3038/2017#newcode57
Line 57: [[self title] drawInRect:[self titleRectForBounds:cellFrame]
indent

http://codereview.chromium.org/165499/diff/3038/2023
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/165499/diff/3038/2023#newcode515
Line 515: //        addClip];
don't comment out; take it out and remove the comment too

http://codereview.chromium.org/165499/diff/3038/2014
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/165499/diff/3038/2014#newcode169
Line 169: - (void)mouseMoved:(NSEvent *)theEvent {
please match the * style of the rest of the file.

[Avi (use Gerrit), 2009-08-20 18:35:37]

Some comments from running with this patch:

- When unthemed, the status bubble looks hideous.
- The specular highlight tracking the mouse is cool.
- Alignment is better but still not perfect.

We need to fix the status bubble, but otherwise I think we could land this and
iterate.

http://codereview.chromium.org/165499/diff/4001/5012
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/165499/diff/4001/5012#newcode145
Line 145: //  }
Is there some reason why you're keeping the commented-out code?

http://codereview.chromium.org/165499/diff/4001/5009
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/165499/diff/4001/5009#newcode272
Line 272: - (void)setParent:(NSWindow *)parent {
plzmatch*stylekthxbye

http://codereview.chromium.org/165499/diff/4001/5009#newcode278
Line 278: - (GTMTheme *) gtm_theme {
ditto

http://codereview.chromium.org/165499/diff/4001/5013
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/165499/diff/4001/5013#newcode12
Line 12: 
Too much empty space

http://codereview.chromium.org/165499/diff/4001/5016
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/165499/diff/4001/5016#newcode521
Line 521: //        addClip];
Please don't leave commented-out code. Either remove it or have a good comment
as to why you're leaving it commented out but checked-in.

[Avi (use Gerrit), 2009-08-20 18:43:24]

Have you tried the "Zen Spring" theme? Looks troublesome. Pink filed a bug on
the background image (I'll look) but the text color is wrong too.

[Avi (use Gerrit), 2009-08-21 15:38:06]

On 2009/08/20 18:43:24, Avi wrote:
> Have you tried the "Zen Spring" theme? Looks troublesome. Pink filed a bug on
> the background image (I'll look) but the text color is wrong too.

Seeing painting issues in tabs when resizing the window.

[Nicholas Jitkoff, 2009-08-24 22:00:20]

Ok. Pushed an updated CL.

I added more unit tests for the methods I added to the status bar and  
code to test the hover states in gradient_button_cell and tab_view.



On Aug 21, 2009, at 8:38 AM, avi@chromium.org wrote:

> On 2009/08/20 18:43:24, Avi wrote:
>> Have you tried the "Zen Spring" theme? Looks troublesome. Pink  
>> filed a
> bug on
>> the background image (I'll look) but the text color is wrong too.
>
> Seeing painting issues in tabs when resizing the window.
>
> http://codereview.chromium.org/165499

[John Grabowski, 2009-08-24 22:39:51]

Much better; thanks Cole.  Thx especially for adding unit tests.
A few more things, mostly related to Google/Chromium style.

http://codereview.chromium.org/165499/diff/4057/4072
File chrome/browser/cocoa/gradient_button_cell.h (right):

http://codereview.chromium.org/165499/diff/4057/4072#newcode59
Line 59: - (void)setMouseInside:(BOOL)flag animate:(BOOL)animate;
The use seems obvious by the name.  But... the code appears to require this gets
called repeatedly so that alpha is adjusted, which may not be expected from an
otherwise boolean "flag".  Doc it?

http://codereview.chromium.org/165499/diff/4057/4069
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/165499/diff/4057/4069#newcode18
Line 18: @implementation GradientButtonCell
blank line after @impl

http://codereview.chromium.org/165499/diff/4057/4069#newcode47
Line 47: - (void)setMouseInside:(BOOL)flag animate:(BOOL)animated {
Unit test
- call setMouseInside:YES, then confirm hoverAlpha is 1.0 
- add adjustHoverValueWithTime:(NSTimeInterval)now { /* meat */ }
- change adjustHoverValue to { adjustHoverValueWithTime:[NSDate .. ]; }
- call adjustHoverValueWithTime with various values to make sure hoverAlpha
decreases

http://codereview.chromium.org/165499/diff/4057/4069#newcode54
Line 54: [self setHoverAlpha:flag];
No guarantee BOOL flag is equal to CGFloat 1.0

http://codereview.chromium.org/165499/diff/4057/4067
File chrome/browser/cocoa/tab_cell.mm (right):

http://codereview.chromium.org/165499/diff/4057/4067#newcode41
Line 41: GTMTheme *theme = [[self controlView] gtm_theme];
Foo *var --> Foo* var (2 spots in this function)

http://codereview.chromium.org/165499/diff/4057/4067#newcode55
Line 55: 
extra blank line

http://codereview.chromium.org/165499/diff/4057/4061
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/165499/diff/4057/4061#newcode77
Line 77: TabView *hoveredTab_; // weak. Tab that the mouse is hovering over
TabView *  --> TabView*
And 2 spaces between ; and //

http://codereview.chromium.org/165499/diff/4057/4063
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/165499/diff/4057/4063#newcode784
Line 784: TabView *targetView = (TabView *)[tabView_ hitTest:[event
locationInWindow]];
TabView * --> TabView*

http://codereview.chromium.org/165499/diff/4057/4060
File chrome/browser/cocoa/tab_view.h (right):

http://codereview.chromium.org/165499/diff/4057/4060#newcode35
Line 35: BOOL isMouseInside_; // Is the mouse hovering over?
; <2 spaces> //
not 1
(many spots here)
Also end comments with . to make a complete sentence (google style)

http://codereview.chromium.org/165499/diff/4057/4071
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/165499/diff/4057/4071#newcode65
Line 65: // Tracking area for mouseover.
Be more explicit that tracking area is currently only the close button, NOT the
whole window, else you might get alarm that we're calling setNeedsDisplay:YES
for every mouse moved which is otherwise brutal.

http://codereview.chromium.org/165499/diff/4057/4071#newcode67
Line 67: NSButton *hoveredButton_; // weak. Current hover button
" *"  --> "* "
Also "." at end

http://codereview.chromium.org/165499/diff/4057/4064
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/165499/diff/4057/4064#newcode153
Line 153: trackingArea_.reset(
If the tracking area is always NSZeroRect, why is this file changed at all?  Or
is there a [[tbcontroller view] trackingAreas] call I missed?

[pink (ping after 24hrs), 2009-08-25 15:15:18]

LGTM with nits applied.

http://codereview.chromium.org/165499/diff/4082/4091
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/165499/diff/4082/4091#newcode13
Line 13: #import "third_party/GTM/AppKit/GTMNSColor+Luminance.h"
alphabetize

http://codereview.chromium.org/165499/diff/4082/4091#newcode54
Line 54: NSWindow *parent_;
NSWindow*, not NSWindow *

http://codereview.chromium.org/165499/diff/4082/4091#newcode59
Line 59: - (void)setParent:(NSWindow *)parent;
Same here

http://codereview.chromium.org/165499/diff/4082/4091#newcode278
Line 278: - (GTMTheme*) gtm_theme {
no space after close paren

http://codereview.chromium.org/165499/diff/4082/4092
File chrome/browser/cocoa/tab_cell.mm (right):

http://codereview.chromium.org/165499/diff/4082/4092#newcode45
Line 45: NSShadow* textShadow = [[[NSShadow alloc] init] autorelease];
use scoped_nsobject

http://codereview.chromium.org/165499/diff/4082/4092#newcode66
Line 66: 
omit blank line

http://codereview.chromium.org/165499/diff/4082/4099
File chrome/browser/cocoa/tab_controller.h (right):

http://codereview.chromium.org/165499/diff/4082/4099#newcode78
Line 78: // (Re)apply the current theme
period at end of sentence.

http://codereview.chromium.org/165499/diff/4082/4095
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/165499/diff/4082/4095#newcode170
Line 170: GTMTheme *theme = [[self view] gtm_theme];
GTMTheme*

http://codereview.chromium.org/165499/diff/4082/4095#newcode171
Line 171: NSColor *color = nil;
NSColor*

http://codereview.chromium.org/165499/diff/4082/4095#newcode176
Line 176: // Default to the selected text color unless told otherwise
period at end of sentence.

http://codereview.chromium.org/165499/diff/4082/4086
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/165499/diff/4082/4086#newcode75
Line 75: // when the mouse moves in the tab strip
period at end of sentence

http://codereview.chromium.org/165499/diff/4082/4088
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/165499/diff/4082/4088#newcode103
Line 103: initWithRect:NSZeroRect // Ignored by NSTrackingInVisibleRect
2 spaces before // on same-line comment

http://codereview.chromium.org/165499/diff/4082/4088#newcode784
Line 784: TabView* targetView = (TabView*)[tabView_ hitTest:[event
locationInWindow]];
all this needs a comment

http://codereview.chromium.org/165499/diff/4082/4088#newcode794
Line 794: [hoveredTab_ mouseExited:nil]; // We don't pass event because moved
events
2 spaces before //

http://codereview.chromium.org/165499/diff/4082/4088#newcode795
Line 795: [targetView mouseEntered:nil]; // don't have valid tracking areas
2 spaces before //

http://codereview.chromium.org/165499/diff/4082/4098
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/165499/diff/4082/4098#newcode520
Line 520: NSPoint p = [self patternPhase];
call this |phase| or something, no need to skimp on letters for variable names

http://codereview.chromium.org/165499/diff/4082/4098#newcode564
Line 564: NSPoint p1 = hoverPoint_;
is there a better name than p1?

http://codereview.chromium.org/165499/diff/4082/4098#newcode578
Line 578: 
omit blank line

http://codereview.chromium.org/165499/diff/4082/4098#newcode590
Line 590: // Draw the top stroke
period at end of sentence

http://codereview.chromium.org/165499/diff/4082/4096
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/165499/diff/4082/4096#newcode67
Line 67: NSButton *hoveredButton_;  // weak. Button under the mouse cursor.
NSButton*

[Avi (use Gerrit), 2009-08-26 20:51:27]

During this patch the TabControllerTest starts falling over dead halfway through
the run:

http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Mac%20(valgr...

Given that this patch touches that code, can you take a look?

[Avi (use Gerrit), 2009-08-26 20:52:36]

TestResizeViews fails too:

http://build.chromium.org/buildbot/waterfall/builders/Mac10.5%20Tests/builds/...

Reverting is likely.

[Nicholas Jitkoff, 2009-08-26 20:53:56]

This is all due to the 2 px shorter toolbar. We must need to rewrite  
all the expected metrics.


On Aug 26, 2009, at 1:52 PM, avi@chromium.org wrote:

> TestResizeViews fails too:
>
>
http://build.chromium.org/buildbot/waterfall/builders/Mac10.5%20Tests/builds/...
>
> Reverting is likely.
>
> http://codereview.chromium.org/165499

[Avi (use Gerrit), 2009-08-26 20:57:13]

Reverted.

[Avi (use Gerrit), 2009-08-26 21:07:26]

On 2009/08/26 20:53:56, Nicholas Jitkoff wrote:
> This is all due to the 2 px shorter toolbar. We must need to rewrite  
> all the expected metrics.

OK.

What's far more worrying is that the TabControllerTest on lots of the trybot
runs was dying halfway through. Sometimes it was on the tooltip tests, and
during commit it died on UserSelection (split so that Rieltveld doesn't munge
it):

http://build.chromium.org/buildbot/waterfall/bui
lders/Chromium%20Mac%20(valgrind)/builds/1269/st
eps/valgrind%20test%3A%20unit/logs/stdio

Something's seriously wrong for the test to die; we need to find it.

[pink (ping after 24hrs), 2009-08-26 21:19:35]

I have a fix for the tab dragging not being able to merge tabs back into windows
(it's in tab_view.mm).

Also, mousing over bg tabs takes about 25% CPU. It's cool, but wasteful.

[Avi (use Gerrit), 2009-08-27 23:06:59]

As of patchset 28 we're still failing the toolbar_controller_unittest. You said
you wanted to pull the pixel specs in there. Do you have an updated file?

[pink (ping after 24hrs), 2009-08-27 23:48:54]

That's another test that we should either disable or delete as it's
more hard-coded values. We just didn't get a chance to do that change
before Cole left for the day. That should be it, however.

On Thu, Aug 27, 2009 at 4:06 PM, <avi@chromium.org> wrote:
> As of patchset 28 we're still failing the toolbar_controller_unittest.
> You said you wanted to pull the pixel specs in there. Do you have an
> updated file?
>
> http://codereview.chromium.org/165499
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Avi (use Gerrit), 2009-08-28 00:44:19]

Clear failure in UI tests:
http://build.chromium.org/buildbot/waterfall/builders/Mac10.5%20Tests/builds/...

TestLifetimeOfDomAutomationController

Reverting.