Implement confirm bubble for Mac.

Implement confirm bubble for Mac.
This change is the initial implementation that ports the ConfirmBubble classes to Mac. To allow the ConfirmBubbleModel class to add/remove its controls, this change programmatically creates a custom NSView without using a nib file.

BUG=99130
TEST=ConfirmBubbleControllerTest.*

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

[Hironori Bono, 2011-11-28 10:05:40]

Greetings Mark,

Even though this change overrides loadView: to create a custom NSView
programmatically, I'm not sure if this is the right approach. Would it be
possible to give me your advice?

Regards,

Hironori Bono

[Mark Mentovai, 2011-11-28 14:33:06]

Seems reasonable.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_controller.mm (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:20:
bundle:base::mac::MainAppBundle()])) {
I think this is unused if the preceding parameter is nil, so you might as well
just pass nil for this one too.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:28: - (void)dealloc {
This method isn’t necessary. If you don’t provide an implementation, the
superclass’ -dealloc will be called anyway.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:38:
NSLog(@"[ConfirmBubbleController windowWillClose:]");
?

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:97: 
Remove the blank line at the end of this file.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_view.h (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:18: // * two optional buttons
("ok" and "cancel")
It has a close button too?

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:29: //   |          [OK] [Cancel]
|
Normal Mac button ordering in dialogs is to put the “proceed” action (like “OK”)
all the way on the right.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:96: - (void)dealloc {
Not needed.

[Robert Sesek, 2011-11-28 15:55:39]

Could you also add a test for the controller/view?

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_controller.h (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.h:4: 
#if guards

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_view.h (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:4: 
#if guards

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:26: //   | icon title           x
|
It's  a little odd to have the close button on the right on Mac. Is it even
necessary with the buttons below?

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:61: 
nit: remove

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:74: - (void)initBubble;
In other code, we name this method |-performLayout|.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:178: [cancelButton_.get()
sizeToFit];
Does this look OK? You may want to use -[GTMUILocalizerAndLayoutTweaker
sizeToFitView:]

[tfarina, 2011-11-30 15:58:38]

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/confirm_bu...
File chrome/browser/ui/confirm_bubble_model.cc (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/confirm_bu...
chrome/browser/ui/confirm_bubble_model.cc:45: void
ConfirmBubbleModel::Show(gfx::NativeView view,
This is better implemented as a free function.

May be something like:

confirm_bubble.h

namespace browser {

void ShowConfirmBubble(gfx::NativeView view,
                       const gfx::Point& origin,
                       ConfirmBubbleModel* model);

}  // namespace browser

And let the platform specific files implement it.

confirm_bubble_view.cc

#include "chrome/browser/ui/confirm_bubble.h

....

namespace browser {

void ShowConfirmBubble(...) {
...
}

}  // namespace browser

With that I think you can avoid having the model know about view and all these
ugly ifdefs!

[Hironori Bono, 2011-12-09 11:14:32]

Greetings Mark, Robert, and Thiago,

Thank you for your review and comments.
I have applied your comments as much as possible and added tests. Is it possible
to take another look?

Regards,

Hironori Bono

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_controller.h (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.h:4: 
On 2011/11/28 15:55:39, rsesek wrote:
> #if guards

Done. Thanks for reminding me.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_controller.mm (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:20:
bundle:base::mac::MainAppBundle()])) {
On 2011/11/28 14:33:06, Mark Mentovai wrote:
> I think this is unused if the preceding parameter is nil, so you might as well
> just pass nil for this one too.

Done. Thanks for your explanation.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:28: - (void)dealloc {
On 2011/11/28 14:33:06, Mark Mentovai wrote:
> This method isn’t necessary. If you don’t provide an implementation, the
> superclass’ -dealloc will be called anyway.

Done. Thanks for noticing it.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:38:
NSLog(@"[ConfirmBubbleController windowWillClose:]");
On 2011/11/28 14:33:06, Mark Mentovai wrote:
> ?

Done. Whoops, I forgot deleting this line before uploading this change.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_view.h (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:4: 
On 2011/11/28 15:55:39, rsesek wrote:
> #if guards

Done.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:18: // * two optional buttons
("ok" and "cancel")
On 2011/11/28 14:33:06, Mark Mentovai wrote:
> It has a close button too?

As written below, I have removed the close button on Mac. Thanks for your
comment.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:26: //   | icon title           x
|
On 2011/11/28 15:55:39, rsesek wrote:
> It's  a little odd to have the close button on the right on Mac. Is it even
> necessary with the buttons below?

Thanks for noticing it. This change straightforwardly implemented the Windows
code and it indeed looks weird on Mac. After talking with some people, I have
removed this button on Mac.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.h:29: //   |          [OK] [Cancel]
|
On 2011/11/28 14:33:06, Mark Mentovai wrote:
> Normal Mac button ordering in dialogs is to put the “proceed” action (like
“OK”)
> all the way on the right.

Done.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:61: 
On 2011/11/28 15:55:39, rsesek wrote:
> nit: remove

Done.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:74: - (void)initBubble;
On 2011/11/28 15:55:39, rsesek wrote:
> In other code, we name this method |-performLayout|.

Done. Thanks for your correction.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:96: - (void)dealloc {
On 2011/11/28 14:33:06, Mark Mentovai wrote:
> Not needed.

Done.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/cocoa/conf...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:178: [cancelButton_.get()
sizeToFit];
On 2011/11/28 15:55:39, rsesek wrote:
> Does this look OK? You may want to use -[GTMUILocalizerAndLayoutTweaker
> sizeToFitView:]

Thanks for your suggestion. Even though this looks ok to me, let me try it.

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/confirm_bu...
File chrome/browser/ui/confirm_bubble_model.cc (right):

http://codereview.chromium.org/8697001/diff/4001/chrome/browser/ui/confirm_bu...
chrome/browser/ui/confirm_bubble_model.cc:45: void
ConfirmBubbleModel::Show(gfx::NativeView view,
On 2011/11/30 15:58:38, tfarina wrote:
> This is better implemented as a free function.
> 
> May be something like:
> 
> confirm_bubble.h
> 
> namespace browser {
> 
> void ShowConfirmBubble(gfx::NativeView view,
>                        const gfx::Point& origin,
>                        ConfirmBubbleModel* model);
> 
> }  // namespace browser
> 
> And let the platform specific files implement it.
> 
> confirm_bubble_view.cc
> 
> #include "chrome/browser/ui/confirm_bubble.h
> 
> ....
> 
> namespace browser {
> 
> void ShowConfirmBubble(...) {
> ...
> }
> 
> }  // namespace browser
> 
> With that I think you can avoid having the model know about view and all these
> ugly ifdefs!

Thanks for your suggestion. I'm implementing GTK code now and it may need more
changes to this function. When I finish implementing this function for all
platforms, I will refactor this class to remove this #ifdef.

[Robert Sesek, 2011-12-12 15:42:38]

This is starting to really look good. Nice job. Just a few more questions and
some nits.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_controller.h (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller.h:34: // Objective-C
variables.
nit: they convert from C++ types to Obj-C types, not variables

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller.h:39: - (NSString*)okButton;
Seeing this in code, this name makes it seem like it's returning NSButton*. I'd
recommend |-okButtonText| and |-cancelButtonText|.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_controller.mm (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller.mm:10: #import
"chrome/browser/ui/cocoa/confirm_bubble_view.h"
nit: alphabetize

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller_unittest.mm:110: virtual
~ConfirmBubbleControllerTest() {
No need to provide an empty dtor

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller_unittest.mm:122:
ConfirmBubbleController* controller_;
  // weak; owns self

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.h (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.h:50: // custom NSView (i.e. without
using a nib file), this function should be called
Why do you not use a nib here? The layout of this seems static (minus fixup for
localization and text overflow).

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.h:58: @interface
ConfirmBubbleView(ExposedForUnitTesting)
nit: space before (

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:12: //#include
"grit/theme_resources.h"
Remove?

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:23: // The corner redius of this
bubble view
nit: full-stop

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:72: @interface
ConfirmBubbleView(Private)
nit: space before (

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:83: NSRect bounds;
Use NSMakeRect

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:97: float topLeftRadius =
kBubbleCornerRadius;
Any reason why you don't just use the constant as an argument to
gtm_bezierPtah...?

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:157: - (void)performLayout {
This function may benefit from being broken up into helper methods. At ~100
lines long, it's on the line, so I'll leave that up to you.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:236: left, bottom, [iconImage
size].width,[iconImage size].height)]);
nit: space after ,

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:274: @implementation
ConfirmBubbleView(ExposedForUnitTesting)
nit: space after (

[Hironori Bono, 2011-12-15 13:06:14]

Greetings Robert,

Thanks again for your review. I have updated this change to apply your comments.
Is it possible to take another look?

Regards,

Hironori Bono

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_controller.h (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller.h:34: // Objective-C
variables.
On 2011/12/12 15:42:43, rsesek wrote:
> nit: they convert from C++ types to Obj-C types, not variables

Done. Thanks for your comment. I have updated this comment and the one in
confirm_bubble_controller.mm.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller.h:39: - (NSString*)okButton;
On 2011/12/12 15:42:43, rsesek wrote:
> Seeing this in code, this name makes it seem like it's returning NSButton*.
I'd
> recommend |-okButtonText| and |-cancelButtonText|.

Done. It also makes sense to me.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller_unittest.mm:110: virtual
~ConfirmBubbleControllerTest() {
On 2011/12/12 15:42:43, rsesek wrote:
> No need to provide an empty dtor

Done. Thank you for your comment.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_controller_unittest.mm:122:
ConfirmBubbleController* controller_;
On 2011/12/12 15:42:43, rsesek wrote:
>   // weak; owns self

Done. Thank you for your comment.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.h (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.h:50: // custom NSView (i.e. without
using a nib file), this function should be called
On 2011/12/12 15:42:43, rsesek wrote:
> Why do you not use a nib here? The layout of this seems static (minus fixup
for
> localization and text overflow).

Good point. In brief, I omit this feature in this version.
This class was initially designed for allowing the ConfirmBubbleModel class to
change the layout of a bubble at run time, i.e. providing Chrome developers a
platform-independent method that they can change the layout of their bubbles.
But, we decided not to implement this feature in the first version to make this
change as small as possible. Is is better to use a nib until we do need layout
components  manually?

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:12: //#include
"grit/theme_resources.h"
On 2011/12/12 15:42:43, rsesek wrote:
> Remove?

Done. Thanks. I forgot removing them after checking required header files.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:23: // The corner redius of this
bubble view
On 2011/12/12 15:42:43, rsesek wrote:
> nit: full-stop

Done.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:72: @interface
ConfirmBubbleView(Private)
On 2011/12/12 15:42:43, rsesek wrote:
> nit: space before (

Done.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:97: float topLeftRadius =
kBubbleCornerRadius;
On 2011/12/12 15:42:43, rsesek wrote:
> Any reason why you don't just use the constant as an argument to
> gtm_bezierPtah...?

Done. Sorry, I forgot cleaning up this code after checking the behavior of
gtm_bezierPath:.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:157: - (void)performLayout {
On 2011/12/12 15:42:43, rsesek wrote:
> This function may benefit from being broken up into helper methods. At ~100
> lines long, it's on the line, so I'll leave that up to you.

Thanks for your comment. I would like to keep the current code temporarily for
consistency with the Views version of ConfirmBubbleView. (Peter suggested to put
all layout code to one function when I implemented this class for Views.)

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:236: left, bottom, [iconImage
size].width,[iconImage size].height)]);
On 2011/12/12 15:42:43, rsesek wrote:
> nit: space after ,

Done.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:274: @implementation
ConfirmBubbleView(ExposedForUnitTesting)
On 2011/12/12 15:42:43, rsesek wrote:
> nit: space after (

Does this mean 'before (' as written above?

[Robert Sesek, 2011-12-15 18:47:08]

LGTM. Nice work!

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.h (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.h:50: // custom NSView (i.e. without
using a nib file), this function should be called
On 2011/12/15 13:06:14, Hironori Bono wrote:
> On 2011/12/12 15:42:43, rsesek wrote:
> > Why do you not use a nib here? The layout of this seems static (minus fixup
> for
> > localization and text overflow).
> 
> Good point. In brief, I omit this feature in this version.
> This class was initially designed for allowing the ConfirmBubbleModel class to
> change the layout of a bubble at run time, i.e. providing Chrome developers a
> platform-independent method that they can change the layout of their bubbles.
> But, we decided not to implement this feature in the first version to make
this
> change as small as possible. Is is better to use a nib until we do need layout
> components  manually?

If the intention is for the layout to be changed in platform-independent code,
then keeping the layout as code makes more sense and this is fine for now.

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/19001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:157: - (void)performLayout {
On 2011/12/15 13:06:14, Hironori Bono wrote:
> On 2011/12/12 15:42:43, rsesek wrote:
> > This function may benefit from being broken up into helper methods. At ~100
> > lines long, it's on the line, so I'll leave that up to you.
> 
> Thanks for your comment. I would like to keep the current code temporarily for
> consistency with the Views version of ConfirmBubbleView. (Peter suggested to
put
> all layout code to one function when I implemented this class for Views.)

As I said, it's borderline right now, so this is fine. But if you add more
layout logic in the future, this should probably broken up. It may not be the
convention in Views, but it is in Cocoa.

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_model.mm (right):

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_model.mm:10: void
ConfirmBubbleModel::Show(gfx::NativeView view,
This could probably just go in the controller.mm file if you wanted.

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:63: - (BOOL)acceptsFirstResponder
{
Does this mean the text of the bubble is not selectable?

[Hironori Bono, 2012-01-05 10:45:55]

Greetings,

Many thanks for your comments and I'm so sorry for my slow response. (My Mac
does not work so well these days and I needed to re-install Mac OS.) I have
updated license headers and will land it when try jobs cycle green.

Regards,

Hironori Bono

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_model.mm (right):

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_model.mm:10: void
ConfirmBubbleModel::Show(gfx::NativeView view,
On 2011/12/15 18:47:09, rsesek wrote:
> This could probably just go in the controller.mm file if you wanted.

Thanks for your suggestion. I would like to talk with developers of other
platforms (Win, GTK, Views) to find a consistent solution.

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/confirm_bubble_view.mm (right):

http://codereview.chromium.org/8697001/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/confirm_bubble_view.mm:63: - (BOOL)acceptsFirstResponder
{
On 2011/12/15 18:47:09, rsesek wrote:
> Does this mean the text of the bubble is not selectable?

Thanks for your good question. In brief, same as NSTextView, it is up to the
return value of -isSelectable. That is, when we call [view setSelectable:YES],
we can create selection in the text of this view. On the other hand, we cannot
do it when we call [view setSelectable:NO]. (We allow creating selections into
the message text of a ConfirmBubbleView and we do not allow creating selection
in its title.)

[Hironori Bono, 2012-01-10 09:22:45]

Greetings Peter,

Would it be possible to review a change to
"chrome/browser/ui/confirm_bubble_model.cc"? (I have just commented out on Mac
because I needed to implement this function in Objective-C on Mac.)

Regards,

Hironori Bono

[Peter Kasting, 2012-01-10 18:52:03]

http://codereview.chromium.org/8697001/diff/43001/chrome/browser/ui/confirm_b...
File chrome/browser/ui/confirm_bubble_model.cc (right):

http://codereview.chromium.org/8697001/diff/43001/chrome/browser/ui/confirm_b...
chrome/browser/ui/confirm_bubble_model.cc:43: #if !defined(OS_MACOSX)
Don't do things this way.  If you can't put the ObjC impl in this file, then
create separate files for Views and GTK like the one you have for Mac, and move
the code here into those.

[Hironori Bono, 2012-01-11 06:36:40]

Greetings Peter,

Thanks for your review and suggestion. I have updated this change to apply your
comments.

Regards,

Hironori Bono

http://codereview.chromium.org/8697001/diff/43001/chrome/browser/ui/confirm_b...
File chrome/browser/ui/confirm_bubble_model.cc (right):

http://codereview.chromium.org/8697001/diff/43001/chrome/browser/ui/confirm_b...
chrome/browser/ui/confirm_bubble_model.cc:43: #if !defined(OS_MACOSX)
On 2012/01/10 18:52:03, Peter Kasting wrote:
> Don't do things this way.  If you can't put the ObjC impl in this file, then
> create separate files for Views and GTK like the one you have for Mac, and
move
> the code here into those.

Done. Thanks for your suggestion. (I should have moved it when Thiago suggested
it.) I have moved this function to "ui/views/confirm_bubble_model_views.cc". (I
have added "_views" because I somehow saw a build break on Windows when I added
"ui/views/confirm_bubble_view.cc".)

[Peter Kasting, 2012-01-11 18:52:48]

On 2012/01/11 06:36:40, Hironori Bono wrote:
> I have moved this function to "ui/views/confirm_bubble_model_views.cc". (I
> have added "_views" because I somehow saw a build break on Windows when I
added
> "ui/views/confirm_bubble_view.cc".)

Right... can't have two C++ source files with the same name in the same library
or the linker gets confused.

The only reason we avoid this problem on Mac is because one ends with .cc and
the other .mm.  Maybe the best answer here, for both platforms, is to do what we
do in a number of other places, which is to define this lone function atop the
"confirm_bubble_view.xx" file, instead of adding a separate file for it.  This
makes some sense anyway because this function is so closely related to the
ConfirmBubbleView object.

[Hironori Bono, 2012-01-12 10:48:29]

Greetings Peter,

Many thanks for your explanation. I have moved the function to
confirm_bubble_view.*. Is it possible to take another look?

Regards,

Hironori Bono

On 2012/01/11 18:52:48, Peter Kasting wrote:
> Right... can't have two C++ source files with the same name in the same
library
> or the linker gets confused.
> 
> The only reason we avoid this problem on Mac is because one ends with .cc and
> the other .mm.  Maybe the best answer here, for both platforms, is to do what
we
> do in a number of other places, which is to define this lone function atop the
> "confirm_bubble_view.xx" file, instead of adding a separate file for it.  This
> makes some sense anyway because this function is so closely related to the
> ConfirmBubbleView object.

[Peter Kasting, 2012-01-12 20:56:34]

LGTM.  If you want, you can also add separator lines:

// ConfirmBubbleModel ---------------------...

ConfirmBubbleModel::Show() { ...

// ConfirmBubbleView ----------------------...

...

Up to you.

[Mark Mentovai, 2012-01-12 21:09:48]

LGTM from me also.

[commit-bot: I haz the power, 2012-01-13 07:55:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hbono@chromium.org/8697001/69001

[commit-bot: I haz the power, 2012-01-13 09:11:09]

Change committed as 117621