Fix Widget::InitParams::activatable logic

Fix broken Widget::InitParams::activatable logic as a result of https://codereview.chromium.org/286733002

This CL modifies the default return value of WidgetDelegate::CanActivate() based on the type passed into WidgetDelegate::InitParams::type
This allows whether the widget is activatable to change over time. In particular, whether the zoom bubble can be activated can change over its lifetime

BUG=353533, 374095
TEST=None

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

[pkotwicz, 2014-05-20 00:14:53]

Scott, can you please take a look?

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget.h...
ui/views/widget/widget.h:197: bool accept_events;
Scott, do you have a suggestion for a comment for |activatable|.
Specifying whether a widget is activatable in InitParams and
WidgetDelegate::CanActivate() is similar but not the same.

In particular, if only WidgetDelegate::CanActivate() returns false, the
WS_EX_NOACTIVATE style in CalculateWindowStylesFromInitParams() is not set. I
have read the MSDN documentation but I am unsure about the practical effect of
WS_EX_NOACTIVATE
http://msdn.microsoft.com/en-us/library/windows/desktop/ms632680(v=vs.85).aspx
(This CL nor https://codereview.chromium.org/286733002/ affect when
WS_EX_NOACTIVATE is set)

[sky, 2014-05-20 13:16:17]

I'm not clear on why you need to do this change. Can you better elaborate on the
behavior you need to change?

[pkotwicz, 2014-05-20 14:36:40]

The short answer: This CL is supposed to fix the regression described in
crbug.com/374095

The long answer:
ZoomBubbleView is created with BubbleDelegateView::set_use_focusless(true) when
it is shown as a result of Ctrl+/-
The ZoomBubbleView transitions to BubbleDelegateView::set_use_focusless(false)
when the user attempts to press the "Reset to Default" button in the bubble.

With the code in ToT, when the user attempts to press the "Reset to Default"
button, because Widget::CanActivate() still returns false,
HWNDMessageHandler::OnMouseActivate() returns MA_NOACTIVATEANDEAT. Hence, Chrome
never gets the mouse pressed event.

[sky, 2014-05-20 15:02:55]

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget.h...
ui/views/widget/widget.h:197: bool accept_events;
On 2014/05/20 00:14:53, pkotwicz wrote:
> Scott, do you have a suggestion for a comment for |activatable|.
> Specifying whether a widget is activatable in InitParams and
> WidgetDelegate::CanActivate() is similar but not the same.
> 
> In particular, if only WidgetDelegate::CanActivate() returns false, the
> WS_EX_NOACTIVATE style in CalculateWindowStylesFromInitParams() is not set. I
> have read the MSDN documentation but I am unsure about the practical effect of
> WS_EX_NOACTIVATE
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms632680(v=vs.85).aspx
> (This CL nor https://codereview.chromium.org/286733002/ affect when
> WS_EX_NOACTIVATE is set)

If we correctly query CanActivate at the right places it may not be necessary to
set WS_EX_NOACTIVATE at all. You would have to look into that though.

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:33: // Sets the default value of
CanActivate().
Why default here? Isn't this just set_can_activate? Also, can any overrides of
CanActivate be changed to explicitly set_can_activate now?

[pkotwicz, 2014-05-20 16:13:52]

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:33: // Sets the default value of
CanActivate().
I do not think that's possible. In particular, I think that
BrowserView::CanActivate() cannot be easily simplified to just returning a
boolean.

I called this set_default_can_activate() because I thought it was confusing for
WidgetDelegate::CanActivate() to return true by default regardless of the widget
type. I do not pass in a Widget::InitParams::Type because I want to avoid
duplicating the computation of whether a widget should be activatable.

[pkotwicz, 2014-05-20 16:33:29]

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:33: // Sets the default value of
CanActivate().
ToastContentsView is the only easy override to change. (Which I have changed in
my latest patch set)
With some work, I can probably change the overrides for BubbleDelegateView and
TrayBubbleView.

[sky, 2014-05-20 17:33:20]

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/293833002/diff/60001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:33: // Sets the default value of
CanActivate().
On 2014/05/20 16:33:30, pkotwicz wrote:
> ToastContentsView is the only easy override to change. (Which I have changed
in
> my latest patch set)
> With some work, I can probably change the overrides for BubbleDelegateView and
> TrayBubbleView.

I still prefer set_can_activate. Rationale being the corresponding method this
effects is CanActivate, not CAnDefaultActivate.

[pkotwicz, 2014-05-20 17:38:54]

I have updated the CL to use WidgetDelegate::set_can_activate()

[sky, 2014-05-20 17:43:19]

LGTM

https://codereview.chromium.org/293833002/diff/80001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.cc (right):

https://codereview.chromium.org/293833002/diff/80001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.cc:20: WidgetDelegate::WidgetDelegate() :
default_contents_view_(NULL),
nit: when you have multiple params wrap and indent 4.

https://codereview.chromium.org/293833002/diff/80001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/293833002/diff/80001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:33: // Sets whether the widget is activatable.
Sets the return value of CanActivate. Default is true.

[pkotwicz, 2014-05-20 21:22:51]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-05-20 21:24:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/293833002/100001

[commit-bot: I haz the power, 2014-05-21 03:45:40]

Change committed as 271850