[Mac] Window titles for Expose.

[Mac] Window titles for Expose.

http://crbug.com/18854
TEST=Titles on mouse-over in Expose.  No extra title in titlebar.

[Scott Hess - ex-Googler, 2009-10-02 21:04:40]

Last night was noodling with faking events, and feeling frisky, I looked at
this.  On the up side, it works.  On the flip side ... ick.  When someone
suggests this kind of stuff, should we have some sort of committee which says
"Yea, verily, we should go there"?

[TVL, 2009-10-02 21:08:27]

I think you only want to replace the cell for non popup windows.  popups should
still show a title.

[TVL, 2009-10-02 21:11:19]

http://codereview.chromium.org/259023/diff/1/4
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/259023/diff/1/4#newcode109
Line 109: [NSApp changeWindowsItem:window_ title:newTitle filename:NO];
this and the one below can probably come out since we'll be setting the titles
now.

[pink (ping after 24hrs), 2009-10-02 21:13:12]

I don't think this is really that skanky. I don't have any problem with the
approach. Good find!

[Mark Mentovai, 2009-10-02 21:15:27]

http://codereview.chromium.org/259023/diff/1/3
File chrome/browser/cocoa/browser_window.mm (right):

http://codereview.chromium.org/259023/diff/1/3#newcode52
Line 52: - (id)_customTitleCell {
This isn't really so bad.

[Scott Hess - ex-Googler, 2009-10-02 22:46:32]

My prior hack was 23% less l33t than it could have been.  Sigh.

In modifying things to not suppress for popups, I found myself with a dilemma. 
Nobody except the nib file knows this window as CocoaBrowserWindow.  I suspect
this is unintentional, and am certainly open to re-plumbing things, but it
seemed reasonable to check if that is even worthwhile or desirable.  I put the
popup check in browser_window_controller.mm because that feels like the place
where "Things which change the layout" belong, though it could as easily have
gone into browser_window_cocoa.mm.

http://codereview.chromium.org/259023/diff/1/4
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/259023/diff/1/4#newcode109
Line 109: [NSApp changeWindowsItem:window_ title:newTitle filename:NO];
On 2009/10/02 21:11:19, TVL wrote:
> this and the one below can probably come out since we'll be setting the titles
> now.

Done.

[John Grabowski, 2009-10-02 23:18:10]

LGTM with minor things addressed.

I <3 unit tests but even I think this is minor enough to not need one.  (What,
did I really said that?)

Perhaps you can add one anyway just to be super-hardcore and peer pressure the
weak?

http://codereview.chromium.org/259023/diff/5006/15
File chrome/browser/cocoa/browser_window.h (right):

http://codereview.chromium.org/259023/diff/5006/15#newcode15
Line 15: BOOL shouldHideTitle_;
Brief comment explaining when this will be true or false (e.g. only false on
pop-ups).

http://codereview.chromium.org/259023/diff/5006/16
File chrome/browser/cocoa/browser_window.mm (right):

http://codereview.chromium.org/259023/diff/5006/16#newcode42
Line 42: // Undocumented method to control whether the title cell is drawn.
Explain this is an undocumented method of Cocoa, not yours, and it's behavior
(e.g no draw but yes dock et al)

http://codereview.chromium.org/259023/diff/5006/18
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/259023/diff/5006/18#newcode1229
Line 1229: if ([window_ respondsToSelector:@selector(setShouldHideTitle:)]) {
Why not make window_ a BrowserWindow* so you don't have to respondsToSelector:?

[Scott Hess - ex-Googler, 2009-10-02 23:23:15]

http://codereview.chromium.org/259023/diff/5006/18
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/259023/diff/5006/18#newcode1229
Line 1229: if ([window_ respondsToSelector:@selector(setShouldHideTitle:)]) {
On 2009/10/02 23:18:10, John Grabowski wrote:
> Why not make window_ a BrowserWindow* so you don't have to
respondsToSelector:?

If nobody counter-argues that this isn't always a CocoaBrowserWindow, I'll put
together a CL on Monday to refactor things.  Calling it "refactor" because while
I'm in there I'll rename browser/cocoa/browser_window.h on the off chance that
it's confusing vs browser/browser_window.h (and also vs containing
CocoaBrowserWindow class).

[Nico, 2009-10-04 03:47:11]

Sweet! How did you find this?

[Scott Hess - ex-Googler, 2009-10-05 20:56:48]

On 2009/10/04 03:47:11, Nico wrote:
> Sweet! How did you find this?

NSObjCMessageLoggingEnabled=YES plus class-dump.  I've probably been using
class-dump longer than some Chromium committers have been alive :-).

[Scott Hess - ex-Googler, 2009-10-06 18:16:16]

Fear my unit tests!

http://codereview.chromium.org/259023/diff/5006/15
File chrome/browser/cocoa/browser_window.h (right):

http://codereview.chromium.org/259023/diff/5006/15#newcode15
Line 15: BOOL shouldHideTitle_;
On 2009/10/02 23:18:10, John Grabowski wrote:
> Brief comment explaining when this will be true or false (e.g. only false on
> pop-ups).

Is that kind of comment relevant to this code?  The only caller for
-setShouldHideTitle: indicates why it wants to suppress the title.

http://codereview.chromium.org/259023/diff/5006/16
File chrome/browser/cocoa/browser_window.mm (right):

http://codereview.chromium.org/259023/diff/5006/16#newcode42
Line 42: // Undocumented method to control whether the title cell is drawn.
On 2009/10/02 23:18:10, John Grabowski wrote:
> Explain this is an undocumented method of Cocoa, not yours, and it's behavior
> (e.g no draw but yes dock et al)

As part of writing the unit test, I exposed this method out in the interface
file, and threw in an undocumented-Cocoa-API comment there.

http://codereview.chromium.org/259023/diff/5006/18
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/259023/diff/5006/18#newcode1229
Line 1229: if ([window_ respondsToSelector:@selector(setShouldHideTitle:)]) {
On 2009/10/02 23:23:15, shess wrote:
> On 2009/10/02 23:18:10, John Grabowski wrote:
> > Why not make window_ a BrowserWindow* so you don't have to
> respondsToSelector:?
> 
> If nobody counter-argues that this isn't always a CocoaBrowserWindow, I'll put
> together a CL on Monday to refactor things.  Calling it "refactor" because
while
> I'm in there I'll rename browser/cocoa/browser_window.h on the off chance that
> it's confusing vs browser/browser_window.h (and also vs containing
> CocoaBrowserWindow class).

Done.

[John Grabowski, 2009-10-06 18:24:41]

I DO fear your unit tests!  But I like to be scared so it's fine.

Still LGTM but please reuse test util classes to reduce boilerplate in your
test.

http://codereview.chromium.org/259023/diff/1019/1027
File chrome/browser/cocoa/chrome_browser_window_unittest.mm (right):

http://codereview.chromium.org/259023/diff/1019/1027#newcode31
Line 31: class ChromeBrowserWindowTest : public PlatformTest {
Use CocoaTestHelper and BrowserTestHelper to take care a lot of the boilerplate
done here (e.g. create NSApplication, create window, etc).  See
bookmark_bar_controller_unittest.mm for example use.

[Scott Hess - ex-Googler, 2009-10-06 18:59:44]

http://codereview.chromium.org/259023/diff/1019/1027
File chrome/browser/cocoa/chrome_browser_window_unittest.mm (right):

http://codereview.chromium.org/259023/diff/1019/1027#newcode31
Line 31: class ChromeBrowserWindowTest : public PlatformTest {
On 2009/10/06 18:24:41, John Grabowski wrote:
> Use CocoaTestHelper and BrowserTestHelper to take care a lot of the
boilerplate
> done here (e.g. create NSApplication, create window, etc).  See
> bookmark_bar_controller_unittest.mm for example use.

Since I'm testing a custom window, I was aiming to avoid the default window. 
One solution would be to have both the default and custom window ... another
solution would be to refactor the window creation out and pass it into the
constructor.  WDYT?

[John Grabowski, 2009-10-06 19:06:22]

Hmm.  Not sure you can pass in a custom window
since CocoaTestHelper calls [NSApplication sharedApplication] and you
shouldn't create NSWindows before that.
So I think a 2nd constructor which lets you specify "no default
window" seems best.

jrg


On Tue, Oct 6, 2009 at 11:59 AM, <shess@chromium.org> wrote:

>
> http://codereview.chromium.org/259023/diff/1019/1027
> File chrome/browser/cocoa/chrome_browser_window_unittest.mm (right):
>
> http://codereview.chromium.org/259023/diff/1019/1027#newcode31
> Line 31: class ChromeBrowserWindowTest : public PlatformTest {
> On 2009/10/06 18:24:41, John Grabowski wrote:
>
>> Use CocoaTestHelper and BrowserTestHelper to take care a lot of the
>>
> boilerplate
>
>> done here (e.g. create NSApplication, create window, etc).  See
>> bookmark_bar_controller_unittest.mm for example use.
>>
>
> Since I'm testing a custom window, I was aiming to avoid the default
> window.  One solution would be to have both the default and custom
> window ... another solution would be to refactor the window creation out
> and pass it into the constructor.  WDYT?
>
>
> http://codereview.chromium.org/259023
>

[Scott Hess - ex-Googler, 2009-10-06 19:21:36]

How about a CocoaNoWindowTestHelper class which pulls up the shared
constructor, which CocoaTestHelper inherits from and augments?

On Tue, Oct 6, 2009 at 12:05 PM, John Grabowski <jrg@chromium.org> wrote:
> Hmm. =A0Not sure you can pass in a custom window
> since=A0CocoaTestHelper=A0calls=A0[NSApplication=A0sharedApplication]=A0a=
nd=A0you=A0shouldn't=A0create=A0NSWindows=A0before=A0that.
> So=A0I=A0think=A0a=A02nd=A0constructor=A0which=A0lets=A0you=A0specify=A0"=
no=A0default=A0window"=A0seems=A0best.
> jrg
>
> On Tue, Oct 6, 2009 at 11:59 AM, <shess@chromium.org> wrote:
>>
>> http://codereview.chromium.org/259023/diff/1019/1027
>> File chrome/browser/cocoa/chrome_browser_window_unittest.mm (right):
>>
>> http://codereview.chromium.org/259023/diff/1019/1027#newcode31
>> Line 31: class ChromeBrowserWindowTest : public PlatformTest {
>> On 2009/10/06 18:24:41, John Grabowski wrote:
>>>
>>> Use CocoaTestHelper and BrowserTestHelper to take care a lot of the
>>
>> boilerplate
>>>
>>> done here (e.g. create NSApplication, create window, etc). =A0See
>>> bookmark_bar_controller_unittest.mm for example use.
>>
>> Since I'm testing a custom window, I was aiming to avoid the default
>> window. =A0One solution would be to have both the default and custom
>> window ... another solution would be to refactor the window creation out
>> and pass it into the constructor. =A0WDYT?
>>
>> http://codereview.chromium.org/259023
>
>

[John Grabowski, 2009-10-06 19:46:12]

SGTM

On Tue, Oct 6, 2009 at 12:15 PM, Scott Hess <shess@chromium.org> wrote:

> How about a CocoaNoWindowTestHelper class which pulls up the shared
> constructor, which CocoaTestHelper inherits from and augments?
>
> On Tue, Oct 6, 2009 at 12:05 PM, John Grabowski <jrg@chromium.org> wrote:
> > Hmm.  Not sure you can pass in a custom window
> >
> since CocoaTestHelper calls [NSApplication sharedApplication] and you
shouldn't create NSWindows before that.
> >
> So I think a 2nd constructor which lets you specify "no default window" seems
best.
> > jrg
> >
> > On Tue, Oct 6, 2009 at 11:59 AM, <shess@chromium.org> wrote:
> >>
> >> http://codereview.chromium.org/259023/diff/1019/1027
> >> File chrome/browser/cocoa/chrome_browser_window_unittest.mm (right):
> >>
> >> http://codereview.chromium.org/259023/diff/1019/1027#newcode31
> >> Line 31: class ChromeBrowserWindowTest : public PlatformTest {
> >> On 2009/10/06 18:24:41, John Grabowski wrote:
> >>>
> >>> Use CocoaTestHelper and BrowserTestHelper to take care a lot of the
> >>
> >> boilerplate
> >>>
> >>> done here (e.g. create NSApplication, create window, etc).  See
> >>> bookmark_bar_controller_unittest.mm for example use.
> >>
> >> Since I'm testing a custom window, I was aiming to avoid the default
> >> window.  One solution would be to have both the default and custom
> >> window ... another solution would be to refactor the window creation out
> >> and pass it into the constructor.  WDYT?
> >>
> >> http://codereview.chromium.org/259023
> >
> >
>

[Scott Hess - ex-Googler, 2009-10-07 17:59:14]

OK, I pulled the code out where I could share it.  I tried to make it even
easier, per the TODO, but I found issues with exposing a subclass of
PlatformTest (which is probably why we have CocoaTestHelper rather than
CocoaTest in the first place :-).

On 2009/10/06 19:46:12, John Grabowski wrote:
> SGTM
> 
> On Tue, Oct 6, 2009 at 12:15 PM, Scott Hess <mailto:shess@chromium.org> wrote:
> 
> > How about a CocoaNoWindowTestHelper class which pulls up the shared
> > constructor, which CocoaTestHelper inherits from and augments?

[John Grabowski, 2009-10-07 18:07:00]

LGTM++