[Autofill] (Mostly) fix Autofill popup positioning on multiple monitors.

[Autofill] (Mostly) fix Autofill popup positioning on multiple monitors.

The coordinate (0, 0) always refers to the primary screen, rather than the
screen containing the browser window for the popup.

BUG=246468
TEST=Autofill popup should be correctly positioned on multiple monitors, except
     possibly when a retina display is involved.

R=shess@chromium.org

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

[Scott Hess - ex-Googler, 2013-06-26 00:46:21]

I don't understand the comment.  The primary screen determines the 0,0 location,
but you're accessing the height of the primary screen, which I don't think
includes the union of everything.

Could you toss me some background on what the incoming frame is relative to?  If
it's relative to the window's screen, say to the top left, then I think you'd
want something like:
  NSScreen* screen = [[controller_->container_view() window] screen];	
  frame.origin.y = NSMaxY([screen frame]) - NSMaxY(frame);
I think.

[Ilya Sherman, 2013-06-26 00:51:27]

On 2013/06/26 00:46:21, shess wrote:
> I don't understand the comment.  The primary screen determines the 0,0
location,
> but you're accessing the height of the primary screen, which I don't think
> includes the union of everything.
> 
> Could you toss me some background on what the incoming frame is relative to? 
If
> it's relative to the window's screen, say to the top left, then I think you'd
> want something like:
>   NSScreen* screen = [[controller_->container_view() window] screen];	
>   frame.origin.y = NSMaxY([screen frame]) - NSMaxY(frame);
> I think.

It looks like the coordinates that are being used here are relative to the
primary screen's (0, 0).  The controller calls Screen::GetDisplayNearestPoint()
to figure out which screen is relative, and you can see that in that
implementation, [[NSScreen screens] objectAtIndex:0] is the one that gets used:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/screen_mac....

Basically, the coordinates that we're getting in have nothing to do with the
"active" screen in any way.  They're always relative to a fixed point, which
happens to be the bottom-left of the first screen.

[Scott Hess - ex-Googler, 2013-06-27 18:30:50]

On 2013/06/26 00:51:27, Ilya Sherman wrote:
> On 2013/06/26 00:46:21, shess wrote:
> > I don't understand the comment.  The primary screen determines the 0,0
> location,
> > but you're accessing the height of the primary screen, which I don't think
> > includes the union of everything.
> > 
> > Could you toss me some background on what the incoming frame is relative to?

> If
> > it's relative to the window's screen, say to the top left, then I think
you'd
> > want something like:
> >   NSScreen* screen = [[controller_->container_view() window] screen];	
> >   frame.origin.y = NSMaxY([screen frame]) - NSMaxY(frame);
> > I think.
> 
> It looks like the coordinates that are being used here are relative to the
> primary screen's (0, 0).  The controller calls
Screen::GetDisplayNearestPoint()
> to figure out which screen is relative, and you can see that in that
> implementation, [[NSScreen screens] objectAtIndex:0] is the one that gets
used:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/screen_mac....
> 
> Basically, the coordinates that we're getting in have nothing to do with the
> "active" screen in any way.  They're always relative to a fixed point, which
> happens to be the bottom-left of the first screen.

The coordinates are relative to the origin of screen 0, but you're using the
height of that screen, here.  For a screen "above" screen 0, the NSMinY([screen
frame]) should be NSHeight([screen0 frame]) and NSMaxY([screen frame]) should be
way larger than NSHeight([screen0 frame]).  If the screen is "below" screen 0,
then those values should be negative.  So if |frame| is on a screen above or
below screen 0, calculations relative to NSHeight([screen0 frame]) don't make
logical sense.

I think.  Of course, since this is abstract coordinate transforms, it's entirely
possible to get to correct results via a logically-suspect sequence of
operations.  It just makes it harder to reason about things.

Does that make sense?  I'm just arguing from imagination, I haven't actually
setup my screens this way.

[Scott Hess - ex-Googler, 2013-06-27 21:55:58]

lgtm.

https://codereview.chromium.org/17733005/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right):

https://codereview.chromium.org/17733005/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:95: // Flip
coordinates back into Cocoa-land.
OK, per offline discussion, I think I see what you mean.  This comment maybe
could be expanded on the order of:
  // Flip from origin at top-left of screen 0 to origin at bottom-left of screen
0.
Mostly because it's not obvious from the local code.  It might also make sense
to phrase as:
  frame.origin.y = NSMaxY([screen frame]) - NSMaxY(frame);
to make it clear.

[Ilya Sherman, 2013-06-28 00:51:16]

https://codereview.chromium.org/17733005/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right):

https://codereview.chromium.org/17733005/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:95: // Flip
coordinates back into Cocoa-land.
On 2013/06/27 21:55:58, shess wrote:
> OK, per offline discussion, I think I see what you mean.  This comment maybe
> could be expanded on the order of:
>   // Flip from origin at top-left of screen 0 to origin at bottom-left of
screen
> 0.
> Mostly because it's not obvious from the local code.  It might also make sense
> to phrase as:
>   frame.origin.y = NSMaxY([screen frame]) - NSMaxY(frame);
> to make it clear.

Done.

[commit-bot: I haz the power, 2013-06-28 00:51:24]

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

[commit-bot: I haz the power, 2013-06-28 23:43:40]

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

[commit-bot: I haz the power, 2013-06-29 00:02:34]

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

[commit-bot: I haz the power, 2013-06-29 00:06:26]

Change committed as 209238