MacViews: Handling changing of first responder inside NativeWidgetMac.

MacViews: Handling changing of first responder inside NativeWidgetMac.

NativeWidgetMac usually contains more then one NSView.
For example browser window contains ViewsCompositorSuperview and WebContentsView.
So views::FocusManager must handle not only changing of key NSWindow,
but also changing of first responder inside NSWindow, that holds
NativeWidget.

BUG=591016
Committed: https://crrev.com/c2936ff6d9dfed0b4fea95575a1a3a099b46be5d
Cr-Commit-Position: refs/heads/master@{#379525}

[kirr, 2016-03-03 11:14:00]

kirr@yandex-team.ru changed reviewers:
+ tapted@chromium.org

[kirr, 2016-03-03 11:14:01]

Please take a look.

[tapted, 2016-03-04 00:56:20]

Thanks for working on this!

In a follow-up, I think we might also need
-validateProposedFirstResponder:forEvent so the webcontents doesn't lose focus
except when clicking into a textfield, but this is a good step.

https://codereview.chromium.org/1759913004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1759913004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:412: if (result) {
We need to check for |hostedView_| being null since it's not refcounted, so has
a different lifetime.

perhaps for these,

  BOOL result = [super becomeFirstResponder];
  if (!result || !hostedView_)
    return result;

then, e.g.,
  hostedView_->GetWidget()->GetFocusManager()->RestoreFocusedView();

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:74: @interface FocusedNSView :
NSView
nit: FocusableTestNSView  (name collisions are easy in ObjC, so it's good for
test classes to always have, e.g., "Test" or "Mock" in the name)

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1323: DCHECK_EQ([child_view
acceptsFirstResponder], YES);
nit: EXPECT_TRUE

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1324:
DCHECK_EQ(widget->GetRootView()->IsFocusable(), YES);
EXPECT_TRUE  (somewhat moot here since this won't be compiled on Windows, but I
think MSVC would refuse to compile stuff like this since it's comparing a `char`
with a `bool`)

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1328:
DCHECK_EQ(manager->GetFocusedView(), widget->GetRootView());
EXPECT_EQ

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1330: [widget->GetNativeWindow()
makeFirstResponder: child_view];
nit: no space before `child_view`

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1331:
DCHECK(!manager->GetFocusedView());
EXPECT_FALSE

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1333: [widget->GetNativeWindow()
makeFirstResponder: widget->GetNativeView()];
nit: no space before `widget`.  -- running `git cl format` might actually fix
these up automatically for you

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1334:
DCHECK_EQ(manager->GetFocusedView(), widget->GetRootView());
EXPECT_EQ

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1376: 
nit: remove trailing blank line

[kirr, 2016-03-04 08:31:31]

Sorry for untidy code.

> In a follow-up, I think we might also need
> -validateProposedFirstResponder:forEvent so the webcontents doesn't lose focus
except when clicking into a textfield, but this is a good step.

Yep. Sounds very reasonable. I can try to do it next (but not now
unfortunately).

https://codereview.chromium.org/1759913004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1759913004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:412: if (result) {
On 2016/03/04 00:56:19, tapted wrote:
> We need to check for |hostedView_| being null since it's not refcounted, so
has
> a different lifetime.
> 
> perhaps for these,
> 
>   BOOL result = [super becomeFirstResponder];
>   if (!result || !hostedView_)
>     return result;
> 
> then, e.g.,
>   hostedView_->GetWidget()->GetFocusManager()->RestoreFocusedView();

Done. Thanks.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:74: @interface FocusedNSView :
NSView
On 2016/03/04 00:56:19, tapted wrote:
> nit: FocusableTestNSView  (name collisions are easy in ObjC, so it's good for
> test classes to always have, e.g., "Test" or "Mock" in the name)

Done.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1323: DCHECK_EQ([child_view
acceptsFirstResponder], YES);
On 2016/03/04 00:56:19, tapted wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1324:
DCHECK_EQ(widget->GetRootView()->IsFocusable(), YES);
On 2016/03/04 00:56:19, tapted wrote:
> EXPECT_TRUE  (somewhat moot here since this won't be compiled on Windows, but
I
> think MSVC would refuse to compile stuff like this since it's comparing a
`char`
> with a `bool`)

Done.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1328:
DCHECK_EQ(manager->GetFocusedView(), widget->GetRootView());
On 2016/03/04 00:56:19, tapted wrote:
> EXPECT_EQ

Done.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1330: [widget->GetNativeWindow()
makeFirstResponder: child_view];
On 2016/03/04 00:56:19, tapted wrote:
> nit: no space before `child_view`

Done.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1331:
DCHECK(!manager->GetFocusedView());
On 2016/03/04 00:56:19, tapted wrote:
> EXPECT_FALSE

Done.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1333: [widget->GetNativeWindow()
makeFirstResponder: widget->GetNativeView()];
On 2016/03/04 00:56:19, tapted wrote:
> nit: no space before `widget`.  -- running `git cl format` might actually fix
> these up automatically for you

Done.
"git cl format" - is super helpful. Thanks a lot.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1334:
DCHECK_EQ(manager->GetFocusedView(), widget->GetRootView());
On 2016/03/04 00:56:19, tapted wrote:
> EXPECT_EQ

Done.

https://codereview.chromium.org/1759913004/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac_unittest.mm:1376: 
On 2016/03/04 00:56:20, tapted wrote:
> nit: remove trailing blank line

Done.

[tapted, 2016-03-07 00:12:42]

lgtm with a test comment - thanks!

https://codereview.chromium.org/1759913004/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1759913004/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_unittest.mm:1315: TEST_F(NativeWidgetMacTest,
ChangeFocusOnChangeFirstResponder) {
nit: comment before to explain the test, e.g.

// Ensure traversing NSView focus correctly updates the views::FocusManager.

[kirr, 2016-03-07 06:07:52]

The CQ bit was checked by kirr@yandex-team.ru

[kirr, 2016-03-07 06:07:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1759913004/#ps40001
(title: "Added comment for test.")

[commit-bot: I haz the power, 2016-03-07 06:08:03]

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

[commit-bot: I haz the power, 2016-03-07 07:58:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-07 07:59:27]

Description was changed from

==========
MacViews: Handling changing of first responder inside NativeWidgetMac.

NativeWidgetMac usually contains more then one NSView.
For example browser window contains ViewsCompositorSuperview and
WebContentsView.
So views::FocusManager must handle not only changing of key NSWindow,
but also changing of first responder inside NSWindow, that holds
NativeWidget.

BUG=591016
==========

to

==========
MacViews: Handling changing of first responder inside NativeWidgetMac.

NativeWidgetMac usually contains more then one NSView.
For example browser window contains ViewsCompositorSuperview and
WebContentsView.
So views::FocusManager must handle not only changing of key NSWindow,
but also changing of first responder inside NSWindow, that holds
NativeWidget.

BUG=591016

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

[commit-bot: I haz the power, 2016-03-07 07:59:28]

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