Implemented most of HtmlDialogWindowController, which is a Cocoa port

Implemented most of HtmlDialogWindowController, which is a Cocoa port
of HtmlDialogView.

Added TODO to fix inaccurate font height metric for OS X font class (and maybe width).

Added the BrowserCommandExecutor protocol so that not just a
BrowserWindowController can be the window controller for a
ChromeEventProcessingWindow.

Added unittests.  Also tested manually with the bookmark sync setup
wizard dialog.

BUG=23073
TEST=added unittests, trybot, and manual testing

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

[akalin, 2009-10-28 17:14:50]

+pink for code review.

[pink (ping after 24hrs), 2009-10-28 18:39:47]

http://codereview.chromium.org/344008/diff/4011/3012
File app/gfx/font_mac.mm (right):

http://codereview.chromium.org/344008/diff/4011/3012#newcode36
Line 36: height_ = [font boundingRectForFont].size.height;
do you have an idea of how different these are? what other areas would be
affected by this change?

http://codereview.chromium.org/344008/diff/4011/3018
File chrome/browser/cocoa/html_dialog_window_controller.h (right):

http://codereview.chromium.org/344008/diff/4011/3018#newcode23
Line 23: class HtmlDialogWindowDelegate : public HtmlDialogUIDelegate,
if it's a bridge class from C++ to objc, generally we put "bridge" in the name,
eg HtmlDialogWindowDelegateBridge.

http://codereview.chromium.org/344008/diff/4011/3018#newcode37
Line 37: 
no need for blank lines between these methods, esp since there are no comments
and they're all in iherited.

http://codereview.chromium.org/344008/diff/4011/3018#newcode88
Line 88: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/344008/diff/4011/3018#newcode117
Line 117: // Must be called before showWindow.
What does this do? How does it work? If you don't want to put those details
here, they need to be in the .mm file.

http://codereview.chromium.org/344008/diff/4011/3019
File chrome/browser/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/344008/diff/4011/3019#newcode22
Line 22: CHECK(delegate_ != NULL);
DCHECK? Do we really want to crash the browser on the user?

http://codereview.chromium.org/344008/diff/4011/3019#newcode37
Line 37: if ((delegate_ != NULL) && delegate_->IsDialogModal()) {
just do if (delegate_ && delegate_->...). Cleaner and easier to read.

http://codereview.chromium.org/344008/diff/4011/3019#newcode52
Line 52: std::vector<DOMMessageHandler*>* handlers) const {
does the win code have checks that these ptr params can be null? should all
platform impls check?

http://codereview.chromium.org/344008/diff/4011/3019#newcode81
Line 81: [controller_ release];
whoa. you said in the header this is a weak reference. why releasing it here?

http://codereview.chromium.org/344008/diff/4011/3019#newcode144
Line 144: @interface HtmlDialogWindowController (InternalAPI)
<BrowserCommandExecutor>
why not make this a part of the public api decl?

http://codereview.chromium.org/344008/diff/4011/3019#newcode176
Line 176: CHECK(delegate != NULL);
dcheck?

http://codereview.chromium.org/344008/diff/4011/3019#newcode180
Line 180: // Put the dialog box in the center of the window.
is this dialog modal or modeless?

http://codereview.chromium.org/344008/diff/4011/3019#newcode188
Line 188: CGRectMake(parent_window_origin.x +
why not just use NSMakeRect() and save the conversions?

http://codereview.chromium.org/344008/diff/4011/3019#newcode194
Line 194: NSUInteger style = NSTitledWindowMask | NSClosableWindowMask;
i'm a bit concerned that the dialogs we will be displaying will have ok/cancel
type action buttons in them, in which case they shouldn't have the
NSClosableWindowMask. If these are modeless, they probably shouldn't have these
buttons in them at all, however. The UI should most likely be different on Mac
from the other platforms, contrary to the design of these dialogs.

http://codereview.chromium.org/344008/diff/4011/3019#newcode195
Line 195: ChromeEventProcessingWindow* window =
use a scoped_nsobject here rather than manually releasing below.

http://codereview.chromium.org/344008/diff/4011/3019#newcode196
Line 196: [[ChromeEventProcessingWindow alloc]
continuation indents are 4 spaces.

http://codereview.chromium.org/344008/diff/4011/3019#newcode221
Line 221: Profile* profile = browser_->profile();
could this be an incognito window? do we need to get the original profile out of
it or something?

http://codereview.chromium.org/344008/diff/4011/3019#newcode223
Line 223: [self.window setContentView:tab_contents_->GetNativeView()];
we don't allow dot-notation in the obj-c guide.

http://codereview.chromium.org/344008/diff/4011/3019#newcode234
Line 234: // TODO(akalin): add accelerator for ESC to close the dialog box.
you should just be able to implement -(void)cancel:(id)sender and get the
message automatically.

http://codereview.chromium.org/344008/diff/4011/3019#newcode239
Line 239: delegate_->OnDialogClosed("");
don't we need to autorelease ourselves here?

[akalin (wrong akalin), 2009-10-28 20:29:15]

On Wed, Oct 28, 2009 at 11:39 AM,  <pinkerton@chromium.org> wrote:
>
> http://codereview.chromium.org/344008/diff/4011/3012
> File app/gfx/font_mac.mm (right):
>
> http://codereview.chromium.org/344008/diff/4011/3012#newcode36
> Line 36: height_ = [font boundingRectForFont].size.height;
> do you have an idea of how different these are? what other areas would
> be affected by this change?

Yes.  For Helvetica 16 point, x-height is 8, which made the sync setup
wizard dialog box way too small (since its size is estimated # lines *
approximate web font height).  On windows, the web font is Arial 16
point with a height of 24.  I checked the documentation for
GetTextMetrics(), which is what Windows uses and where the height is
taken from, and found that it returns the ascender + descender height.
 Then I tried changing that line to be ascender + descender but that
only gave me 16. (ascender = 12, descender = 4, IIRC).  At least how
it is used by SyncSetupWizard, I figured the height of
boundingRectForFont would be the most fitting metric, since it really
wants the line height of the font.

As far as I can tell, the only other non-Windows use of font height is
in printed_document.cc, in PrintHeaderFooter().  I will see if that is
affected by this change.

> http://codereview.chromium.org/344008/diff/4011/3018
> File chrome/browser/cocoa/html_dialog_window_controller.h (right):
>
> http://codereview.chromium.org/344008/diff/4011/3018#newcode23
> Line 23: class HtmlDialogWindowDelegate : public HtmlDialogUIDelegate,
> if it's a bridge class from C++ to objc, generally we put "bridge" in
> the name, eg HtmlDialogWindowDelegateBridge.

Fixed.

> http://codereview.chromium.org/344008/diff/4011/3018#newcode37
> Line 37:
> no need for blank lines between these methods, esp since there are no
> comments and they're all in iherited.

Done.

> http://codereview.chromium.org/344008/diff/4011/3018#newcode88
> Line 88: };
> DISALLOW_COPY_AND_ASSIGN?

Done.

> http://codereview.chromium.org/344008/diff/4011/3018#newcode117
> Line 117: // Must be called before showWindow.
> What does this do? How does it work? If you don't want to put those
> details here, they need to be in the .mm file.

Done.

> http://codereview.chromium.org/344008/diff/4011/3019
> File chrome/browser/cocoa/html_dialog_window_controller.mm (right):
>
> http://codereview.chromium.org/344008/diff/4011/3019#newcode22
> Line 22: CHECK(delegate_ != NULL);
> DCHECK? Do we really want to crash the browser on the user?

Done.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode37
> Line 37: if ((delegate_ != NULL) && delegate_->IsDialogModal()) {
> just do if (delegate_ && delegate_->...). Cleaner and easier to read.

Done.  Also fixed other places.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode52
> Line 52: std::vector<DOMMessageHandler*>* handlers) const {
> does the win code have checks that these ptr params can be null? should
> all platform impls check?

Nope, but they should, and also clear handlers if delegate_ is NULL.
Added TODO.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode81
> Line 81: [controller_ release];
> whoa. you said in the header this is a weak reference. why releasing it
> here?

My mistake, it's not a weak reference.  On the other hand, it doesn't
own controller_, but may call release on it.  Documented this awkward
behavior.

>
> http://codereview.chromium.org/344008/diff/4011/3019#newcode144
> Line 144: @interface HtmlDialogWindowController (InternalAPI)
> <BrowserCommandExecutor>
> why not make this a part of the public api decl?

It seems to me that the fact that this windowcontroller's window is a
ChromeEventProcessingWindow (which requires its controller to be a
BrowserCommandExecutor) is an implementation detail, and we don't
expect anything but the window to use executeCommand on the window
controller.

>
> http://codereview.chromium.org/344008/diff/4011/3019#newcode176
> Line 176: CHECK(delegate != NULL);
> dcheck?

done.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode180
> Line 180: // Put the dialog box in the center of the window.
> is this dialog modal or modeless?

Depends on what the delegate says, although for now we make everything
modeless.  I will implement modal in a later changelist.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode188
> Line 188: CGRectMake(parent_window_origin.x +
> why not just use NSMakeRect() and save the conversions?

Done.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode194
> Line 194: NSUInteger style = NSTitledWindowMask | NSClosableWindowMask;
> i'm a bit concerned that the dialogs we will be displaying will have
> ok/cancel type action buttons in them, in which case they shouldn't have
> the NSClosableWindowMask. If these are modeless, they probably shouldn't
> have these buttons in them at all, however. The UI should most likely be
> different on Mac from the other platforms, contrary to the design of
> these dialogs.

I added the close button to mimic the windows version.  However,
there's nothing that says that the HTML content must provide some way
to close the window.  Let's discuss this in chat later...

>
> http://codereview.chromium.org/344008/diff/4011/3019#newcode195
> Line 195: ChromeEventProcessingWindow* window =
> use a scoped_nsobject here rather than manually releasing below.

Done.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode196
> Line 196: [[ChromeEventProcessingWindow alloc]
> continuation indents are 4 spaces.

Fixed.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode221
> Line 221: Profile* profile = browser_->profile();
> could this be an incognito window? do we need to get the original
> profile out of it or something?

Hmm, the windows version uses the profile directly.  It could be that
the original profile is retrieved before passing it into this.  Added
a TODO for further investigation.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode223
> Line 223: [self.window setContentView:tab_contents_->GetNativeView()];
> we don't allow dot-notation in the obj-c guide.

Fixed.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode234
> Line 234: // TODO(akalin): add accelerator for ESC to close the dialog
> box.
> you should just be able to implement -(void)cancel:(id)sender and get
> the message automatically.

Are you sure?  I couldn't find any documentation on this in
NSWindowController; the closest thing I could find was in NSSavePanel.

> http://codereview.chromium.org/344008/diff/4011/3019#newcode239
> Line 239: delegate_->OnDialogClosed("");
> don't we need to autorelease ourselves here?

No, the delegate will release the controller itself.

Please take another look.

-- 
Frederick Akalin
Software Engineer

[dmac, 2009-10-28 21:14:55]

http://codereview.chromium.org/344008/diff/2003/2010
File chrome/browser/cocoa/html_dialog_window_controller.h (right):

http://codereview.chromium.org/344008/diff/2003/2010#newcode71
Line 71: // does not own, but may call release on this.
you don't want to call release on a NSWindowController, you want to call close.

http://codereview.chromium.org/344008/diff/2003/2011
File chrome/browser/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/344008/diff/2003/2011#newcode208
Line 208: if (window.get() == nil) {
usually would be if (!window.get()) {

http://codereview.chromium.org/344008/diff/2003/2011#newcode212
Line 212: if (self == nil) {
usually it would be if (!self) {

http://codereview.chromium.org/344008/diff/2003/2011#newcode218
Line 218: parent_window_ = parent_window;
you don't appear to be using parent_window_ anywhere that I can see. Any reason
to have it?

http://codereview.chromium.org/344008/diff/2003/2011#newcode245
Line 245: delegate_->OnDialogClosed("");
Definitely need to autorelease ourselves here. Details here:
http://developer.apple.com/mac/library/DOCUMENTATION/Cocoa/Conceptual/Documen...

http://codereview.chromium.org/344008/diff/2003/2012
File chrome/browser/cocoa/html_dialog_window_controller_unittest.mm (right):

http://codereview.chromium.org/344008/diff/2003/2012#newcode37
Line 37: class HtmlDialogWindowControllerTest : public BrowserWithTestWindowTest
{
Not needed as part of this review, but I need to get BrowserWithTestWindowTest
working with CocoaTest.

http://code.google.com/p/chromium/issues/detail?id=26133

http://codereview.chromium.org/344008/diff/2003/2012#newcode62
Line 62: scoped_nsobject<HtmlDialogWindowController>
html_dialog_window_controller_;
don't wrap NSWindowController's in scoped_nsobjects.

http://codereview.chromium.org/344008/diff/2003/2012#newcode120
Line 120: [html_dialog_window_controller_ retain];
we should need a retain here. Something is wrong if you have a retain. close
should be autoreleasing us properly.

[akalin (wrong akalin), 2009-10-28 22:19:52]

On Wed, Oct 28, 2009 at 2:14 PM,  <dmaclach@chromium.org> wrote:
>
> http://codereview.chromium.org/344008/diff/2003/2010
> File chrome/browser/cocoa/html_dialog_window_controller.h (right):
>
> http://codereview.chromium.org/344008/diff/2003/2010#newcode71
> Line 71: // does not own, but may call release on this.
> you don't want to call release on a NSWindowController, you want to call
> close.

Done.  Fixed it so that the window controller autoreleases itself.
However, I had to add a ClearController() method to the bridge to make
sure close wasn't being called twice (either close or OnDialogClose()
in the delegate could be the initial entry point, so they call each
other).

>
> http://codereview.chromium.org/344008/diff/2003/2011
> File chrome/browser/cocoa/html_dialog_window_controller.mm (right):
>
> http://codereview.chromium.org/344008/diff/2003/2011#newcode208
> Line 208: if (window.get() == nil) {
> usually would be if (!window.get()) {

Fixed.

> http://codereview.chromium.org/344008/diff/2003/2011#newcode212
> Line 212: if (self == nil) {
> usually it would be if (!self) {

Fixed.

> http://codereview.chromium.org/344008/diff/2003/2011#newcode218
> Line 218: parent_window_ = parent_window;
> you don't appear to be using parent_window_ anywhere that I can see. Any
> reason to have it?

Removed.  May add back in later when I implement modal windows.

> http://codereview.chromium.org/344008/diff/2003/2011#newcode245
> Line 245: delegate_->OnDialogClosed("");
> Definitely need to autorelease ourselves here. Details here:
>
http://developer.apple.com/mac/library/DOCUMENTATION/Cocoa/Conceptual/Documen...
>
> http://codereview.chromium.org/344008/diff/2003/2012

Done, per above.

> http://codereview.chromium.org/344008/diff/2003/2012#newcode37
> Line 37: class HtmlDialogWindowControllerTest : public
> BrowserWithTestWindowTest {
> Not needed as part of this review, but I need to get
> BrowserWithTestWindowTest working with CocoaTest.

What do you mean?

> http://code.google.com/p/chromium/issues/detail?id=26133
>
> http://codereview.chromium.org/344008/diff/2003/2012#newcode62
> Line 62: scoped_nsobject<HtmlDialogWindowController>
> html_dialog_window_controller_;
> don't wrap NSWindowController's in scoped_nsobjects.

Done.

> http://codereview.chromium.org/344008/diff/2003/2012#newcode120
> Line 120: [html_dialog_window_controller_ retain];
> we should need a retain here. Something is wrong if you have a retain.
> close should be autoreleasing us properly.

Fixed.  Also reduced to one test case.

Please take another look.

-- 
Frederick Akalin
Software Engineer

[dmac, 2009-10-28 23:20:30]

http://codereview.chromium.org/344008/diff/8002/8003
File app/gfx/font_mac.mm (right):

http://codereview.chromium.org/344008/diff/8002/8003#newcode36
Line 36: height_ = [font boundingRectForFont].size.height;
not sure how expensive that is to calculate. [font ascender] - [font descender]
should give you the same value.

http://codereview.chromium.org/344008/diff/8002/8010
File chrome/browser/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/344008/diff/8002/8010#newcode84
Line 84: [controller_ close];
who aside from us calls OnDialogClosed?

I'm confused the controller_ owns the delegate bridge, but it appears to be
releasing the controller_? Who owns whom?

http://codereview.chromium.org/344008/diff/8002/8011
File chrome/browser/cocoa/html_dialog_window_controller_unittest.mm (right):

http://codereview.chromium.org/344008/diff/8002/8011#newcode68
Line 68: base::ScopedNSAutoreleasePool release_pool;
Could you mark this with a TODO(dmaclach) remove? Unfortunately just putting an
autorelease pool there does not always destroy the window controller. CocoaTest
is very careful about making sure that all the windows are shut down properly
hence my earlier comment about making sure that BrowserWithTestWindowTest
inherited from CocoaTest properly. Once I have
http://code.google.com/p/chromium/issues/detail?id=26133 fixed, this should be
removed.

[dmac, 2009-10-29 00:37:33]

http://codereview.chromium.org/344008/diff/8002/8003
File app/gfx/font_mac.mm (right):

http://codereview.chromium.org/344008/diff/8002/8003#newcode36
Line 36: height_ = [font boundingRectForFont].size.height;
On 2009/10/28 23:20:30, dmac wrote:
> not sure how expensive that is to calculate. [font ascender] - [font
descender]
> should give you the same value.

You want to use [font ascender] - [font descender]. I have done a lot of
research that is to large to put here. I'll write up a blog post on it and post
it somewhere, and link this to it. Suffice it to say I doubt any text editor on
the Mac handles it right. To match up with what Windows and Skia are doing (and
work for the majority) of cases you want to use [font ascender] - [font
descender] and not the bounding box.

But... xHeight is DEFINITELY wrong ;-)

[akalin (wrong akalin), 2009-10-29 05:35:02]

Hmm.  From my experiments ascender - descender didn't match results I
was getting on windows (which is supposedly just ascender - descender
itself).  And it produces a too-small dialog box for the sync setup
wizard (which calculates the size based on expected # of
lines/characters * avg width/height).  If you want, we can sit down
tomorrow and I can show you.

On Wed, Oct 28, 2009 at 5:37 PM,  <dmaclach@chromium.org> wrote:
>
> http://codereview.chromium.org/344008/diff/8002/8003
> File app/gfx/font_mac.mm (right):
>
> http://codereview.chromium.org/344008/diff/8002/8003#newcode36
> Line 36: height_ = [font boundingRectForFont].size.height;
> On 2009/10/28 23:20:30, dmac wrote:
>>
>> not sure how expensive that is to calculate. [font ascender] - [font
>
> descender]
>>
>> should give you the same value.
>
> You want to use [font ascender] - [font descender]. I have done a lot of
> research that is to large to put here. I'll write up a blog post on it
> and post it somewhere, and link this to it. Suffice it to say I doubt
> any text editor on the Mac handles it right. To match up with what
> Windows and Skia are doing (and work for the majority) of cases you want
> to use [font ascender] - [font descender] and not the bounding box.
>
> But... xHeight is DEFINITELY wrong ;-)
>
> http://codereview.chromium.org/344008
>



-- 
Frederick Akalin
Software Engineer

[akalin (wrong akalin), 2009-10-29 09:29:21]

Also, we can spin this off into its own changelist, leaving the rest
to be reviewed separately.

On Wed, Oct 28, 2009 at 10:34 PM, Fred Akalin <akalin@google.com> wrote:
> Hmm. =C2=A0From my experiments ascender - descender didn't match results =
I
> was getting on windows (which is supposedly just ascender - descender
> itself). =C2=A0And it produces a too-small dialog box for the sync setup
> wizard (which calculates the size based on expected # of
> lines/characters * avg width/height). =C2=A0If you want, we can sit down
> tomorrow and I can show you.
>
> On Wed, Oct 28, 2009 at 5:37 PM, =C2=A0<dmaclach@chromium.org> wrote:
>>
>> http://codereview.chromium.org/344008/diff/8002/8003
>> File app/gfx/font_mac.mm (right):
>>
>> http://codereview.chromium.org/344008/diff/8002/8003#newcode36
>> Line 36: height_ =3D [font boundingRectForFont].size.height;
>> On 2009/10/28 23:20:30, dmac wrote:
>>>
>>> not sure how expensive that is to calculate. [font ascender] - [font
>>
>> descender]
>>>
>>> should give you the same value.
>>
>> You want to use [font ascender] - [font descender]. I have done a lot of
>> research that is to large to put here. I'll write up a blog post on it
>> and post it somewhere, and link this to it. Suffice it to say I doubt
>> any text editor on the Mac handles it right. To match up with what
>> Windows and Skia are doing (and work for the majority) of cases you want
>> to use [font ascender] - [font descender] and not the bounding box.
>>
>> But... xHeight is DEFINITELY wrong ;-)
>>
>> http://codereview.chromium.org/344008
>>
>
>
>
> --
> Frederick Akalin
> Software Engineer
>



--=20
Frederick Akalin
Software Engineer

[dmaclach1, 2009-10-29 14:56:52]

Sounds good. I'll be back in KIR on Friday. That should give me time  
to do a write up on this.

Cheers,
Dave

On Oct 29, 2009, at 2:28 AM, Fred Akalin wrote:

> Also, we can spin this off into its own changelist, leaving the rest
> to be reviewed separately.
>
> On Wed, Oct 28, 2009 at 10:34 PM, Fred Akalin <akalin@google.com>  
> wrote:
>> Hmm.  From my experiments ascender - descender didn't match results I
>> was getting on windows (which is supposedly just ascender - descender
>> itself).  And it produces a too-small dialog box for the sync setup
>> wizard (which calculates the size based on expected # of
>> lines/characters * avg width/height).  If you want, we can sit down
>> tomorrow and I can show you.
>>
>> On Wed, Oct 28, 2009 at 5:37 PM,  <dmaclach@chromium.org> wrote:
>>>
>>> http://codereview.chromium.org/344008/diff/8002/8003
>>> File app/gfx/font_mac.mm (right):
>>>
>>> http://codereview.chromium.org/344008/diff/8002/8003#newcode36
>>> Line 36: height_ = [font boundingRectForFont].size.height;
>>> On 2009/10/28 23:20:30, dmac wrote:
>>>>
>>>> not sure how expensive that is to calculate. [font ascender] -  
>>>> [font
>>>
>>> descender]
>>>>
>>>> should give you the same value.
>>>
>>> You want to use [font ascender] - [font descender]. I have done a  
>>> lot of
>>> research that is to large to put here. I'll write up a blog post  
>>> on it
>>> and post it somewhere, and link this to it. Suffice it to say I  
>>> doubt
>>> any text editor on the Mac handles it right. To match up with what
>>> Windows and Skia are doing (and work for the majority) of cases  
>>> you want
>>> to use [font ascender] - [font descender] and not the bounding box.
>>>
>>> But... xHeight is DEFINITELY wrong ;-)
>>>
>>> http://codereview.chromium.org/344008
>>>
>>
>>
>>
>> --
>> Frederick Akalin
>> Software Engineer
>>
>
>
>
> -- 
> Frederick Akalin
> Software Engineer

[pink (ping after 24hrs), 2009-10-29 15:57:01]

Please, let's spin off the gfx change to another CL.

Let us know when you're fixed the other issues and we'll re-review. Thanks!

On Thu, Oct 29, 2009 at 10:56 AM, Dave MacLachlan <dmaclach@google.com> wro=
te:
> Sounds good. I'll be back in KIR on Friday. That should give me time to d=
o a
> write up on this.
>
> Cheers,
> Dave
>
> On Oct 29, 2009, at 2:28 AM, Fred Akalin wrote:
>
>> Also, we can spin this off into its own changelist, leaving the rest
>> to be reviewed separately.
>>
>> On Wed, Oct 28, 2009 at 10:34 PM, Fred Akalin <akalin@google.com> wrote:
>>>
>>> Hmm. =A0From my experiments ascender - descender didn't match results I
>>> was getting on windows (which is supposedly just ascender - descender
>>> itself). =A0And it produces a too-small dialog box for the sync setup
>>> wizard (which calculates the size based on expected # of
>>> lines/characters * avg width/height). =A0If you want, we can sit down
>>> tomorrow and I can show you.
>>>
>>> On Wed, Oct 28, 2009 at 5:37 PM, =A0<dmaclach@chromium.org> wrote:
>>>>
>>>> http://codereview.chromium.org/344008/diff/8002/8003
>>>> File app/gfx/font_mac.mm (right):
>>>>
>>>> http://codereview.chromium.org/344008/diff/8002/8003#newcode36
>>>> Line 36: height_ =3D [font boundingRectForFont].size.height;
>>>> On 2009/10/28 23:20:30, dmac wrote:
>>>>>
>>>>> not sure how expensive that is to calculate. [font ascender] - [font
>>>>
>>>> descender]
>>>>>
>>>>> should give you the same value.
>>>>
>>>> You want to use [font ascender] - [font descender]. I have done a lot =
of
>>>> research that is to large to put here. I'll write up a blog post on it
>>>> and post it somewhere, and link this to it. Suffice it to say I doubt
>>>> any text editor on the Mac handles it right. To match up with what
>>>> Windows and Skia are doing (and work for the majority) of cases you wa=
nt
>>>> to use [font ascender] - [font descender] and not the bounding box.
>>>>
>>>> But... xHeight is DEFINITELY wrong ;-)
>>>>
>>>> http://codereview.chromium.org/344008
>>>>
>>>
>>>
>>>
>>> --
>>> Frederick Akalin
>>> Software Engineer
>>>
>>
>>
>>
>> --
>> Frederick Akalin
>> Software Engineer
>
>



--=20
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[akalin (wrong akalin), 2009-10-29 19:08:19]

On Wed, Oct 28, 2009 at 4:20 PM,  <dmaclach@chromium.org> wrote:
>
> http://codereview.chromium.org/344008/diff/8002/8003
> File app/gfx/font_mac.mm (right):
>
> http://codereview.chromium.org/344008/diff/8002/8003#newcode36
> Line 36: height_ = [font boundingRectForFont].size.height;
> not sure how expensive that is to calculate. [font ascender] - [font
> descender] should give you the same value.

Reverted this change, added a TODO.

> http://codereview.chromium.org/344008/diff/8002/8010
> File chrome/browser/cocoa/html_dialog_window_controller.mm (right):
>
> http://codereview.chromium.org/344008/diff/8002/8010#newcode84
> Line 84: [controller_ close];
> who aside from us calls OnDialogClosed?

The HTMLDialogUI object, which is the user of the
HtmlDialogUIDelegate.  For example, in the sync setup wizard, whenever
the user clicks the "Cancel" button, OnDialogClosed gets called first.

> I'm confused the controller_ owns the delegate bridge, but it appears to
> be releasing the controller_? Who owns whom?

We get close events either from the standard UI elements (close
button, accelerator) or from the HTML content.  In either case, only
one of {OnDialogClosed, [controller_ close]} is called, but we want
both.  So I make them call each other, with some code to make sure
only one call of each function gets done.

I added some comments to explain this.

> http://codereview.chromium.org/344008/diff/8002/8011
> File chrome/browser/cocoa/html_dialog_window_controller_unittest.mm
> (right):
>
> http://codereview.chromium.org/344008/diff/8002/8011#newcode68
> Line 68: base::ScopedNSAutoreleasePool release_pool;
> Could you mark this with a TODO(dmaclach) remove? Unfortunately just
> putting an autorelease pool there does not always destroy the window
> controller. CocoaTest is very careful about making sure that all the
> windows are shut down properly hence my earlier comment about making
> sure that BrowserWithTestWindowTest inherited from CocoaTest properly.
> Once I have http://code.google.com/p/chromium/issues/detail?id=26133
> fixed, this should be removed.
>
> http://codereview.chromium.org/344008

Done.

Please take another look!

-- 
Frederick Akalin
Software Engineer

[dmac, 2009-10-29 20:33:10]

http://codereview.chromium.org/344008/diff/6005/7013
File chrome/browser/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/344008/diff/6005/7013#newcode79
Line 79: // been set to nil so it's safe to call [controller_ close].
would it make more sense to have a different method (eg named
WindowControllerClosed) that controller_ called as part of it's windowWillClose
call? Then this call would be reduced to just [controller_ close] (which seems
right for the bridge to do). Then in windowWillClose it would call
WindowControllerClosed on this object. That way you don't have to worry about
clearing the controller, and we're always going to go through the same basic
path. Hopefully I'm making sense here.

[akalin (wrong akalin), 2009-10-29 22:23:48]

On Thu, Oct 29, 2009 at 1:33 PM,  <dmaclach@chromium.org> wrote:
>
> http://codereview.chromium.org/344008/diff/6005/7013
> File chrome/browser/cocoa/html_dialog_window_controller.mm (right):
>
> http://codereview.chromium.org/344008/diff/6005/7013#newcode79
> Line 79: // been set to nil so it's safe to call [controller_ close].
> would it make more sense to have a different method (eg named
> WindowControllerClosed) that controller_ called as part of it's
> windowWillClose call? Then this call would be reduced to just
> [controller_ close] (which seems right for the bridge to do). Then in
> windowWillClose it would call WindowControllerClosed on this object.
> That way you don't have to worry about clearing the controller, and
> we're always going to go through the same basic path. Hopefully I'm
> making sense here.

Yes, I see what you mean.  Done!

Please take another look.

-- 
Frederick Akalin
Software Engineer

[dmac, 2009-10-29 23:10:24]

Alkalin...

Sorry... some stuff caught my eye that I missed before.

http://codereview.chromium.org/344008/diff/5010/8018
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/344008/diff/5010/8018#newcode299
Line 299: if (parent_window == nil) {
if (!parent_window)

http://codereview.chromium.org/344008/diff/5010/8020
File chrome/browser/cocoa/chrome_event_processing_window.mm (right):

http://codereview.chromium.org/344008/diff/5010/8020#newcode31
Line 31: if (cmdNum != -1) {
do we have a constant for -1 somewhere?

http://codereview.chromium.org/344008/diff/5010/8020#newcode37
Line 37: id<BrowserCommandExecutor> executor = delegate;
no need to do this conversion.

http://codereview.chromium.org/344008/diff/5010/8021
File chrome/browser/cocoa/chrome_event_processing_window_unittest.mm (right):

http://codereview.chromium.org/344008/diff/5010/8021#newcode71
Line 71: // -stub to satisfy the DCHECK.
not sure what the comment above means

http://codereview.chromium.org/344008/diff/5010/8021#newcode73
Line 73: // TODO(akalin): Figure out how to replace OCMOCK_ANY below with
newline before the comment.

http://codereview.chromium.org/344008/diff/5010/8022
File chrome/browser/cocoa/html_dialog_window_controller.h (right):

http://codereview.chromium.org/344008/diff/5010/8022#newcode37
Line 37: void WindowControllerClosed();
everything else is virtual any reason for this not to be?

http://codereview.chromium.org/344008/diff/5010/8022#newcode41
Line 41: virtual std::wstring GetDialogTitle() const;
do you want a const std::wstring reference here? or are you enforcing the copy?

http://codereview.chromium.org/344008/diff/5010/8022#newcode42
Line 42: virtual GURL GetDialogContentURL() const;
again, can we use a reference to avoid copy?

http://codereview.chromium.org/344008/diff/5010/8022#newcode46
Line 46: virtual std::string GetDialogArgs() const;
do you want a const std::string reference here? or are you enforcing the copy?

http://codereview.chromium.org/344008/diff/5010/8023
File chrome/browser/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/344008/diff/5010/8023#newcode93
Line 93: // We want to call close on controller_ only once, too.
mmentovai would ding you for using "We" in a comment ;-)

http://codereview.chromium.org/344008/diff/5010/8023#newcode124
Line 124: void HtmlDialogWindowDelegateBridge::ActivateContents(TabContents*
contents) {}
comment why there are empty? or should we have todos in here (for the next three
methods)

http://codereview.chromium.org/344008/diff/5010/8023#newcode195
Line 195: // TODO(akalin): Surely there must be a cleaner way to do this.
GTMNSAlignRectangles from GTMGeometryUtils.h? But you would have to add it to
the project. Up to you ;-)

[akalin (wrong akalin), 2009-10-29 23:38:50]

On Thu, Oct 29, 2009 at 4:10 PM,  <dmaclach@chromium.org> wrote:
> Alkalin...
>
> Sorry... some stuff caught my eye that I missed before.
>
>
> http://codereview.chromium.org/344008/diff/5010/8018
> File chrome/browser/cocoa/browser_window_cocoa.mm (right):
>
> http://codereview.chromium.org/344008/diff/5010/8018#newcode299
> Line 299: if (parent_window == nil) {
> if (!parent_window)

Fixed.

> http://codereview.chromium.org/344008/diff/5010/8020
> File chrome/browser/cocoa/chrome_event_processing_window.mm (right):
>
> http://codereview.chromium.org/344008/diff/5010/8020#newcode31
> Line 31: if (cmdNum != -1) {
> do we have a constant for -1 somewhere?

I checked, and nope.  All the comments for the called functions say
that -1 is returned if there is no command.

>
> http://codereview.chromium.org/344008/diff/5010/8020#newcode37
> Line 37: id<BrowserCommandExecutor> executor = delegate;
> no need to do this conversion.

Removed.

> http://codereview.chromium.org/344008/diff/5010/8021
> File chrome/browser/cocoa/chrome_event_processing_window_unittest.mm
> (right):
>
> http://codereview.chromium.org/344008/diff/5010/8021#newcode71
> Line 71: // -stub to satisfy the DCHECK.
> not sure what the comment above means

Clarified comment.

> http://codereview.chromium.org/344008/diff/5010/8021#newcode73
> Line 73: // TODO(akalin): Figure out how to replace OCMOCK_ANY below
> with
> newline before the comment.

Done.

> http://codereview.chromium.org/344008/diff/5010/8022
> File chrome/browser/cocoa/html_dialog_window_controller.h (right):
>
> http://codereview.chromium.org/344008/diff/5010/8022#newcode37
> Line 37: void WindowControllerClosed();
> everything else is virtual any reason for this not to be?

I don't expect this class to be inherited from, so I left it
non-virtual.  The other functions are virtual only because they're
overrides anyway.

> http://codereview.chromium.org/344008/diff/5010/8022#newcode41
> Line 41: virtual std::wstring GetDialogTitle() const;
> do you want a const std::wstring reference here? or are you enforcing
> the copy?
>
> http://codereview.chromium.org/344008/diff/5010/8022#newcode42
> Line 42: virtual GURL GetDialogContentURL() const;
> again, can we use a reference to avoid copy?
>
> http://codereview.chromium.org/344008/diff/5010/8022#newcode46
> Line 46: virtual std::string GetDialogArgs() const;
> do you want a const std::string reference here? or are you enforcing the
> copy?

The overridden functions have these signatures, so we can't change
them.  Changing the signatures would involve changing them for the
base class and other inheriting classes so I'd rather do that in a
separate CL if you want it done.

> http://codereview.chromium.org/344008/diff/5010/8023
> File chrome/browser/cocoa/html_dialog_window_controller.mm (right):
>
> http://codereview.chromium.org/344008/diff/5010/8023#newcode93
> Line 93: // We want to call close on controller_ only once, too.
> mmentovai would ding you for using "We" in a comment ;-)

Rewrote comment.

> http://codereview.chromium.org/344008/diff/5010/8023#newcode124
> Line 124: void
> HtmlDialogWindowDelegateBridge::ActivateContents(TabContents* contents)
> {}
> comment why there are empty? or should we have todos in here (for the
> next three methods)

Added comment at the top explaining empty function bodies.

> http://codereview.chromium.org/344008/diff/5010/8023#newcode195
> Line 195: // TODO(akalin): Surely there must be a cleaner way to do
> this.
> GTMNSAlignRectangles from GTMGeometryUtils.h? But you would have to add
> it to the project. Up to you ;-)

[window center] does almost what I want but it centers it w.r.t. the
screen.  On the other hand, maybe that's what we want, even for the
Windows version.  I'd rather figure this out in some other CL. :)

Please take another look.

-- 
Frederick Akalin
Software Engineer

[dmac, 2009-10-29 23:43:00]

LGTM

may want to add a code clean up bug to change those routines that return objects
that are having their copy constructors called.

[akalin (wrong akalin), 2009-10-30 01:43:35]

On Wed, Oct 28, 2009 at 11:39 AM,  <pinkerton@chromium.org> wrote:
> http://codereview.chromium.org/344008/diff/4011/3019#newcode234
> Line 234: // TODO(akalin): add accelerator for ESC to close the dialog
> box.
> you should just be able to implement -(void)cancel:(id)sender and get
> the message automatically.

I found the docs for this and tried it.  However, barring some stupid
mistake I'm making, it doesn't work, perhaps because the HTML dialog
is eating the keystroke.  In any case, I'd like to do this in a
separate CL.

-- 
Frederick Akalin
Software Engineer

[pink (ping after 24hrs), 2009-10-30 18:51:23]

LGTM

As far as -cancel working, it should, we do it in several other places. I think
you may need to hook it up such that the window controller is the delegate of
the window object. It's fine to do that in a followup cl though.

http://codereview.chromium.org/344008/diff/3038/4025
File chrome/browser/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/344008/diff/3038/4025#newcode195
Line 195: // Put the dialog box in the center of the window.
[window center] is probably what you want, the mac doesn't have the concept of
"centering a dialog in a window" because window-modals are sheets and app-modals
are centered on the screen.