[Mac] The autocomplete popup now gets its position from the toolbar controlle...

[Mac] The autocomplete popup now gets its position from the toolbar controller,
rather than simply growing its width by 2*height.

BUG=None
TEST=The autocomplete popup should continue to appear in the same location.

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

[Scott Hess - ex-Googler, 2009-08-26 17:53:00]

As we backfill the NOTIMPLEMENTED() routines in LocationBarViewMac, I bet it
will come closer and closer to the toolbar controller.  It may be that it could
keep a reference, and implement AutocompletePopupPositioner directly :-).

http://codereview.chromium.org/173439/diff/1003/1011
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/173439/diff/1003/1011#newcode60
Line 60: popupPositioner_;
Style seems poor, here.

How wed are you to the namespace thing?  Seems somewhat arbitrary, the other
platforms aren't doing it for this guy, the Mac suffix would seem sufficient,
not having it would fix this style nit :-).

Hmm.  You could also use scoped_ptr<AutocompletePopupPositioner>, since you only
use it to get deallocation right, and then you could put the implementation in
an anonymous namespace.  Or does that get screwed up because
AutocompletePopupPositioner doesn't have a virtual destructor?  [And, if so,
should it?]

http://codereview.chromium.org/173439/diff/1003/1012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/173439/diff/1003/1012#newcode406
Line 406: NSMaxX([goButton_ frame]) - minX, 0);
Rather than constructing via NSMakeRect, could you use NSUnionRect() on the
three frames?

[rohitrao (ping after 24h), 2009-08-26 20:23:42]

http://codereview.chromium.org/173439/diff/1003/1011
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/173439/diff/1003/1011#newcode60
Line 60: popupPositioner_;
On 2009/08/26 17:53:00, shess wrote:
> Style seems poor, here.
> 
> How wed are you to the namespace thing?  Seems somewhat arbitrary, the other
> platforms aren't doing it for this guy, the Mac suffix would seem sufficient,
> not having it would fix this style nit :-).
> 
> Hmm.  You could also use scoped_ptr<AutocompletePopupPositioner>, since you
only
> use it to get deallocation right, and then you could put the implementation in
> an anonymous namespace.  Or does that get screwed up because
> AutocompletePopupPositioner doesn't have a virtual destructor?  [And, if so,
> should it?]

Changed to use the base class here, and I put the subclass into an anonymous
namespace in the .mm file.  I also gave both the base class and subclass virtual
destructors, for good measure.

http://codereview.chromium.org/173439/diff/1003/1012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/173439/diff/1003/1012#newcode406
Line 406: NSMaxX([goButton_ frame]) - minX, 0);
On 2009/08/26 17:53:00, shess wrote:
> Rather than constructing via NSMakeRect, could you use NSUnionRect() on the
> three frames?
> 

I kinda prefer it this way, since I know exactly what I'm getting.

[Scott Hess - ex-Googler, 2009-08-26 20:38:14]

lgtm unless you want to argue about these comments.

http://codereview.chromium.org/173439/diff/1003/1012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/173439/diff/1003/1012#newcode406
Line 406: NSMaxX([goButton_ frame]) - minX, 0);
On 2009/08/26 20:23:42, rohitrao wrote:
> On 2009/08/26 17:53:00, shess wrote:
> > Rather than constructing via NSMakeRect, could you use NSUnionRect() on the
> > three frames?
> > 
> 
> I kinda prefer it this way, since I know exactly what I'm getting.

OK.  Then add some DCHECKs to make sure that your assumptions hold (left of
startButton_ is really left of locationBar_ and goButton_, right of goButton_ is
really right of the other two).

http://codereview.chromium.org/173439/diff/27/37#newcode58
Line 58: };
As I mentioned earlier, jrg's ghost would really appreciate a unit test for
this...

I'm completely open to busting it out of any namespaces and testing it, but if
there's already a toolbar_controller_unittest.mm, then maybe it would be easier
to just ask the class for its positioner via a private objc method and probe it
that way, then you can keep the shiny namespace!

[rohitrao (ping after 24h), 2009-08-26 21:47:20]

http://codereview.chromium.org/173439/diff/1003/1012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/173439/diff/1003/1012#newcode406
Line 406: NSMaxX([goButton_ frame]) - minX, 0);
> OK.  Then add some DCHECKs to make sure that your assumptions hold (left of
> startButton_ is really left of locationBar_ and goButton_, right of goButton_
is
> really right of the other two).

DCHECKs added.

http://codereview.chromium.org/173439/diff/27/37#newcode58
Line 58: };
On 2009/08/26 20:38:14, shess wrote:
> As I mentioned earlier, jrg's ghost would really appreciate a unit test for
> this...
> 
> I'm completely open to busting it out of any namespaces and testing it, but if
> there's already a toolbar_controller_unittest.mm, then maybe it would be
easier
> to just ask the class for its positioner via a private objc method and probe
it
> that way, then you can keep the shiny namespace!

Forgot to mention it before -- I added a unittest for the ToolbarController
method.  I'm not going to bother testing the C++ proxy class, since it doesn't
do anything interesting.

[Scott Hess - ex-Googler, 2009-08-27 00:01:29]

lgtm

http://codereview.chromium.org/173439/diff/27/37
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/173439/diff/27/37#newcode58
Line 58: }  // namespace
On 2009/08/26 21:47:20, rohitrao wrote:
> On 2009/08/26 20:38:14, shess wrote:
> > As I mentioned earlier, jrg's ghost would really appreciate a unit test for
> > this...
> > 
> > I'm completely open to busting it out of any namespaces and testing it, but
if
> > there's already a toolbar_controller_unittest.mm, then maybe it would be
> easier
> > to just ask the class for its positioner via a private objc method and probe
> it
> > that way, then you can keep the shiny namespace!
> 
> Forgot to mention it before -- I added a unittest for the ToolbarController
> method.  I'm not going to bother testing the C++ proxy class, since it doesn't
> do anything interesting.

Bravo on the unit test!

The C++ proxy class is code, thus testing it presumably matters to code
coverage.  Just sayin'.