Enable fullscreen windows to be shown in mac window share picker

Enable full screen windows to be shown in window picker for mac. Before this patch a full screen window can be shared if sharing is started before the window is entered into full screen mode, but not if it's already in full screen.

BUG=chromium:575990
TEST: Manual test using TextEdit full screen mode.

Committed: https://crrev.com/3a6bf2d68b8119f5d624eae47495f9f426f81897
Cr-Commit-Position: refs/heads/master@{#11311}

[niklas.enbom, 2016-01-13 19:26:15]

Description was changed from

==========
formatting fixes


Enable fullscreen windows to be shown in window picker for mac.

BUG=
==========

to

==========
Enable full screen windows to be shown in window picker for mac. Before this
patch a full screen window can be shared if sharing is started before the window
is entered into full screen mode, but not if it's already in full screen. 

BUG=chromium:575990
TEST: Manual test using TextEdit full screen mode.
==========

[niklas.enbom, 2016-01-13 19:38:05]

niklas.enbom@webrtc.org changed reviewers:
+ gyzhou@chromium.org

[niklas.enbom, 2016-01-13 19:38:27]

George can you take a first look?

[GeorgeZ, 2016-01-13 21:35:31]

There are only a couple of minor things need to be addressed.

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h
(right):

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h:50:
CFDictionaryRef window);
Please align the parameters as 
static bool IsWindowFullScreen(const MacDesktopConfiguration& desktop_config,
                               CFDictionaryRef window);

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/window_capturer_mac.mm (right):

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:119: continue;
My understanding is that when a window is full screen, it is also minimized.
Therefore only the minimized window that is not full screen need to be skipped.
Please update your comments to make this clear.

[niklas.enbom, 2016-01-13 21:56:45]

updated for all comments.

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h
(right):

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h:50:
CFDictionaryRef window);
On 2016/01/13 21:35:31, GeorgeZ wrote:
> Please align the parameters as 
> static bool IsWindowFullScreen(const MacDesktopConfiguration& desktop_config,
>                                CFDictionaryRef window);

Acknowledged.

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/window_capturer_mac.mm (right):

https://codereview.webrtc.org/1579213007/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:119: continue;
On 2016/01/13 21:35:31, GeorgeZ wrote:
> My understanding is that when a window is full screen, it is also minimized.
> Therefore only the minimized window that is not full screen need to be
skipped.
> Please update your comments to make this clear.

Acknowledged.

[GeorgeZ, 2016-01-13 21:59:59]

lgtm

[niklas.enbom, 2016-01-13 22:03:09]

niklas.enbom@webrtc.org changed reviewers:
+ sergeyu@chromium.org

[niklas.enbom, 2016-01-13 22:03:51]

Sergey, can you take a look?

[Sergey Ulanov, 2016-01-13 22:34:04]

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h
(right):

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h:53:
static bool IsWindowMinimized(CGWindowID id);
Maybe move this to window_list_utils.h/cc? It's not directly related to
full-screen detection.

IsWindowFullScreen() can be moved as well.

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/window_capturer_mac.mm (right):

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:85: kCGWindowListOptionAll
| kCGWindowListExcludeDesktopElements,
kCGWindowListOptionAll is 0. Maybe just removed it from here?

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:114: bool fullScreen =
FullScreenChromeWindowDetector::IsWindowFullScreen(
nit: fullscreen or full_screen

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:118: if (!fullScreen &&
minimized) {
"minimized && !fullscreen" would be more readable and would match the comment
above

Also ideally IsWindowFullScreen() call should be avoided for windows that are
not minimized as it has non-trivial costs. So maybe just move the function call
here, i.e.:
if (IsWindowMinimized(id) && 
!IsWindowFullScreen(desktop_config, window)) { continue;}

[niklas.enbom, 2016-01-14 23:47:06]

thanks, ptal

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h
(right):

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/mac/full_screen_chrome_window_detector.h:53:
static bool IsWindowMinimized(CGWindowID id);
Makes sense, and makes the code nicer by shortening down the function names.

On 2016/01/13 22:34:03, Sergey Ulanov wrote:
> Maybe move this to window_list_utils.h/cc? It's not directly related to
> full-screen detection.
> 
> IsWindowFullScreen() can be moved as well.

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/window_capturer_mac.mm (right):

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:85: kCGWindowListOptionAll
| kCGWindowListExcludeDesktopElements,
On 2016/01/13 22:34:03, Sergey Ulanov wrote:
> kCGWindowListOptionAll is 0. Maybe just removed it from here?

Acknowledged.

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:114: bool fullScreen =
FullScreenChromeWindowDetector::IsWindowFullScreen(
variable not needed any longer.
On 2016/01/13 22:34:04, Sergey Ulanov wrote:
> nit: fullscreen or full_screen

https://codereview.webrtc.org/1579213007/diff/60001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:118: if (!fullScreen &&
minimized) {
On 2016/01/13 22:34:04, Sergey Ulanov wrote:
> "minimized && !fullscreen" would be more readable and would match the comment
> above
> 
> Also ideally IsWindowFullScreen() call should be avoided for windows that are
> not minimized as it has non-trivial costs. So maybe just move the function
call
> here, i.e.:
> if (IsWindowMinimized(id) && 
> !IsWindowFullScreen(desktop_config, window)) { continue;}

Acknowledged.

[Sergey Ulanov, 2016-01-20 00:16:25]

lgtm

https://codereview.chromium.org/1579213007/diff/100001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/window_capturer_mac.mm (right):

https://codereview.chromium.org/1579213007/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/window_capturer_mac.mm:115:
!IsWindowFullScreen(desktop_config, window)) { continue;}
nit: move continue to a separate line. It's allowed on the a single line only
when the whole statement fits in one line, e.g.
 if (condition) { continue; }

In this case you need to wrtap it to a separatte line.

[niklas.enbom, 2016-01-20 00:34:25]

The CQ bit was checked by niklas.enbom@webrtc.org

[niklas.enbom, 2016-01-20 00:34:25]

The patchset sent to the CQ was uploaded after l-g-t-m from gyzhou@chromium.org
Link to the patchset: https://codereview.webrtc.org/1579213007/#ps100001 (title:
"formatting")

[commit-bot: I haz the power, 2016-01-20 00:34:27]

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

[commit-bot: I haz the power, 2016-01-20 01:34:18]

Description was changed from

==========
Enable full screen windows to be shown in window picker for mac. Before this
patch a full screen window can be shared if sharing is started before the window
is entered into full screen mode, but not if it's already in full screen. 

BUG=chromium:575990
TEST: Manual test using TextEdit full screen mode.
==========

to

==========
Enable full screen windows to be shown in window picker for mac. Before this
patch a full screen window can be shared if sharing is started before the window
is entered into full screen mode, but not if it's already in full screen. 

BUG=chromium:575990
TEST: Manual test using TextEdit full screen mode.
==========

[commit-bot: I haz the power, 2016-01-20 01:34:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-20 01:34:24]

Description was changed from

==========
Enable full screen windows to be shown in window picker for mac. Before this
patch a full screen window can be shared if sharing is started before the window
is entered into full screen mode, but not if it's already in full screen. 

BUG=chromium:575990
TEST: Manual test using TextEdit full screen mode.
==========

to

==========
Enable full screen windows to be shown in window picker for mac. Before this
patch a full screen window can be shared if sharing is started before the window
is entered into full screen mode, but not if it's already in full screen.

BUG=chromium:575990
TEST: Manual test using TextEdit full screen mode.

Committed: https://crrev.com/3a6bf2d68b8119f5d624eae47495f9f426f81897
Cr-Commit-Position: refs/heads/master@{#11311}
==========

[commit-bot: I haz the power, 2016-01-20 01:34:25]

Patchset 6 (id:??) landed as
https://crrev.com/3a6bf2d68b8119f5d624eae47495f9f426f81897
Cr-Commit-Position: refs/heads/master@{#11311}