Do not allow NULL to be passed into FocusManager::ViewRemoved.

ui/views/focus/focus_manager.cc

Index: ui/views/focus/focus_manager.cc
diff --git a/ui/views/focus/focus_manager.cc b/ui/views/focus/focus_manager.cc
index bd6b0dc19fe4749a958afe41f8c49dd158ed446a..c65fa84199112981a7331fa27060bd10966c13a9 100644
--- a/ui/views/focus/focus_manager.cc
+++ b/ui/views/focus/focus_manager.cc
@@ -517,7 +517,10 @@ void FocusManager::ViewRemoved(View* removed) {
   // clear the focus.  However, it's not safe to call ClearFocus()
   // (and in turn ClearNativeFocus()) here because ViewRemoved() can
   // be called while the top level widget is being destroyed.
-  if (focused_view_ && removed->Contains(focused_view_))
+  if (!focused_view_)

[sky, 2016/06/09 17:23:14]

Contains handles null correctly, so you shouldn't need this early out.

[krasin, 2016/06/09 17:46:11]

I need an early exit, if ViewRemoved is called with focused_view_==NULL and
removed==NULL, which is happening all the time: https://crbug.com/617305#c6

[sky, 2016/06/09 18:06:00]

Not sure I follow. If focused_view_ is null Contains(null) returns false. In
other words, I don't think you need an early out. But maybe I'm missing
something?

[krasin, 2016/06/09 18:13:06]

Consider the case, when removed == NULL. We have two cases:

1. Common, when focused_view_ == NULL as well;
2. Uncommon, fixed in this CL: focused_view != NULL.

It means that I can't put the DCHECK(removed) into the beginning of the
function, as it will fire on the case 1 as well.

That's why we need the condition prior to the DCHECK with an early exit.

[sky, 2016/06/09 22:34:24]

removed == null is an error in the call sites and should be fixed. That's why
you're fixing ui/views/widget/widget.cc.

In case I'm not being clear this code should look like:

void FocusManager::ViewRemoved(View* removed) {
  DCHECK(removed);  // We should never be supplied a null view.
  if (removed->Contains(focused_view_))
    SetFocusedView(nullptr);
}

[krasin, 2016/06/09 23:10:57]

I have tried that. Multiple (almost all) tests are broken in this case, see the
very first diff in the current CL:
https://codereview.chromium.org/2047083002/#ps1

If you believe it's always wrong to pass NULL into this function, I will try to
fix all the callers. 

+    return;
+  DCHECK(removed != NULL);

[sky, 2016/06/09 17:23:14]

DCHECK(removed);

[krasin, 2016/06/09 17:46:11]

Done.

+  if (removed->Contains(focused_view_))
     SetFocusedView(NULL);
 }