Honor can_activate flag in ShowWindowWithState

Honor can_activate flag in ShowWindowWithState

In HWNDMessageHandler::ShowWindowWitState(), when ui::SHOW_STATE_DEFAULT
is passed as state, the window grabs activation after being shown. The
correct behavior would be to activate the widget if and only if the
can_activate flag is set. HWNDMessageHandler::Show() method already does
this since r244605.
Also, the DefaultWidgetDelegate should honor InitParams::can_activate
flag.

BUG=340736
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271102

[mohsen, 2014-02-04 22:59:31]

Please take a look...

[sadrul, 2014-02-04 23:25:10]

There should be a test for this.

[mohsen, 2014-05-14 23:11:50]

Can you take a look please. I have added two tests. One is a simple widget-level
interactive ui test. The other one is an interactive ui test for the touch
selection quick menu. I'm not sure if widget_interactive_uitest is a good place
for the second one. Maybe, I should create a separate file for it besides touch
selection controller. Or, maybe we can get rid of it completely, if it's not
adding that much (especially, that ugly changes in test_views_delegate and
touch_selection_controller_impl are because of this test). What do you think?

[sadrul, 2014-05-15 12:52:01]

I think this looks reasonable. +sky@ since changes Windows too.

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget_...
File ui/views/widget/widget_interactive_uitest.cc (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget_...
ui/views/widget/widget_interactive_uitest.cc:757:
EXPECT_TRUE(widget.IsActive());
Do you want to verify that the touch-selection menu is also visible?

[sky, 2014-05-15 16:44:01]

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.cc (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
ui/views/test/test_views_delegate.cc:43: if (use_desktop_native_widgets_)
Only do this if params->native_widget is NULL.

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
ui/views/test/test_views_delegate.h:22: void SetUseDesktopNativeWidgets(bool
desktop);
Inline this (meaning set_use_desktop_native_widgets(), and add a description.

https://codereview.chromium.org/154783002/diff/330001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_controller_impl.h (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_controller_impl.h:33: static void
SetOpenQuickMenuImmediatelyForTest(bool immediate);
I'm not a fan of the ForTest methods, especially when you have a friend down
below. Can't your test be friended and invoke ContextMenuTimerFired()?

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget....
ui/views/widget/widget.cc:74: can_activate_(params.can_activate && !params.child
&&
You need to sync with https://codereview.chromium.org/286733002/ .

[mohsen, 2014-05-16 15:56:08]

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.cc (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
ui/views/test/test_views_delegate.cc:43: if (use_desktop_native_widgets_)
On 2014/05/15 16:44:01, sky wrote:
> Only do this if params->native_widget is NULL.

Done.

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/test/test_view...
ui/views/test/test_views_delegate.h:22: void SetUseDesktopNativeWidgets(bool
desktop);
On 2014/05/15 16:44:01, sky wrote:
> Inline this (meaning set_use_desktop_native_widgets(), and add a description.

Done.

https://codereview.chromium.org/154783002/diff/330001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_controller_impl.h (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_controller_impl.h:33: static void
SetOpenQuickMenuImmediatelyForTest(bool immediate);
On 2014/05/15 16:44:01, sky wrote:
> I'm not a fan of the ForTest methods, especially when you have a friend down
> below. Can't your test be friended and invoke ContextMenuTimerFired()?

Agreed. Done.

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget....
ui/views/widget/widget.cc:74: can_activate_(params.can_activate && !params.child
&&
On 2014/05/15 16:44:01, sky wrote:
> You need to sync with https://codereview.chromium.org/286733002/ .

Not needed anymore. Already handled in the above CL.

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget_...
File ui/views/widget/widget_interactive_uitest.cc (right):

https://codereview.chromium.org/154783002/diff/330001/ui/views/widget/widget_...
ui/views/widget/widget_interactive_uitest.cc:757:
EXPECT_TRUE(widget.IsActive());
On 2014/05/15 12:52:02, sadrul wrote:
> Do you want to verify that the touch-selection menu is also visible?

Done.

[sky, 2014-05-16 17:21:29]

LGTM with the following changes

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.cc (right):

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
ui/views/test/test_views_delegate.cc:7: #include
"ui/views/widget/desktop_aura/desktop_native_widget_aura.h"
Only include this on !chromeos (match your ifdef on 36).

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
ui/views/test/test_views_delegate.h:24: // be.
nit: add a comment this has no effect on chromeos.

[mohsen, 2014-05-16 17:36:38]

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.cc (right):

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
ui/views/test/test_views_delegate.cc:7: #include
"ui/views/widget/desktop_aura/desktop_native_widget_aura.h"
On 2014/05/16 17:21:29, sky wrote:
> Only include this on !chromeos (match your ifdef on 36).

Done.

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/154783002/diff/380001/ui/views/test/test_view...
ui/views/test/test_views_delegate.h:24: // be.
On 2014/05/16 17:21:29, sky wrote:
> nit: add a comment this has no effect on chromeos.

Done.

[mohsen, 2014-05-16 19:22:42]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-05-16 19:23:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohsen@chromium.org/154783002/400001

[commit-bot: I haz the power, 2014-05-16 20:25:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-16 21:32:00]

Change committed as 271102