Reduce the size of the fullscreen window on activation loss only if the window being activated is …

Reduce the size of the fullscreen window on activation loss only if the window being  activated is on the same monitor

Currently we reduce the size of the fullscreen window on activation loss by 1 px to ensure that other maximized windows on the same thread don't
get treated as fullscreen windows, i.e. they draw over the taskbar, autohide taskbar does not work etc.

Based on some testing it looks like this problem only happens if the fullscreen window and the window being activated are on the same monitor.

The fix is to reduce the fullscreen window bounds only if it is on the same monitor as the window being activated. For this we have a map in
HWNDMessageHandler which is keyed on HMONITOR. We use this to determine the same. This map is populated when a window becomes fullscreen.

BUG=604359
TEST=The fullscreen window size reducing is already covered by the interactive_ui_test WidgetTestInteractive.
FullscreenBoundsReducedOnActivationLoss. Don't have a good way to test the multiple monitor case.

Committed: https://crrev.com/b14dd7c0ea2957b091eb98c7b5f461bfc8129fe0
Cr-Commit-Position: refs/heads/master@{#394000}

[ananta, 2016-05-13 22:05:56]

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

[sky, 2016-05-13 23:32:36]

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:305:
HWNDMessageHandler::FullscreenWindowMonitorMap*
Use LazyInstance, it's less error prone than what you have here.

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:2755: PostMessage(index->second,
Why bother with the PostMessage? Why not call the HWNDMessageHandler directly?

[ananta, 2016-05-16 19:56:43]

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:305:
HWNDMessageHandler::FullscreenWindowMonitorMap*
On 2016/05/13 23:32:36, sky wrote:
> Use LazyInstance, it's less error prone than what you have here.

Done.

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:2755: PostMessage(index->second,
On 2016/05/13 23:32:36, sky wrote:
> Why bother with the PostMessage? Why not call the HWNDMessageHandler directly?

The fullscreen window could be a different instance of HWNDMessageHandler. We
need to get to that context and hence PostMessage.

[sky, 2016-05-16 21:14:28]

On Mon, May 16, 2016 at 12:56 PM,  <ananta@chromium.org> wrote:
>
>
https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
> ui/views/win/hwnd_message_handler.cc:305:
> HWNDMessageHandler::FullscreenWindowMonitorMap*
> On 2016/05/13 23:32:36, sky wrote:
>> Use LazyInstance, it's less error prone than what you have here.
>
> Done.
>
>
https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
> ui/views/win/hwnd_message_handler.cc:2755: PostMessage(index->second,
> On 2016/05/13 23:32:36, sky wrote:
>> Why bother with the PostMessage? Why not call the HWNDMessageHandler
> directly?
>
> The fullscreen window could be a different instance of
> HWNDMessageHandler. We need to get to that context and hence
> PostMessage.

You could store this in your map, right?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ananta, 2016-05-16 21:35:42]

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1976923004/diff/40001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:2755: PostMessage(index->second,
On 2016/05/16 19:56:43, ananta wrote:
> On 2016/05/13 23:32:36, sky wrote:
> > Why bother with the PostMessage? Why not call the HWNDMessageHandler
directly?
> 
> The fullscreen window could be a different instance of HWNDMessageHandler. We
> need to get to that context and hence PostMessage.

yeah. done.

[sky, 2016-05-16 23:05:10]

LGTM

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:828: FullscreenWindowMonitorMap::iterator
index =
index->iter

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1366: FullscreenWindowMonitorMap::iterator
index;
index->iter (as it's an iterator).

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1367: for (index =
fullscreen_monitor_map_.Get().begin();
nit: use enhanced for loop (and move iter inside loop).

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:2728: FullscreenWindowMonitorMap::iterator
index =
index->iter

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:2742: &monitor_info);
nit: run git cl format

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.h:669: typedef std::map<HMONITOR,
HWNDMessageHandler*> FullscreenWindowMonitorMap;
using

[ananta, 2016-05-16 23:31:03]

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1366: FullscreenWindowMonitorMap::iterator
index;
On 2016/05/16 23:05:10, sky wrote:
> index->iter (as it's an iterator).

Done.

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1367: for (index =
fullscreen_monitor_map_.Get().begin();
On 2016/05/16 23:05:10, sky wrote:
> nit: use enhanced for loop (and move iter inside loop).

Done.

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:2728: FullscreenWindowMonitorMap::iterator
index =
On 2016/05/16 23:05:10, sky wrote:
> index->iter

Done.

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:2742: &monitor_info);
On 2016/05/16 23:05:10, sky wrote:
> nit: run git cl format

Done.

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/1976923004/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.h:669: typedef std::map<HMONITOR,
HWNDMessageHandler*> FullscreenWindowMonitorMap;
On 2016/05/16 23:05:10, sky wrote:
> using

Done.

[ananta, 2016-05-16 23:56:49]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-05-16 23:56:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1976923004/#ps140001
(title: "Fix loop")

[commit-bot: I haz the power, 2016-05-16 23:57:03]

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

[commit-bot: I haz the power, 2016-05-17 01:05:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-17 01:07:52]

Description was changed from

==========
Reduce the size of the fullscreen window on activation loss only if the window
being  activated is on the same monitor

Currently we reduce the size of the fullscreen window on activation loss by 1 px
to ensure that other maximized windows on the same thread don't
get treated as fullscreen windows, i.e. they draw over the taskbar, autohide
taskbar does not work etc.

Based on some testing it looks like this problem only happens if the fullscreen
window and the window being activated are on the same monitor.

The fix is to reduce the fullscreen window bounds only if it is on the same
monitor as the window being activated. For this we have a map in
HWNDMessageHandler which is keyed on HMONITOR. We use this to determine the
same. This map is populated when a window becomes fullscreen.

BUG=604359
TEST=The fullscreen window size reducing is already covered by the
interactive_ui_test WidgetTestInteractive.
FullscreenBoundsReducedOnActivationLoss. Don't have a good way to test the
multiple monitor case.
==========

to

==========
Reduce the size of the fullscreen window on activation loss only if the window
being  activated is on the same monitor

Currently we reduce the size of the fullscreen window on activation loss by 1 px
to ensure that other maximized windows on the same thread don't
get treated as fullscreen windows, i.e. they draw over the taskbar, autohide
taskbar does not work etc.

Based on some testing it looks like this problem only happens if the fullscreen
window and the window being activated are on the same monitor.

The fix is to reduce the fullscreen window bounds only if it is on the same
monitor as the window being activated. For this we have a map in
HWNDMessageHandler which is keyed on HMONITOR. We use this to determine the
same. This map is populated when a window becomes fullscreen.

BUG=604359
TEST=The fullscreen window size reducing is already covered by the
interactive_ui_test WidgetTestInteractive.
FullscreenBoundsReducedOnActivationLoss. Don't have a good way to test the
multiple monitor case.

Committed: https://crrev.com/b14dd7c0ea2957b091eb98c7b5f461bfc8129fe0
Cr-Commit-Position: refs/heads/master@{#394000}
==========

[commit-bot: I haz the power, 2016-05-17 01:07:53]

Patchset 8 (id:??) landed as
https://crrev.com/b14dd7c0ea2957b091eb98c7b5f461bfc8129fe0
Cr-Commit-Position: refs/heads/master@{#394000}