Check for null |tabstrip_| in BrowserView::IsTabStripVisible()

chrome/browser/ui/views/frame/opaque_browser_frame_view.cc

Index: chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
diff --git a/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc b/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
index 53e69df1d366abeb7dfb64743978d2278e1e1999..c163f6ff11335136666ce1469ba1c95e56af3f9b 100644
--- a/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
+++ b/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
@@ -493,30 +493,24 @@ bool OpaqueBrowserFrameView::DoesIntersectRect(const views::View* target,
   if (!frame()->client_view()->HitTestRect(rect_in_client_view_coords))
     return true;
 
-  // Otherwise, claim |rect| only if it is above the bottom of the tabstrip in
-  // a non-tab portion.
-  TabStrip* tabstrip = browser_view()->tabstrip();
-  if (!tabstrip || !browser_view()->IsTabStripVisible())
+  if (!browser_view()->IsTabStripVisible())
     return false;
 
+  // Claim |rect| only if it is above the bottom of the tabstrip in a non-tab
+  // portion. In particular, the avatar label/button is left of the tabstrip and
+  // the window controls are right of the tabstrip.
+  TabStrip* tabstrip = browser_view()->tabstrip();
   gfx::RectF rect_in_tabstrip_coords_f(rect);
   View::ConvertRectToTarget(this, tabstrip, &rect_in_tabstrip_coords_f);
-  gfx::Rect rect_in_tabstrip_coords = gfx::ToEnclosingRect(
-      rect_in_tabstrip_coords_f);
+  const gfx::Rect rect_in_tabstrip_coords =
+      gfx::ToEnclosingRect(rect_in_tabstrip_coords_f);
   if (rect_in_tabstrip_coords.bottom() > tabstrip->GetLocalBounds().bottom()) {

[tdanderson, 2016/03/14 20:38:33]

This is different than the corresponding check performed by Ash/Mus. Here you're
saying "return false if any part of the rect is below the bottom of the
tabstrip" whereas Ash/Mus says "return false if the entire rect is below the
bottom of the tabstrip". I believe the Ash/Mus version is what we want here.

[Peter Kasting, 2016/03/14 23:04:42]

I agree.

[tdanderson, 2016/03/15 15:51:43]

Can address as follow-up.

     // |rect| is below the tabstrip.
     return false;
   }
 
-  if (tabstrip->HitTestRect(rect_in_tabstrip_coords)) {
-    // Claim |rect| if it is in a non-tab portion of the tabstrip.
-    return tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords);
-  }
-
-  // We claim |rect| because it is above the bottom of the tabstrip, but
-  // not in the tabstrip itself. In particular, the avatar label/button is left
-  // of the tabstrip and the window controls are right of the tabstrip.
-  return true;
+  return !tabstrip->HitTestRect(rect_in_tabstrip_coords) ||
+         tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords);
 }

[tdanderson, 2016/03/14 20:38:33]

GlassBFV::DoesIntersectRect() looks very different but (I think) it's
essentially trying to do the same thing as the other three implementations, so
perhaps we'd want to change its implementation to match these three?

[Peter Kasting, 2016/03/14 23:04:42]

I don't know.  Maybe.  I think I'd want to verify some of these behavioral
differences by coming up with concrete sequences of actions that test the
differences.  Then we could probably just tell "should this return true or false
when I do this" by what actually ends up happening.

[tdanderson, 2016/03/15 15:51:42]

OK, I'll leave that alone then.

 
 views::ImageButton* OpaqueBrowserFrameView::InitWindowCaptionButton(