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

ui/views/focus/focus_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/focus/focus_manager.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/auto_reset.h"
@@ skipping 499 matching lines @@
 // static
 bool FocusManager::IsTabTraversalKeyEvent(const ui::KeyEvent& key_event) {
   return key_event.key_code() == ui::VKEY_TAB && !key_event.IsControlDown();
 }
 
 void FocusManager::ViewRemoved(View* removed) {
   // If the view being removed contains (or is) the focused view,
   // 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);
 }
 
 void FocusManager::AddFocusChangeListener(FocusChangeListener* listener) {
   focus_change_listeners_.AddObserver(listener);
 }
 
 void FocusManager::RemoveFocusChangeListener(FocusChangeListener* listener) {
   focus_change_listeners_.RemoveObserver(listener);
 }
@@ skipping 21 matching lines @@
 // |keyboard_accessible_| is only used on Mac.
 #if defined(OS_MACOSX)
   return keyboard_accessible_ ? view->IsAccessibilityFocusable()
                               : view->IsFocusable();
 #else
   return view->IsAccessibilityFocusable();
 #endif
 }
 
 }  // namespace views