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

chrome/browser/ui/views/frame/opaque_browser_frame_view.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 "chrome/browser/ui/views/frame/opaque_browser_frame_view.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/compiler_specific.h"
@@ skipping 466 matching lines @@
 
 // views::NonClientFrameView:
 bool OpaqueBrowserFrameView::DoesIntersectRect(const views::View* target,
                                                const gfx::Rect& rect) const {
   CHECK_EQ(target, this);
   if (!views::ViewTargeterDelegate::DoesIntersectRect(this, rect)) {
     // |rect| is outside OpaqueBrowserFrameView's bounds.
     return false;
   }
 
   // If the rect is outside the bounds of the client area, claim it.

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

Lines 487-497 differ from Ash/Mus. Would removing lines 488-494 and replacing
line 497 with 'return rect.y() < GetTopInset(false)' be logically equivalent
here?

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

I doubt it.  I think the difference may be that the opaque frame has visible
frame area along the sides/bottom of the client area and Ash/Mus do not?  If so
these differences need to remain.

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

Ah, OK. It is true that the Ash/Mus frames do not have visible frame area along
the sides and bottom. So I think we should be able to change the Ash/Mus
implementations to match what we have here. Will explore that as a follow-up.

   gfx::RectF rect_in_client_view_coords_f(rect);
   View::ConvertRectToTarget(this, frame()->client_view(),
       &rect_in_client_view_coords_f);
   gfx::Rect rect_in_client_view_coords = gfx::ToEnclosingRect(
       rect_in_client_view_coords_f);
   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(
     int normal_image_id,
     int hot_image_id,
     int pushed_image_id,
     int mask_image_id,
     int accessibility_string_id,
     ViewID view_id) {
   views::ImageButton* button = new views::ImageButton(this);
   const ui::ThemeProvider* tp = frame()->GetThemeProvider();
@@ skipping 298 matching lines @@
   gfx::Rect side(x, y, kClientEdgeThickness, h);
   canvas->FillRect(side, color);
   if (draw_bottom) {
     canvas->FillRect(gfx::Rect(x, y + h, w + (2 * kClientEdgeThickness),
                                kClientEdgeThickness),
                      color);
   }
   side.Offset(w + kClientEdgeThickness, 0);
   canvas->FillRect(side, color);
 }