Make sure new maximized window gets activated

Make sure new maximized window gets activated

BUG=401818
TEST=Maximize window and start typing in omnibox. Open new window from jump list. Check if new window is active and it's possible to type in omnibox.

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

[Tomasz Moniuszko, 2014-11-14 14:59:55]

tmoniuszko@opera.com changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[sky, 2014-11-14 17:51:46]

Can you add a test case?

[sadrul, 2014-11-18 17:17:57]

sadrul@chromium.org changed reviewers:
- sadrul@chromium.org

[sadrul, 2014-11-18 17:17:58]

[ Removing myself from reviewer list since Windows, sky@ is OWNER ]

[Tomasz Moniuszko, 2014-11-24 14:38:05]

On 2014/11/14 17:51:46, sky wrote:
> Can you add a test case?

I added TEST line to description. Did you mean adding an unit test? I don't see
unit tests for HWNDMessageHandler. I think it may be difficult to write unit
tests because HWNDMessageHandler calls Win API functions and needs a valid HWND.

[sky, 2014-11-24 16:23:07]

https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
ui/views/win/hwnd_message_handler.cc:643: Activate();
I think you're working around a problem that may be else where in the code.
Does this also happen when showing non-maximized?

If Chrome is already running and you click a shortcut then I believe we end up
passing a message to chrome. Do we need to do something else to ensure the
existing chrome ends up as the active/focused app?

[Tomasz Moniuszko, 2014-11-27 15:35:50]

https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
ui/views/win/hwnd_message_handler.cc:643: Activate();
On 2014/11/24 16:23:07, sky wrote:
> I think you're working around a problem that may be else where in the code.
> Does this also happen when showing non-maximized?

No it doesn't because Activate() is being called from
HWNDMessageHandler::ShowWindowWithState(). It does when I remove Activate() call
from that method.

If new window is opened from jumplist while existing window is maximized,
HWNDMessageHandler::ShowMaximizedWithBounds() is called instead of
ShowWindowWithState() so calling Activate() from here is just analogous
solution.

[sky, 2014-12-01 17:08:23]

On Thu, Nov 27, 2014 at 7:35 AM,  <tmoniuszko@opera.com> wrote:
>
>
https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
> ui/views/win/hwnd_message_handler.cc:643: Activate();
> On 2014/11/24 16:23:07, sky wrote:
>>
>> I think you're working around a problem that may be else where in the
>
> code.
>>
>> Does this also happen when showing non-maximized?
>
>
> No it doesn't because Activate() is being called from
> HWNDMessageHandler::ShowWindowWithState(). It does when I remove
> Activate() call from that method.

I'm surprised that works as the process we're calling Active() from
isn't the foreground process.

>
> If new window is opened from jumplist while existing window is
> maximized, HWNDMessageHandler::ShowMaximizedWithBounds() is called
> instead of ShowWindowWithState() so calling Activate() from here is just
> analogous solution.
>
> https://codereview.chromium.org/725953002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-12-01 17:11:45]

LGTM

https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
ui/views/win/hwnd_message_handler.cc:643: Activate();
On 2014/11/27 15:35:50, Tomasz Moniuszko wrote:
> On 2014/11/24 16:23:07, sky wrote:
> > I think you're working around a problem that may be else where in the code.
> > Does this also happen when showing non-maximized?
> 
> No it doesn't because Activate() is being called from
> HWNDMessageHandler::ShowWindowWithState(). It does when I remove Activate()
call
> from that method.

Ah, ok, I get it.

> 
> If new window is opened from jumplist while existing window is maximized,
> HWNDMessageHandler::ShowMaximizedWithBounds() is called instead of
> ShowWindowWithState() so calling Activate() from here is just analogous
> solution.

I don't think you need the comment here. We should activate just as we do in the

[sky, 2014-12-01 17:12:19]

On 2014/12/01 17:11:45, sky wrote:
> LGTM
> 
>
https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
> File ui/views/win/hwnd_message_handler.cc (right):
> 
>
https://codereview.chromium.org/725953002/diff/1/ui/views/win/hwnd_message_ha...
> ui/views/win/hwnd_message_handler.cc:643: Activate();
> On 2014/11/27 15:35:50, Tomasz Moniuszko wrote:
> > On 2014/11/24 16:23:07, sky wrote:
> > > I think you're working around a problem that may be else where in the
code.
> > > Does this also happen when showing non-maximized?
> > 
> > No it doesn't because Activate() is being called from
> > HWNDMessageHandler::ShowWindowWithState(). It does when I remove Activate()
> call
> > from that method.
> 
> Ah, ok, I get it.
> 
> > 
> > If new window is opened from jumplist while existing window is maximized,
> > HWNDMessageHandler::ShowMaximizedWithBounds() is called instead of
> > ShowWindowWithState() so calling Activate() from here is just analogous
> > solution.
> 
> I don't think you need the comment here. We should activate just as we do in
the

Sorry for the noise. Ignore this last note and leave the comment you have.

[Tomasz Moniuszko, 2014-12-02 09:20:39]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2014-12-02 09:21:46]

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

[commit-bot: I haz the power, 2014-12-02 10:42:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-12-02 10:43:37]

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