[webkit-reviews] review granted: [Bug 40933] Playing movie full screen on second monitor hides menu bar and title bar on main monitor : [Attachment 59293] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 40933: Playing movie full screen on second monitor hides menu bar and title
bar on main monitor
https://bugs.webkit.org/show_bug.cgi?id=40933

Attachment 59293: Patch
https://bugs.webkit.org/attachment.cgi?id=59293&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    SetSystemUIMode(_savedUIMode, _savedUIOptions);
> +    [self updateMenuAndDockForFullscreen];	

You have some extra trailing spaces on this line of code.

> +    // NSApplicationPresentationOptions is available on > 10.6 only:

This comment says > 10.6, but the code says >= 10.6.

> +	   if ([[NSScreen screens] objectAtIndex:0] == fullscreenScreen)
> +	       options |= (NSApplicationPresentationAutoHideMenuBar |
NSApplicationPresentationAutoHideDock);

I don't think you need those parentheses.

> +    if ([NSApp respondsToSelector:@selector(setPresentationOptions:)])
> +	   [NSApp setPresentationOptions:options];
> +    else
> +#endif
> +	   SetSystemUIMode(_isEndingFullscreen ? kUIModeNormal :
kUIModeAllHidden, 0);

I recommend using a "return" here to avoid the need for the else and the ugly
need to have code outside the #if indented based on code inside the #if.

Is the respondsToSelector: call needed or helpful?

r=me