[rAc] [OSX] Set tooltip bubble arrow's anchor point based on the icon location.

[rAc] [OSX] Set tooltip bubble arrow's anchor point based on the icon location.

The bubble should always be between the horizontal edges of the dialog.

BUG=325482
TEST=(see bug)
R=groby@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241085

[groby-ooo-7-16, 2013-12-13 01:35:49]

https://codereview.chromium.org/113763002/diff/1/chrome/browser/ui/cocoa/auto...
File chrome/browser/ui/cocoa/autofill/autofill_notification_controller.mm
(right):

https://codereview.chromium.org/113763002/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/autofill_notification_controller.mm:146:
initWithArrowLocation:info_bubble::kTopRight]);
That's a lot of code for a single kTopCenter... Why not

  [[tooltipController_ bubble] setArrowLocation:info_bubble::kTopCenter]; 

in this file and be done with it?

[Ilya Sherman, 2013-12-13 02:17:07]

https://codereview.chromium.org/113763002/diff/1/chrome/browser/ui/cocoa/auto...
File chrome/browser/ui/cocoa/autofill/autofill_notification_controller.mm
(right):

https://codereview.chromium.org/113763002/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/autofill_notification_controller.mm:146:
initWithArrowLocation:info_bubble::kTopRight]);
On 2013/12/13 01:35:49, groby wrote:
> That's a lot of code for a single kTopCenter... Why not
> 
>   [[tooltipController_ bubble] setArrowLocation:info_bubble::kTopCenter]; 
> 
> in this file and be done with it? 

Well, because the tooltip controller doesn't always have a bubble showing, so at
a minimum we need to cache the bubble position in the AutofillTooltipController
class.  That basically leaves two remaining parts to the question: Can I just
leave the autofill_bubble_controller.* files alone, and can I leave
autofill_main_container.mm alone by not changing the init method.

To the first point: I could leave it alone, but the current structure seems more
explicit to me, which I think is a positive.  Still, I could remove that part if
you'd really like.

To the second: I could move the setter out of the -init: method, but then new
users of the bubble -- such as my CL to add a (?) icon to the phone number field
-- might forget to set the correct arrow location.  I could revert this part of
the change too, but I would push back a little harder ;)

[groby-ooo-7-16, 2013-12-14 01:33:21]

Ideally, the tooltip controller would figure out by itself where to put the
arrow - center if the content doesn't overlap the window, otherwise use
right/left to make it fit.

[groby-ooo-7-16, 2013-12-14 01:33:45]

On 2013/12/14 01:33:21, groby wrote:
> Ideally, the tooltip controller would figure out by itself where to put the
> arrow - center if the content doesn't overlap the window, otherwise use
> right/left to make it fit.

But overall, the current version is LGTM - we can do that as a cleanup pass
later on.

[commit-bot: I haz the power, 2013-12-14 02:03:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/113763002/1

[commit-bot: I haz the power, 2013-12-14 02:55:54]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-12-14 03:18:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/113763002/1

[commit-bot: I haz the power, 2013-12-14 05:27:02]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-14 05:31:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/113763002/1

[commit-bot: I haz the power, 2013-12-14 07:23:00]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 20:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/113763002/1

[commit-bot: I haz the power, 2013-12-16 22:26:28]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 22:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/113763002/1

[commit-bot: I haz the power, 2013-12-17 00:28:02]

Change committed as 241085