Sad Tab view for the Mac.

Sad Tab view for the Mac.


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

[Avi (use Gerrit), 2009-02-12 22:31:48]

Pink: all the squishy ObjC stuff
Brett: I can have the view observe the contents like this, right?

[pink (ping after 24hrs), 2009-02-12 22:40:20]

LGTM with questions addressed

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

http://codereview.chromium.org/20334/diff/1/5#newcode18
Line 18: - (id)initWithFrame:(NSRect)frame;
inherited, no need to include in public API.

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

http://codereview.chromium.org/20334/diff/1/4#newcode13
Line 13: - (id)initWithFrame:(NSRect)frame {
why bother implementing this? why not just omit it?

http://codereview.chromium.org/20334/diff/1/4#newcode29
Line 29: // Layout
can we not just load this view from the nib?

http://codereview.chromium.org/20334/diff/1/7
File chrome/chrome.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/20334/diff/1/7#newcode2391
Line 2391: 824FC14E0F44C56A000299E5 /* sadtab.png */ = {isa = PBXFileReference;
lastKnownFileType = image.png; name = sadtab.png; path = theme/sadtab.png;
sourceTree = "<group>"; };
maybe file a bug that we want a PDF for this?

[Avi (use Gerrit), 2009-02-12 22:50:33]

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

http://codereview.chromium.org/20334/diff/1/5#newcode18
Line 18: - (id)initWithFrame:(NSRect)frame;
On 2009/02/12 22:40:21, pink wrote:
> inherited, no need to include in public API.

True, but it's the designated initializer.

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

http://codereview.chromium.org/20334/diff/1/4#newcode13
Line 13: - (id)initWithFrame:(NSRect)frame {
On 2009/02/12 22:40:21, pink wrote:
> why bother implementing this? why not just omit it?

Needed if we declare the initializer in the .h.

http://codereview.chromium.org/20334/diff/1/4#newcode29
Line 29: // Layout
On 2009/02/12 22:40:21, pink wrote:
> can we not just load this view from the nib?

We want a nib for this view? I ported the Windows view code as it was simple
enough, and would match.

http://codereview.chromium.org/20334/diff/1/7
File chrome/chrome.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/20334/diff/1/7#newcode2391
Line 2391: 824FC14E0F44C56A000299E5 /* sadtab.png */ = {isa = PBXFileReference;
lastKnownFileType = image.png; name = sadtab.png; path = theme/sadtab.png;
sourceTree = "<group>"; };
On 2009/02/12 22:40:21, pink wrote:
> maybe file a bug that we want a PDF for this?

I thought the idea was that it was an ugly bitmap, like the Sad Mac. That's a
design issue. Who designed this page?

[pink (ping after 24hrs), 2009-02-12 23:02:06]

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

http://codereview.chromium.org/20334/diff/1/5#newcode18
Line 18: - (id)initWithFrame:(NSRect)frame;
On 2009/02/12 22:50:34, Avi wrote:
> On 2009/02/12 22:40:21, pink wrote:
> > inherited, no need to include in public API.
> 
> True, but it's the designated initializer.

i guess, though you don't say that in a comment. If it's obvious because it's an
NSView subclass, then omitting it would still be obvious.

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

http://codereview.chromium.org/20334/diff/1/4#newcode29
Line 29: // Layout
On 2009/02/12 22:50:34, Avi wrote:
> On 2009/02/12 22:40:21, pink wrote:
> > can we not just load this view from the nib?
> 
> We want a nib for this view? I ported the Windows view code as it was simple
> enough, and would match.

I just figured it'd be easier (less code), but maybe we should be nib adverse
for l10n reasons?

http://codereview.chromium.org/20334/diff/1/7
File chrome/chrome.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/20334/diff/1/7#newcode2391
Line 2391: 824FC14E0F44C56A000299E5 /* sadtab.png */ = {isa = PBXFileReference;
lastKnownFileType = image.png; name = sadtab.png; path = theme/sadtab.png;
sourceTree = "<group>"; };
On 2009/02/12 22:50:34, Avi wrote:
> On 2009/02/12 22:40:21, pink wrote:
> > maybe file a bug that we want a PDF for this?
> 
> I thought the idea was that it was an ugly bitmap, like the Sad Mac. That's a
> design issue. Who designed this page?

ugly or not, it should still probably scale gracefully. maybe the png is
sufficient, i dunno.

[brettw, 2009-02-12 23:47:42]

http://codereview.chromium.org/20334/diff/1/2
File chrome/browser/tab_contents/web_contents_view_mac.mm (right):

http://codereview.chromium.org/20334/diff/1/2#newcode22
Line 22: NotificationService::current()->AddObserver(
Can you use the NotificationRegistrar object instead? It will automatically
unregister them for you. I actually want to kill the public API to the
notfication service in favor of the registrar, since it's harder to mess up.

http://codereview.chromium.org/20334/diff/1/2#newcode269
Line 269: // set as dominant child
Need capital and period for this comment.

http://codereview.chromium.org/20334/diff/1/6
File chrome/common/temp_scaffolding_stubs.h (right):

http://codereview.chromium.org/20334/diff/1/6#newcode904
Line 904: return true; // assume for now that all renderer crashes are important
Need 2 spaces before the comment, capital, and a period.

[Avi (use Gerrit), 2009-02-13 16:10:48]

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

http://codereview.chromium.org/20334/diff/1/5#newcode18
Line 18: - (id)initWithFrame:(NSRect)frame;
On 2009/02/12 23:02:06, pink wrote:
> If it's obvious because it's an
> NSView subclass, then omitting it would still be obvious.

I'll mention it in a comment.

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

http://codereview.chromium.org/20334/diff/1/4#newcode29
Line 29: // Layout
On 2009/02/12 23:02:06, pink wrote:
> I just figured it'd be easier (less code), but maybe we should be nib adverse
> for l10n reasons?

Sounds like a good reason (two string vs nib). For now, it's simple enough that
I'm not worried.

http://codereview.chromium.org/20334/diff/1/2
File chrome/browser/tab_contents/web_contents_view_mac.mm (right):

http://codereview.chromium.org/20334/diff/1/2#newcode22
Line 22: NotificationService::current()->AddObserver(
On 2009/02/12 23:47:42, brettw wrote:
> Can you use the NotificationRegistrar object instead?

Will do... So it's OK to have the view register on the controller?

http://codereview.chromium.org/20334/diff/1/7
File chrome/chrome.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/20334/diff/1/7#newcode2391
Line 2391: 824FC14E0F44C56A000299E5 /* sadtab.png */ = {isa = PBXFileReference;
lastKnownFileType = image.png; name = sadtab.png; path = theme/sadtab.png;
sourceTree = "<group>"; };
On 2009/02/12 23:02:06, pink wrote:
> ugly or not, it should still probably scale gracefully. maybe the png is
> sufficient, i dunno.

For now it is; crbug.com/7691 filed.

[Avi (use Gerrit), 2009-02-13 20:48:19]

Brett--this look good?

[brettw, 2009-02-13 21:38:12]

LGTM.

http://codereview.chromium.org/20334/diff/17/19
File chrome/browser/tab_contents/web_contents_view_mac.h (right):

http://codereview.chromium.org/20334/diff/17/19#newcode99
Line 99: scoped_cftyperef<SadTabView*> sad_tab_;
Note for the future: it would be better to call this ScopedCFTypeRef to match
Google style. scoped_ptr is from Boost so we keep the name.

[Avi (use Gerrit), 2009-02-17 15:10:55]

On 2009/02/13 21:38:12, brettw wrote:
> Note for the future: it would be better to call this ScopedCFTypeRef to match
> Google style. scoped_ptr is from Boost so we keep the name.

OK; out of scope for this review.