Several background pattern fixes on the Mac....

Several theming fixes for the Mac. Sorry for the extensive change, but they
were all sort of intertwined.

Fixes up patterns in general so that they are all in phase.
Moves the window widget buttons down by two pixels.
Draws overlays correctly.
Fixes up some accessibility issues with the default window widgets.
Gets rid of some out of date files (tab_cell).

BUG=18438, 18547, 19851, 20295, 22213, 23651, 24338
TEST=Launch Chrome. Switch to "dots" theme from the Google themes. Create a couple of tabs. Check to make sure that the background pattern line up with the tabs. Move the tabs around. Check that the hightlight colors and text colors look correct for all of the tabs. Make sure the patterns stay lined up. Resize the window, make sure none of the patterns move around. Create new windows by dragging the tabs out of the windows and make sure a new window is created with the correct pattern. Show the "find" bar. Make sure its pattern lines up correctly with the tabbar. Switch to default theme. Make sure it looks correct and draws properly. Switch to Zen theme and make sure that the overlay at the top draws correctly. Create a new window. make sure that the rollovers in the window widgets work correctly in both the active and inactive window. Mouse down on the zoom button in the inactive window and notice that the window context changes. Move off of the zoom button and mouse up. Mouse down on the miniaturize button on the inactive window and notice that the window context does not change. Move off of the miniaturize button and mouse up. Do the same thing you did for the miniaturize button for the close button. Start up Accessibility Inspector from the developer tools. Make sure that the window widgets report their accessibility information correctly.

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

[dmaclach, 2009-10-05 16:29:39]

Avi, nico, I don't have submitter status, so I'm going to need someone to submit
this for me as well.

[TVL, 2009-10-05 16:37:45]

drive by - what about extension shelf?

[Nico, 2009-10-05 16:42:03]

Thanks for taking this on! I'll leave the actual review to Avi, as I'm not very
proficient with the theming code, but the overall approach looks right to me.

A few high-level points:

* Can you describe the xib changes in english in the CL description?

* Is there a way to have pixel tests for themes? I.e. have a test theme where
every image has clearly defined top, bottom, left, and right (e.g. gradient from
green to red in y direction, blue to yellow in x direction, with some text on
it) loaded into a test browser instance with all the "stuff" (find bar, download
shelf, extension shelf, ...) visible. This themed browser window should then be
rendered into a bitmap and compared with a reference image which is checked in
(would have the additional benefit that it would make it easy to check how
themes work on other platforms -- just look at their reference theme picture). I
don't know if infrastructure for this exists, or how hard it is to do, but
themes seem to be an area that can regress quickly without us noticing.

* Does the theme on the findbar look good while it animates in?

http://codereview.chromium.org/260009/diff/1/13
File chrome/browser/cocoa/background_gradient_view.mm (right):

http://codereview.chromium.org/260009/diff/1/13#newcode56
Line 56: NSGradientDrawsAfterEndingLocation];
Indent looks off

http://codereview.chromium.org/260009/diff/1/7
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/260009/diff/1/7#newcode1424
Line 1424: @end
Don't some compilers complain about "missing newline" without the newline?

[Avi (use Gerrit), 2009-10-05 16:45:07]

http://codereview.chromium.org/260009/diff/1/3
File chrome/app/nibs/BrowserWindow.xib (left):

http://codereview.chromium.org/260009/diff/1/3#oldcode368
Line 368: <string
key="IBDocument.LastKnownRelativeProjectPath">../../../chrome.xcodeproj</string>
Why are we changing the project path? Can you describe what you did to the xibs?

http://codereview.chromium.org/260009/diff/1/13
File chrome/browser/cocoa/background_gradient_view.mm (right):

http://codereview.chromium.org/260009/diff/1/13#newcode33
Line 33: NSImage *backgroundImage
= at the end of the first line; match the existing style.

http://codereview.chromium.org/260009/diff/1/13#newcode50
Line 50: kToolbarMaxHeight)
Don't needlessly change indentation; this made sense since the continuation
lines were changing the second param.

http://codereview.chromium.org/260009/diff/1/13#newcode56
Line 56: NSGradientDrawsAfterEndingLocation];
Was correctly indented.

http://codereview.chromium.org/260009/diff/1/7
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/260009/diff/1/7#newcode853
Line 853: // Set up the tabstrip for our findbar so that it draws it's pattern
its != it's

http://codereview.chromium.org/260009/diff/1/7#newcode1236
Line 1236: // Set up our toolbar so that it draws it's background pattern in
phase
its

http://codereview.chromium.org/260009/diff/1/7#newcode1239
Line 1239: if([toolbarView respondsToSelector:@selector(setTabStripView:)]) {
I get the hinky vibe here, that there's a better way (though I don't know what
it is).

http://codereview.chromium.org/260009/diff/1/5
File chrome/browser/cocoa/find_bar_view.h (right):

http://codereview.chromium.org/260009/diff/1/5#newcode18
Line 18: TabStripView *tabStripView_;
A new strong dependency? We've had so many problems with leaks that I'm very
wary of them.

http://codereview.chromium.org/260009/diff/1/11
File chrome/browser/cocoa/tab_strip_view.mm (right):

http://codereview.chromium.org/260009/diff/1/11#newcode82
Line 82: = [self convertRect:tabStripViewWindowBounds
equals on the top line, indent 4 on the continuation.

http://codereview.chromium.org/260009/diff/1/16
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/260009/diff/1/16#newcode549
Line 549: [context saveGraphicsState];
I like the cleanup this allows.

http://codereview.chromium.org/260009/diff/1/16#newcode596
Line 596: CHECK([superView isKindOfClass:[TabStripView class]]);
DCHECK?

http://codereview.chromium.org/260009/diff/1/4
File chrome/browser/cocoa/toolbar_view.h (right):

http://codereview.chromium.org/260009/diff/1/4#newcode18
Line 18: TabStripView *tabStripView_;
And another strong reference. Same comment.

[Nico, 2009-10-05 16:45:22]

One more:
* Does this happen to fix
http://crbug.com/19851<detail?id=19851&q=os%3Amac%20zen&colspec=ID
Stars Pri Area Type Status
Summary Modified Owner Mstone> as well?

On Mon, Oct 5, 2009 at 9:42 AM, <thakis@chromium.org> wrote:

> Thanks for taking this on! I'll leave the actual review to Avi, as I'm not
> very
> proficient with the theming code, but the overall approach looks right to
> me.
>
> A few high-level points:
>
> * Can you describe the xib changes in english in the CL description?
>
> * Is there a way to have pixel tests for themes? I.e. have a test theme
> where
> every image has clearly defined top, bottom, left, and right (e.g. gradient
> from
> green to red in y direction, blue to yellow in x direction, with some text
> on
> it) loaded into a test browser instance with all the "stuff" (find bar,
> download
> shelf, extension shelf, ...) visible. This themed browser window should
> then be
> rendered into a bitmap and compared with a reference image which is checked
> in
> (would have the additional benefit that it would make it easy to check how
> themes work on other platforms -- just look at their reference theme
> picture). I
> don't know if infrastructure for this exists, or how hard it is to do, but
> themes seem to be an area that can regress quickly without us noticing.
>
> * Does the theme on the findbar look good while it animates in?
>
>
> http://codereview.chromium.org/260009/diff/1/13
> File chrome/browser/cocoa/background_gradient_view.mm (right):
>
> http://codereview.chromium.org/260009/diff/1/13#newcode56
> Line 56: NSGradientDrawsAfterEndingLocation];
> Indent looks off
>
> http://codereview.chromium.org/260009/diff/1/7
> File chrome/browser/cocoa/browser_window_controller.mm (right):
>
> http://codereview.chromium.org/260009/diff/1/7#newcode1424
> Line 1424: @end
> Don't some compilers complain about "missing newline" without the
> newline?
>
>
> http://codereview.chromium.org/260009
>

[TVL, 2009-10-05 16:50:54]

http://codereview.chromium.org/260009/diff/1/3
File chrome/app/nibs/BrowserWindow.xib (left):

http://codereview.chromium.org/260009/diff/1/3#oldcode368
Line 368: <string
key="IBDocument.LastKnownRelativeProjectPath">../../../chrome.xcodeproj</string>
On 2009/10/05 16:45:08, Avi wrote:
> Why are we changing the project path? Can you describe what you did to the
xibs?

He didn't.  No one has edited this since we moved the xibs out of an
English.lproj folder.

[rohitrao (ping after 24h), 2009-10-05 18:17:18]

I also dislike the findbarview->tabstripview and toolbarview->tabstripview
dependencies.  I remember thinking that each view independently figured out
where it was in the window and drew itself accordingly.  Why is that no longer
possible?

If we really need a single third-party to tell everyone what to do, can it be
the BWC instead?

[dmaclach, 2009-10-05 19:31:41]

TVL:

> drive by - what about extension shelf?
Haven't looked at it yet. It's on the list.

Nico:
> * Can you describe the xib changes in english in the CL description?

Done

> * Is there a way to have pixel tests for themes? I.e. have a test theme where
> every image has clearly defined top, bottom, left, and right (e.g. gradient
from
> green to red in y direction, blue to yellow in x direction, with some text on
> it) loaded into a test browser instance with all the "stuff" (find bar,
download
> shelf, extension shelf, ...) visible. This themed browser window should then
be
> rendered into a bitmap and compared with a reference image which is checked in
> (would have the additional benefit that it would make it easy to check how
> themes work on other platforms -- just look at their reference theme picture).
I
> don't know if infrastructure for this exists, or how hard it is to do, but
> themes seem to be an area that can regress quickly without us noticing.

I don't know if they are supported on the Mac yet? Avi? TVL? If not, I do have
some experience in this area and could look into implementing it.

> * Does the theme on the findbar look good while it animates in?
Yes

> Does this happen to fix
http://crbug.com/19851<detail?id=19851&q=os%3Amac%20zen&colspec=ID 
Surprisingly no. I was going to take a look at that next.

Rohit:
> I also dislike the findbarview->tabstripview and toolbarview->tabstripview
> dependencies.  I remember thinking that each view independently figured out
> where it was in the window and drew itself accordingly.  Why is that no longer
> possible?

It's no longer possible because to keep the patterns in phase the toolbar and
findbar need to know where the tabstrip is located in the window, and what it
has its pattern phase set to.

> If we really need a single third-party to tell everyone what to do, can it be
> the BWC instead?

I could do that. That would move some of the drawing logic out of the views
where I usually prefer to have it, but I can understand why that would be
possibly preferable here. Would we have the views have references to the
controller, or would we cast the controller that we get from [[self window]
windowController] to be a BrowserWindowController?

Cheers,
Dave

http://codereview.chromium.org/260009/diff/1/3
File chrome/app/nibs/BrowserWindow.xib (left):

http://codereview.chromium.org/260009/diff/1/3#oldcode368
Line 368: <string
key="IBDocument.LastKnownRelativeProjectPath">../../../chrome.xcodeproj</string>
On 2009/10/05 16:45:08, Avi wrote:
> Why are we changing the project path? Can you describe what you did to the
xibs?

Added description to the CL description.

http://codereview.chromium.org/260009/diff/1/13
File chrome/browser/cocoa/background_gradient_view.mm (right):

http://codereview.chromium.org/260009/diff/1/13#newcode33
Line 33: NSImage *backgroundImage
On 2009/10/05 16:45:08, Avi wrote:
> = at the end of the first line; match the existing style.

fixed

http://codereview.chromium.org/260009/diff/1/13#newcode50
Line 50: kToolbarMaxHeight)
On 2009/10/05 16:45:08, Avi wrote:
> Don't needlessly change indentation; this made sense since the continuation
> lines were changing the second param.
 
copy paste issue. fixed.

http://codereview.chromium.org/260009/diff/1/13#newcode56
Line 56: NSGradientDrawsAfterEndingLocation];
On 2009/10/05 16:45:08, Avi wrote:
> Was correctly indented.

Fixed

http://codereview.chromium.org/260009/diff/1/7
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/260009/diff/1/7#newcode853
Line 853: // Set up the tabstrip for our findbar so that it draws it's pattern
On 2009/10/05 16:45:08, Avi wrote:
> its != it's

Fixed

http://codereview.chromium.org/260009/diff/1/7#newcode1236
Line 1236: // Set up our toolbar so that it draws it's background pattern in
phase
On 2009/10/05 16:45:08, Avi wrote:
> its

Fixed

http://codereview.chromium.org/260009/diff/1/7#newcode1239
Line 1239: if([toolbarView respondsToSelector:@selector(setTabStripView:)]) {
On 2009/10/05 16:45:08, Avi wrote:
> I get the hinky vibe here, that there's a better way (though I don't know what
> it is).

open to suggestions. I didn't want to disturb things too much with my first real
CL ;-)

http://codereview.chromium.org/260009/diff/1/7#newcode1424
Line 1424: @end
On 2009/10/05 16:42:03, Nico wrote:
> Don't some compilers complain about "missing newline" without the newline?

Fixed

http://codereview.chromium.org/260009/diff/1/5
File chrome/browser/cocoa/find_bar_view.h (right):

http://codereview.chromium.org/260009/diff/1/5#newcode18
Line 18: TabStripView *tabStripView_;
On 2009/10/05 16:45:08, Avi wrote:
> A new strong dependency? We've had so many problems with leaks that I'm very
> wary of them.

Usually views have strong dependencies on one another. We could revamp this so
that the browser_window_controller owns the whole phase issue as suggested by
rohit.

http://codereview.chromium.org/260009/diff/1/11
File chrome/browser/cocoa/tab_strip_view.mm (right):

http://codereview.chromium.org/260009/diff/1/11#newcode82
Line 82: = [self convertRect:tabStripViewWindowBounds
On 2009/10/05 16:45:08, Avi wrote:
> equals on the top line, indent 4 on the continuation.

Fixed

http://codereview.chromium.org/260009/diff/1/16
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/260009/diff/1/16#newcode549
Line 549: [context saveGraphicsState];
On 2009/10/05 16:45:08, Avi wrote:
> I like the cleanup this allows.

thank you... and faster ;-)

http://codereview.chromium.org/260009/diff/1/16#newcode596
Line 596: CHECK([superView isKindOfClass:[TabStripView class]]);
On 2009/10/05 16:45:08, Avi wrote:
> DCHECK?

Fixed...either way an exception will be thrown.

[pink (ping after 24hrs), 2009-10-05 19:54:30]

dmac: can you talk to cole about moving the tab strip down by 2px? He probably
has some strong opinions there.

[dmac, 2009-10-05 20:13:59]

On 2009/10/05 19:54:30, pink wrote:
> dmac: can you talk to cole about moving the tab strip down by 2px? He probably
> has some strong opinions there.

Yes he did... I'll need to do some reworking.

[dmac, 2009-10-09 17:00:59]

Hey folks, I think I've dealt with all the issues that were brought up. Please
take a look. Changes have been quite... umm.. extensive to say the least. My
apologies.

[Nico, 2009-10-09 17:21:43]

Great stuff!

I'm having the impression that the pattern phase and theme changed notification
changes are somewhat independent and much less radical than the frame change and
the button moving. If that's true and if it's not too much of a hassle, it'd be
great if you could split this CL in two.

http://codereview.chromium.org/260009/diff/10001/10005
File chrome/browser/cocoa/background_gradient_view.mm (right):

http://codereview.chromium.org/260009/diff/10001/10005#newcode50
Line 50: kToolbarMaxHeight))
Does the style guide say that expressions spanning multiple lines should be
wrapped in parens?

http://codereview.chromium.org/260009/diff/10001/10014
File chrome/browser/cocoa/chrome_browser_window.mm (right):

http://codereview.chromium.org/260009/diff/10001/10014#newcode47
Line 47: [NSDistributedNotificationCenter defaultCenter];
indent 4

http://codereview.chromium.org/260009/diff/10001/10014#newcode80
Line 80: = [NSWindow standardWindowButton:NSWindowMiniaturizeButton
indent 4, move = to previous line

[dmac, 2009-10-09 18:32:51]

On 2009/10/09 17:21:43, Nico wrote:
> Great stuff!
> 
> I'm having the impression that the pattern phase and theme changed
notification
> changes are somewhat independent and much less radical than the frame change
and
> the button moving. If that's true and if it's not too much of a hassle, it'd
be
> great if you could split this CL in two.

Breaking it in two would be a bit of a pain right now (there is overlap in a
couple of files). I will try to make my future CLs far more concise and focused
on specific bugs.

> 
> http://codereview.chromium.org/260009/diff/10001/10005
> File chrome/browser/cocoa/background_gradient_view.mm (right):
> 
> http://codereview.chromium.org/260009/diff/10001/10005#newcode50
> Line 50: kToolbarMaxHeight))
> Does the style guide say that expressions spanning multiple lines should be
> wrapped in parens?

It doesn't say anything that I am aware of, but please educate me if I am wrong.
Fixed though.

> 
> http://codereview.chromium.org/260009/diff/10001/10014
> File chrome/browser/cocoa/chrome_browser_window.mm (right):
> 
> http://codereview.chromium.org/260009/diff/10001/10014#newcode47
> Line 47: [NSDistributedNotificationCenter defaultCenter];
> indent 4

Fixed.
 
> http://codereview.chromium.org/260009/diff/10001/10014#newcode80
> Line 80: = [NSWindow standardWindowButton:NSWindowMiniaturizeButton
> indent 4, move = to previous line

Fixed along with a couple of others.

[Avi (use Gerrit), 2009-10-09 19:10:14]

Much better. Lots of nits, a few major comments.

http://codereview.chromium.org/260009/diff/10001/10022
File chrome/browser/cocoa/browser_frame_view.h (right):

http://codereview.chromium.org/260009/diff/10001/10022#newcode11
Line 11: // This class is never to be instantiated on it's own.
its

http://codereview.chromium.org/260009/diff/10001/10022#newcode30
Line 30: // "C" precludes us from inserting a view between the system frame and
the
all the others are past tense; go with "precluded us"

http://codereview.chromium.org/260009/diff/10001/10022#newcode41
Line 41: // drawRect. It appears in the drawRect that NSGrayView (and it's
superclasses)
Rewrite for clarity: "It appears that NSGrayView's drawRect method (and that of
its superclasses) makes lots..."

http://codereview.chromium.org/260009/diff/10001/10022#newcode42
Line 42: // do lots of "isMemberOfClass/isKindOfClass" calls, and if your class
it NOT
"if your class IS not"

http://codereview.chromium.org/260009/diff/10001/10022#newcode43
Line 43: // an instance of NSGrayView exactly that the system drawing will not
work
nix the words "exactly that" to make the sentence flow better.

http://codereview.chromium.org/260009/diff/10001/10022#newcode46
Line 46: // Given all of the above, we found swizzling drawRect, and adding an
no reason to have a comma after drawRect here

http://codereview.chromium.org/260009/diff/10001/10011
File chrome/browser/cocoa/browser_frame_view.mm (right):

http://codereview.chromium.org/260009/diff/10001/10011#newcode32
Line 32: // need. If any of these fails it shouldn't affect the functionality of
the
make verb agree with subject. "any" is singular, so it should be "If any of
these fail it shouldn't..."

http://codereview.chromium.org/260009/diff/10001/10011#newcode106
Line 106: NSRectFill(rect);
You would think this clears the rect to be transparent. You'd be wrong.

NSRectFill uses NSCompositeCopy which ignores alpha. This will fill the rect
with black. You must use NSRectFillUsingOperation(rect, NSCompositeSourceOver).

(Found this yesterday in my own code and was going to post it to a blog entry,
but you prompted me to do so:
http://www.drissman.com/blog/archives/2009/10/09/nsrectfill_and_nscolor_clear...
)

http://codereview.chromium.org/260009/diff/10001/10002
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/260009/diff/10001/10002#newcode1019
Line 1019: NSNotificationCenter *defaultCenter = [NSNotificationCenter
defaultCenter];
star space, not space star when declaring pointers here and throughout the file.

http://codereview.chromium.org/260009/diff/10001/10002#newcode1484
Line 1484: = provider->GetNSImageNamed(IDR_THEME_FRAME_OVERLAY);
match previous definitions. equal sign goes on the previous line. (and match the
* as well)

http://codereview.chromium.org/260009/diff/10001/10025
File chrome/browser/cocoa/chrome_browser_window.h (right):

http://codereview.chromium.org/260009/diff/10001/10025#newcode67
Line 67: -(BOOL)_isTitleHidden;
If we control drawing due to our swizzles do we need this? Or is this fallback
in case our swizzling fails?

http://codereview.chromium.org/260009/diff/10001/10014
File chrome/browser/cocoa/chrome_browser_window.mm (right):

http://codereview.chromium.org/260009/diff/10001/10014#newcode38
Line 38: NSNotificationCenter *defaultCenter = [NSNotificationCenter
defaultCenter];
Same issue with stars: to the left of the space. Here and the rest of the file.

http://codereview.chromium.org/260009/diff/10001/10014#newcode57
Line 57: // Find all the "original" buttons, and hide them.
Why can't we reuse the originals rather than creating our own? An explanatory
comment might be helpful.

http://codereview.chromium.org/260009/diff/10001/10014#newcode70
Line 70: NSMakePoint(kChromeWindowButtonsOffsetFromLeft,
indent 4

http://codereview.chromium.org/260009/diff/10001/10014#newcode84
Line 84: NSMakePoint((NSMaxX(closeButtonFrame) +
indent 4

http://codereview.chromium.org/260009/diff/10001/10014#newcode97
Line 97: NSMakePoint((NSMaxX(miniaturizeButtonFrame) +
indent 4

http://codereview.chromium.org/260009/diff/10001/10014#newcode123
Line 123: - (id)accessibilityHitTest:(NSPoint)point {
accessibility.... niiiice!

[dmac, 2009-10-09 20:47:27]

On 2009/10/09 19:10:14, Avi wrote:
> Much better. Lots of nits, a few major comments.
> 
> http://codereview.chromium.org/260009/diff/10001/10022
> File chrome/browser/cocoa/browser_frame_view.h (right):
> 
> http://codereview.chromium.org/260009/diff/10001/10022#newcode11
> Line 11: // This class is never to be instantiated on it's own.
> its

Fixed

> http://codereview.chromium.org/260009/diff/10001/10022#newcode30
> Line 30: // "C" precludes us from inserting a view between the system frame
and
> the
> all the others are past tense; go with "precluded us"

Fixed
 
> http://codereview.chromium.org/260009/diff/10001/10022#newcode41
> Line 41: // drawRect. It appears in the drawRect that NSGrayView (and it's
> superclasses)
> Rewrite for clarity: "It appears that NSGrayView's drawRect method (and that
of
> its superclasses) makes lots..."

Fixed
  
> http://codereview.chromium.org/260009/diff/10001/10022#newcode42
> Line 42: // do lots of "isMemberOfClass/isKindOfClass" calls, and if your
class
> it NOT
> "if your class IS not"

Fixed
  
> http://codereview.chromium.org/260009/diff/10001/10022#newcode43
> Line 43: // an instance of NSGrayView exactly that the system drawing will not
> work
> nix the words "exactly that" to make the sentence flow better.

Fixed
  
> http://codereview.chromium.org/260009/diff/10001/10022#newcode46
> Line 46: // Given all of the above, we found swizzling drawRect, and adding an
> no reason to have a comma after drawRect here

Fixed
 
> http://codereview.chromium.org/260009/diff/10001/10011
> File chrome/browser/cocoa/browser_frame_view.mm (right):
> 
> http://codereview.chromium.org/260009/diff/10001/10011#newcode32
> Line 32: // need. If any of these fails it shouldn't affect the functionality
of
> the
> make verb agree with subject. "any" is singular, so it should be "If any of
> these fail it shouldn't..."

Fixed
 
> http://codereview.chromium.org/260009/diff/10001/10011#newcode106
> Line 106: NSRectFill(rect);
> You would think this clears the rect to be transparent. You'd be wrong.
> 
> NSRectFill uses NSCompositeCopy which ignores alpha. This will fill the rect
> with black. You must use NSRectFillUsingOperation(rect,
NSCompositeSourceOver).
> 
> (Found this yesterday in my own code and was going to post it to a blog entry,
> but you prompted me to do so:
>
http://www.drissman.com/blog/archives/2009/10/09/nsrectfill_and_nscolor_clear...
> )

Wrote up rebuttal and re-rebuttal on Avi's website... but fixed it here anyways.
 
> http://codereview.chromium.org/260009/diff/10001/10002
> File chrome/browser/cocoa/browser_window_controller.mm (right):
> 
> http://codereview.chromium.org/260009/diff/10001/10002#newcode1019
> Line 1019: NSNotificationCenter *defaultCenter = [NSNotificationCenter
> defaultCenter];
> star space, not space star when declaring pointers here and throughout the
file.

Fixed in several places in several files. Is this rule actually declared
somewhere? I honestly can't believe that our C++ standard allows char* a. What
do you do in the char* a, *b case? Sigh... puts head back down in trench.
 
> http://codereview.chromium.org/260009/diff/10001/10002#newcode1484
> Line 1484: = provider->GetNSImageNamed(IDR_THEME_FRAME_OVERLAY);
> match previous definitions. equal sign goes on the previous line. (and match
the
> * as well)

Fixed
 
> http://codereview.chromium.org/260009/diff/10001/10025
> File chrome/browser/cocoa/chrome_browser_window.h (right):
> 
> http://codereview.chromium.org/260009/diff/10001/10025#newcode67
> Line 67: -(BOOL)_isTitleHidden;
> If we control drawing due to our swizzles do we need this? Or is this fallback
> in case our swizzling fails?

Yes we need this when we fall back to default system drawing.

> http://codereview.chromium.org/260009/diff/10001/10014
> File chrome/browser/cocoa/chrome_browser_window.mm (right):
> 
> http://codereview.chromium.org/260009/diff/10001/10014#newcode38
> Line 38: NSNotificationCenter *defaultCenter = [NSNotificationCenter
> defaultCenter];
> Same issue with stars: to the left of the space. Here and the rest of the
file.

Fixed. Same rant as above ;-)
  
> http://codereview.chromium.org/260009/diff/10001/10014#newcode57
> Line 57: // Find all the "original" buttons, and hide them.
> Why can't we reuse the originals rather than creating our own? An explanatory
> comment might be helpful.

Fixed
 
> http://codereview.chromium.org/260009/diff/10001/10014#newcode70
> Line 70: NSMakePoint(kChromeWindowButtonsOffsetFromLeft,
> indent 4

Fixed
  
> http://codereview.chromium.org/260009/diff/10001/10014#newcode84
> Line 84: NSMakePoint((NSMaxX(closeButtonFrame) +
> indent 4

Fixed
 
> http://codereview.chromium.org/260009/diff/10001/10014#newcode97
> Line 97: NSMakePoint((NSMaxX(miniaturizeButtonFrame) +
> indent 4

Fixed
 
> http://codereview.chromium.org/260009/diff/10001/10014#newcode123
> Line 123: - (id)accessibilityHitTest:(NSPoint)point {
> accessibility.... niiiice!

Thank you sir.

Thanks for reading the comments too... I'll work on my grammar. It has become
rather sloppy in the past couple of years. You should see my handwriting ;-)

[Avi (use Gerrit), 2009-10-09 21:12:40]

LG