Mac: Use [NSArray firstObject] for [NSScreen screens]

Mac: Use [NSArray firstObject] for [NSScreen screens]

Crash reports indicate that the array is at times empty. Use firstObject
instead of objectAtIndex:0, because firstObject will return nil instead
of raising an exception.

BUG=529723
Committed: https://crrev.com/741debced2db082f4921d7a46f79d81e47caaada
Cr-Commit-Position: refs/heads/master@{#352915}

[ccameron, 2015-10-05 18:56:22]

ccameron@chromium.org changed reviewers:
+ avi@chromium.org

[ccameron, 2015-10-05 18:56:23]

Search-replace for this particular instance.

The calls to objectAtIndex:0 seem to be used for geometry computations (which
will get meaningless results when there is no screen) and tests to see if a
given screen is primary (which won't make a difference).

The firstObject method was added in 10.6.

[Avi (use Gerrit), 2015-10-05 19:09:05]

lgtm

[ccameron, 2015-10-05 19:17:24]

Thanks!

[ccameron, 2015-10-05 19:17:27]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-10-05 19:20:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380083005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380083005/1

[commit-bot: I haz the power, 2015-10-05 19:51:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-05 19:51:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2015-10-05 20:28:31]

Adding sky@ for stamp on chrome/browser/ui

[ccameron, 2015-10-07 00:48:27]

ccameron@chromium.org changed reviewers:
+ sky@chromium.org

[ccameron, 2015-10-07 00:48:28]

Adding sky@ for stamp, for real this time.

[sky, 2015-10-07 01:46:12]

LGTM

[sky, 2015-10-07 01:48:51]

Actually, NOT LGTM.
Yes, this means objectAtIndex won't crash, but it isn't obvious that you won't
crash soon there after. For example:
NSMaxY([[[NSScreen screens] firstObject] frame]);

What does this do if firstObject is null? [null frame] isn't going to return
anything meaningful. Will that mean NSMaxY crashes?

[ccameron, 2015-10-07 06:39:50]

On 2015/10/07 01:48:51, sky wrote:
> Actually, NOT LGTM.
> Yes, this means objectAtIndex won't crash, but it isn't obvious that you won't
> crash soon there after. For example:
> NSMaxY([[[NSScreen screens] firstObject] frame]);
> 
> What does this do if firstObject is null? [null frame] isn't going to return
> anything meaningful. Will that mean NSMaxY crashes?

Sending a message to nil in Objective C will do nothing and return a default
(zero) initialized return value.

So, in the case of 
  NSMaxY([[[NSScreen screens] firstObject] frame])
This will turn into
  NSMaxY([nil frame]) -> NSMaxY(NSMakeRect(0, 0, 0, 0)) -> 0

So, the math itself isn't "right" in the sense that we don't have correct screen
geometry (cause we have no screen), but we won't be throwing exceptions.

[sky, 2015-10-07 15:45:05]

On 2015/10/07 06:39:50, ccameron wrote:
> On 2015/10/07 01:48:51, sky wrote:
> > Actually, NOT LGTM.
> > Yes, this means objectAtIndex won't crash, but it isn't obvious that you
won't
> > crash soon there after. For example:
> > NSMaxY([[[NSScreen screens] firstObject] frame]);
> > 
> > What does this do if firstObject is null? [null frame] isn't going to return
> > anything meaningful. Will that mean NSMaxY crashes?
> 
> Sending a message to nil in Objective C will do nothing and return a default
> (zero) initialized return value.
> 
> So, in the case of 
>   NSMaxY([[[NSScreen screens] firstObject] frame])
> This will turn into
>   NSMaxY([nil frame]) -> NSMaxY(NSMakeRect(0, 0, 0, 0)) -> 0
> 
> So, the math itself isn't "right" in the sense that we don't have correct
screen
> geometry (cause we have no screen), but we won't be throwing exceptions.

How does a nill return value get converted into NSMakeRect(0, 0, 0, 0)?

[sky, 2015-10-07 15:45:36]

On 2015/10/07 15:45:05, sky wrote:
> On 2015/10/07 06:39:50, ccameron wrote:
> > On 2015/10/07 01:48:51, sky wrote:
> > > Actually, NOT LGTM.
> > > Yes, this means objectAtIndex won't crash, but it isn't obvious that you
> won't
> > > crash soon there after. For example:
> > > NSMaxY([[[NSScreen screens] firstObject] frame]);
> > > 
> > > What does this do if firstObject is null? [null frame] isn't going to
return
> > > anything meaningful. Will that mean NSMaxY crashes?
> > 
> > Sending a message to nil in Objective C will do nothing and return a default
> > (zero) initialized return value.
> > 
> > So, in the case of 
> >   NSMaxY([[[NSScreen screens] firstObject] frame])
> > This will turn into
> >   NSMaxY([nil frame]) -> NSMaxY(NSMakeRect(0, 0, 0, 0)) -> 0
> > 
> > So, the math itself isn't "right" in the sense that we don't have correct
> screen
> > geometry (cause we have no screen), but we won't be throwing exceptions.
> 
> How does a nill return value get converted into NSMakeRect(0, 0, 0, 0)?

But even so, if the screens can be empty shouldn't this code take a different
path?

[ccameron, 2015-10-07 16:37:13]

On 2015/10/07 15:45:36, sky wrote:
> On 2015/10/07 15:45:05, sky wrote:
> > On 2015/10/07 06:39:50, ccameron wrote:
> > > On 2015/10/07 01:48:51, sky wrote:
> > > > Actually, NOT LGTM.
> > > > Yes, this means objectAtIndex won't crash, but it isn't obvious that you
> > won't
> > > > crash soon there after. For example:
> > > > NSMaxY([[[NSScreen screens] firstObject] frame]);
> > > > 
> > > > What does this do if firstObject is null? [null frame] isn't going to
> return
> > > > anything meaningful. Will that mean NSMaxY crashes?
> > > 
> > > Sending a message to nil in Objective C will do nothing and return a
default
> > > (zero) initialized return value.
> > > 
> > > So, in the case of 
> > >   NSMaxY([[[NSScreen screens] firstObject] frame])
> > > This will turn into
> > >   NSMaxY([nil frame]) -> NSMaxY(NSMakeRect(0, 0, 0, 0)) -> 0
> > > 
> > > So, the math itself isn't "right" in the sense that we don't have correct
> > screen
> > > geometry (cause we have no screen), but we won't be throwing exceptions.
> > 
> > How does a nill return value get converted into NSMakeRect(0, 0, 0, 0)?

If you send a message to a nil object, it will return a zero-initialized return
value. NSRect is returned by value, and is initialized to zero when returned
from a message sent to a nil object.

> But even so, if the screens can be empty shouldn't this code take a different
> path?

The resulting rectangle (which is y-flipped at the origin) seems to be an
appropriate thing to do (certainly better than crashing).

[sky, 2015-10-07 17:52:04]

I'm not arguing that crashing is good. I'm arguing that if screens is empty it
seems like some of the code should do things differently. For example, the code
we've been talking about is:

int primary_display_height =
  NSMaxY([[[NSScreen screens] firstObject] frame]);
AppListServiceMac::FindAnchorPoint(... primary_display_height ...)

That code assumes it has a good height. If the height is 0 because the screens
is empty then do we even want to run that code? It likely isn't going to do what
we want, right?

I have similar concerns with the next file:
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm
We could end up here:
bounds.set_x(floor((NSWidth(main_screen_rect) - bounds.width()) / 2));
If main_screen_rect is 0 because screens is empty then the x is set very
negative. Do we really want that?

[sky, 2015-10-07 17:53:36]

On 2015/10/07 17:52:04, sky wrote:
> I'm not arguing that crashing is good. I'm arguing that if screens is empty it
> seems like some of the code should do things differently. For example, the
code
> we've been talking about is:
> 
> int primary_display_height =
>   NSMaxY([[[NSScreen screens] firstObject] frame]);
> AppListServiceMac::FindAnchorPoint(... primary_display_height ...)
> 
> That code assumes it has a good height. If the height is 0 because the screens
> is empty then do we even want to run that code? It likely isn't going to do
what
> we want, right?
> 
> I have similar concerns with the next file:
> chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm
> We could end up here:
> bounds.set_x(floor((NSWidth(main_screen_rect) - bounds.width()) / 2));
> If main_screen_rect is 0 because screens is empty then the x is set very
> negative. Do we really want that?

Just to continue on. So, yes, we obviously shouldn't crash, but blindly changing
to firstObject so that we don't crash in the screen call but have some other
weird failure later (and possibly a crash) seems equally wrong. Seems to me we
need to evaluate each usage and potentially do things differently if screens is
empty.

[ccameron, 2015-10-07 18:49:47]

On 2015/10/07 17:53:36, sky wrote:
> On 2015/10/07 17:52:04, sky wrote:
> > I'm not arguing that crashing is good. I'm arguing that if screens is empty
it
> > seems like some of the code should do things differently. For example, the
> code
> > we've been talking about is:
> > 
> > int primary_display_height =
> >   NSMaxY([[[NSScreen screens] firstObject] frame]);
> > AppListServiceMac::FindAnchorPoint(... primary_display_height ...)
> > 
> > That code assumes it has a good height. If the height is 0 because the
screens
> > is empty then do we even want to run that code? It likely isn't going to do
> what
> > we want, right?
> > 
> > I have similar concerns with the next file:
> > chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm
> > We could end up here:
> > bounds.set_x(floor((NSWidth(main_screen_rect) - bounds.width()) / 2));
> > If main_screen_rect is 0 because screens is empty then the x is set very
> > negative. Do we really want that?
> 
> blindly changing

I'm not sure that this characterization is particularly accurate or
constructive.

To clarify the point that I made in the first comment in the CL. I looked
through all of these instances that I changed, and they fall into two
categories: (1) "is equal to the primary" display and (2) y-flips (this include
the aforementioned FindAnchorPoint).

The first category is self-explanatory.

The second category may take a bit more explanation. The coordinate system for
windows in Cocoa has (0,0) as the lower-left corner of the primary screen.
Everybody wants (0,0) to be in the upper-right (and y to descend vertically), so
many people here wrote ~25 instances of the same function to convert coordinate
systems as "take point (x,y) and turn it into (x, primary_monitor_height - y)".
Arguably, I should go through and replace all of these instances with a single
set of helper functions (there are many many more, especially because every
NSView and NSWindow and CALayer have the same system).

Two concerns.

First concern (negatives): In the case that we have no primary screen, we'll end
up with these functions taking (x,y) and making it (x,-y), and there is some
cause for concern that "is a negative value acceptable -- would it cause things
to blow up?". Another thing about Cocoa is that all monitors are in the same
coordinate system, so, when I drag a window to the laptop's monitor, which is
slightly below and to the left of my main monitor, I have a negative x and y
coordinate for the window's position. So if any of the subsequent code chokes on
negative coordinates, then it is already broken on multi-monitor. I'd be very
surprised if it is the case, as so many users have multiple monitors in strange
configurations. If, with this explanation, this remains an issue for you, we
should discuss it further, because I'm fairly convinced on this point.

Second concern (subsequent code): Subsequent code that didn't execute because we
crashed could cause problems. From a cursory look at the changed instances, most
of them are just calling -[NSWindow setFrame:] or some similar function Cocoa
function based on the result. The only one that seems particularly concerned
with the existence of an actual NSScreen is the one in ui/gfx/screen_mac.mm
(which is actually the instance that the bug was filed on -- I went for the
search-replace after this). One could argue that the instances where we check if
a screen is primary do care about the existence of a NSScreen. My take on this
is that (1) they are just as likely to hit situations where the non-primary is
nil, so that wouldn't make them any worse off and (2) many of these are just
doing that switch to do different y-flips.

So, sorry for not expounding on the Cocoa-specific background earlier. If, with
this explanation, you still don't feel comfortable with the change, I can pare
it down to just the ui/gfx/screen_mac.mm part.

[sky, 2015-10-07 18:52:05]

Ok, LGTM

[ccameron, 2015-10-07 18:55:00]

Thanks!

[ccameron, 2015-10-07 18:55:39]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-10-07 18:57:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380083005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380083005/1

[commit-bot: I haz the power, 2015-10-07 19:42:30]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-07 19:43:57]

Patchset 1 (id:??) landed as
https://crrev.com/741debced2db082f4921d7a46f79d81e47caaada
Cr-Commit-Position: refs/heads/master@{#352915}