Use native Cocoa tooltips instead of Mozilla's ToolTip class.

Use native Cocoa tooltips instead of Mozilla's ToolTip class.
This makes tooltips behave more normally, and fixes some of the bugs.
BUG=15655
TEST=none

[pink (ping after 24hrs), 2009-07-02 19:58:23]

http://codereview.chromium.org/150188/diff/8/1007
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/150188/diff/8/1007#newcode39
Line 39: id trackingRectOwner_;
can you describe how these are used? Also, are they strong or weak references?
They appear weak from the usage, which deserve a comment (default non-IBOutlet
ownership is assumed to be strong).

http://codereview.chromium.org/150188/diff/8/1008
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/8/1008#newcode537
Line 537: methods. --snej 7/1/09
i understand we're copying their code, but we really should make an effort to
document it (function-level comments, etc).

http://codereview.chromium.org/150188/diff/8/1008#newcode542
Line 542: #define TRACKING_RECT_TAG 0xBADFACE
const int? or maybe enum? we try to avoid #define for constants.

http://codereview.chromium.org/150188/diff/8/1008#newcode548
Line 548: {
brace goes on the previous line with the method decl.

http://codereview.chromium.org/150188/diff/8/1008#newcode555
Line 555: - (NSTrackingRectTag)_addTrackingRect:(NSRect)rect
what's the difference between these two methods? I don't see |tag| being used at
all. Am I missing something obvious? If this is to override some NSView internal
method, please add a comment to that effect.

http://codereview.chromium.org/150188/diff/8/1008#newcode560
Line 560: {
brace on previous line. same for the rest of these methods below.

http://codereview.chromium.org/150188/diff/8/1008#newcode606
Line 606: int i;
put decl inside of |for|

[Evan Martin, 2009-07-02 20:05:29]

http://codereview.chromium.org/150188/diff/8/1008
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/8/1008#newcode531
Line 531: /* Below is the nasty tooltip stuff -- copied from WebKit's
WebHTMLView.mm.
When we copy from WebKit, do we need to keep it in a separate file due to the
different license?

[Jens, 2009-07-02 22:38:08]

http://codereview.chromium.org/150188/diff/8/1007
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/150188/diff/8/1007#newcode39
Line 39: id trackingRectOwner_;
On 2009/07/02 19:58:23, pink wrote:
> can you describe how these are used? Also, are they strong or weak references?
> They appear weak from the usage, which deserve a comment (default non-IBOutlet
> ownership is assumed to be strong).

They seem to be keeping track of the owner/userdata of the tracking rect set up
by AppKit. The only one for which ownership is an issue is trackingRectOwner_;
the rest are not object references.

http://codereview.chromium.org/150188/diff/8/1008
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/8/1008#newcode531
Line 531: /* Below is the nasty tooltip stuff -- copied from WebKit's
WebHTMLView.mm.
On 2009/07/02 20:05:30, Evan Martin wrote:
> When we copy from WebKit, do we need to keep it in a separate file due to the
> different license?

Good point. I've just asked on IRC, and estade says it's fine, we've done it
before.

http://codereview.chromium.org/150188/diff/8/1008#newcode537
Line 537: methods. --snej 7/1/09
On 2009/07/02 19:58:23, pink wrote:
> i understand we're copying their code, but we really should make an effort to
> document it (function-level comments, etc).

I was trying to leave it alone as much as possible to make it easier to merge in
any future changes from the original file. But I can clean it up if you think
that's best.

http://codereview.chromium.org/150188/diff/8/1008#newcode542
Line 542: #define TRACKING_RECT_TAG 0xBADFACE
On 2009/07/02 19:58:23, pink wrote:
> const int? or maybe enum? we try to avoid #define for constants.

Done.

http://codereview.chromium.org/150188/diff/8/1008#newcode548
Line 548: {
On 2009/07/02 19:58:23, pink wrote:
> brace goes on the previous line with the method decl.

Done.

http://codereview.chromium.org/150188/diff/8/1008#newcode555
Line 555: - (NSTrackingRectTag)_addTrackingRect:(NSRect)rect
On 2009/07/02 19:58:23, pink wrote:
> what's the difference between these two methods? I don't see |tag| being used
at
> all. Am I missing something obvious? If this is to override some NSView
internal
> method, please add a comment to that effect.

Several of these appear to be overrides of internal NSView methods, yes. I'll
add comments.

http://codereview.chromium.org/150188/diff/8/1008#newcode560
Line 560: {
On 2009/07/02 19:58:23, pink wrote:
> brace on previous line. same for the rest of these methods below.

Done.

http://codereview.chromium.org/150188/diff/8/1008#newcode606
Line 606: int i;
On 2009/07/02 19:58:23, pink wrote:
> put decl inside of |for|

Done.

[pink (ping after 24hrs), 2009-07-06 15:31:36]

LGTM with nits fixed. If you can't figure out what the overrides are doing,
that's fine, I guess.

http://codereview.chromium.org/150188/diff/3002/3004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/3002/3004#newcode544
Line 544: // Override of a public NSView method, replacing the inherited
functionality
Clearly :-) But why?

http://codereview.chromium.org/150188/diff/3002/3004#newcode583
Line 583: // Override of a public NSView method, replacing the inherited
functionality
period at end of sentence.

http://codereview.chromium.org/150188/diff/3002/3004#newcode585
Line 585: {
fix brace.

http://codereview.chromium.org/150188/diff/3002/3004#newcode616
Line 616: // Sends a fake NSMouseExited event to the view for its current
tracking rect
period at end of sentence.

http://codereview.chromium.org/150188/diff/3002/3004#newcode632
Line 632: // Sends a fake NSMouseEntered event to the view for its current
tracking rect
period at end of sentence.

[Jens, 2009-07-06 17:19:53]

http://codereview.chromium.org/150188/diff/3002/3004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/3002/3004#newcode544
Line 544: // Override of a public NSView method, replacing the inherited
functionality
On 2009/07/06 15:31:36, pink wrote:
> Clearly :-) But why?

Added more explanation of the overrides to the block-comment above.

http://codereview.chromium.org/150188/diff/3002/3004#newcode583
Line 583: // Override of a public NSView method, replacing the inherited
functionality
On 2009/07/06 15:31:36, pink wrote:
> period at end of sentence.

Done.

http://codereview.chromium.org/150188/diff/3002/3004#newcode585
Line 585: {
On 2009/07/06 15:31:36, pink wrote:
> fix brace.

Done.

http://codereview.chromium.org/150188/diff/3002/3004#newcode616
Line 616: // Sends a fake NSMouseExited event to the view for its current
tracking rect
On 2009/07/06 15:31:36, pink wrote:
> period at end of sentence.

Done.

http://codereview.chromium.org/150188/diff/3002/3004#newcode632
Line 632: // Sends a fake NSMouseEntered event to the view for its current
tracking rect
On 2009/07/06 15:31:36, pink wrote:
> period at end of sentence.

Done.

[pink (ping after 24hrs), 2009-07-06 19:08:23]

LGTM

[Mark Mentovai, 2009-07-06 19:45:20]

http://codereview.chromium.org/150188/diff/3008/3010
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/3008/3010#newcode550
Line 550: /* Below is the nasty tooltip stuff -- copied from WebKit's
WebHTMLView.mm
This file uses //.  You use // elsewhere.  Use // here.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Commen...

http://codereview.chromium.org/150188/diff/3008/3010#newcode563
Line 563: // Any non-zero value will do,
Why's this line end so early?  Reflow the comment.

http://codereview.chromium.org/150188/diff/3008/3010#newcode565
Line 565: const NSTrackingRectTag TRACKING_RECT_TAG = 0xBADFACE;
Make this static.  We don't need an external symbol for this.

Also, use proper constant naming: kTrackingRectTag.  CAPITAL_LETTERS are for
macros, which this is not. 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...

http://codereview.chromium.org/150188/diff/3008/3010#newcode573
Line 573: CHECK(trackingRectOwner_ == nil);
Shouldn't these be DCHECKs?  CHECKs operate in non-debug mode and will take the
app down.  Seems draconian for tooltips.  (Throughout.)

[Jens, 2009-07-06 20:52:51]

http://codereview.chromium.org/150188/diff/3008/3010
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/3008/3010#newcode550
Line 550: /* Below is the nasty tooltip stuff -- copied from WebKit's
WebHTMLView.mm
On 2009/07/06 19:45:20, Mark Mentovai wrote:
> This file uses //.  You use // elsewhere.  Use // here.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Commen...

I consistently use /**/ for multi-line block comments, because it's a lot easier
to re-flow the text without the punctuation at the start of every line.

http://codereview.chromium.org/150188/diff/3008/3010#newcode563
Line 563: // Any non-zero value will do,
On 2009/07/06 19:45:20, Mark Mentovai wrote:
> Why's this line end so early?  Reflow the comment.

I didn't write this, it's copied from WebKit. I imagine the author broke the
text on phrase boundaries, which is not uncommon. Since it's going to be two
lines regardless of how it's broken, I prefer to leave it the way it is.

http://codereview.chromium.org/150188/diff/3008/3010#newcode565
Line 565: const NSTrackingRectTag TRACKING_RECT_TAG = 0xBADFACE;
On 2009/07/06 19:45:20, Mark Mentovai wrote:
> Make this static.  We don't need an external symbol for this.
> 
> Also, use proper constant naming: kTrackingRectTag.  CAPITAL_LETTERS are for
> macros, which this is not. 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...

Done.

http://codereview.chromium.org/150188/diff/3008/3010#newcode573
Line 573: CHECK(trackingRectOwner_ == nil);
On 2009/07/06 19:45:20, Mark Mentovai wrote:
> Shouldn't these be DCHECKs?  CHECKs operate in non-debug mode and will take
the
> app down.  Seems draconian for tooltips.  (Throughout.)

Done.

[brettw, 2009-07-06 21:01:37]

http://codereview.chromium.org/150188/diff/3008/3010
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/3008/3010#newcode550
Line 550: /* Below is the nasty tooltip stuff -- copied from WebKit's
WebHTMLView.mm
Our team uniformly uses // for all comments except for when you need comments in
the middle of a line (rare) or your commenting out a whole bunch of code
temporarily.

Please use //

http://codereview.chromium.org/150188/diff/3008/3010#newcode563
Line 563: // Any non-zero value will do,
Our team would always do this a certain way, and your way of doing it seems
clearly wrong to me, even though it doesn't "matter." Please trust the opinions
of the more experienced engineers like Mark.

[Jens, 2009-07-06 21:12:46]

http://codereview.chromium.org/150188/diff/3008/3010
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150188/diff/3008/3010#newcode550
Line 550: /* Below is the nasty tooltip stuff -- copied from WebKit's
WebHTMLView.mm
On 2009/07/06 21:01:37, brettw wrote:
> Our team uniformly uses // for all comments except for when you need comments
in
> the middle of a line (rare) or your commenting out a whole bunch of code
> temporarily.
> 
> Please use //

Done.

http://codereview.chromium.org/150188/diff/3008/3010#newcode563
Line 563: // Any non-zero value will do,
On 2009/07/06 21:01:37, brettw wrote:
> Our team would always do this a certain way, and your way of doing it seems
> clearly wrong to me, even though it doesn't "matter." Please trust the
opinions
> of the more experienced engineers like Mark.

Done.

http://codereview.chromium.org/150188/diff/3008/3010#newcode573
Line 573: CHECK(trackingRectOwner_ == nil);
On 2009/07/06 20:52:51, Jens wrote:
> On 2009/07/06 19:45:20, Mark Mentovai wrote:
> > Shouldn't these be DCHECKs?  CHECKs operate in non-debug mode and will take
> the
> > app down.  Seems draconian for tooltips.  (Throughout.)
> 
> Done.

Done.

[Mark Mentovai, 2009-07-06 21:17:56]

lgtm

[pink (ping after 24hrs), 2009-07-07 11:54:02]

pushed as r20019