Refactor DoesIntersectRect into BrowserNonClientFrameView.

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

Index: chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
diff --git a/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc b/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
index 98e42f6f43cf99e0c12b93b879e17a008b0dd60b..98b6f2f3ca0228e7a14eb0486c578ea882367a20 100644
--- a/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
+++ b/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
@@ -209,6 +209,49 @@ void BrowserNonClientFrameView::ActivationChanged(bool active) {
   SchedulePaint();
 }
 
+bool BrowserNonClientFrameView::DoesIntersectRect(const views::View* target,
+                                               const gfx::Rect& rect) const {

[msw, 2016/08/19 23:56:40]

nit: indentation. I see other formatting issues; please run 'git cl format'

[Bret, 2016/08/22 18:29:42]

Done.

+  CHECK_EQ(target, this);

[sky, 2016/08/20 20:17:10]

Normally we use DCHECK for these sort of checks.

[Bret, 2016/08/22 18:29:42]

Done.

+  if (!views::ViewTargeterDelegate::DoesIntersectRect(this, rect)) {
+    // |rect| is outside OpaqueBrowserFrameView's bounds.

[msw, 2016/08/19 23:56:40]

nit: s/OpaqueBrowserFrameView/BrowserNonClientFrameView/

[Bret, 2016/08/22 18:29:42]

Done.

+    return false;
+  }
+
+  // If the rect is outside the bounds of the client area, claim it.

[msw, 2016/08/19 23:56:40]

Neither Ash or Mus did this previously; I suspect the difference is that the
desktop opaque frame impl supports non-client targeting for clicks on the window
left/right/bottom edges, while ash/chromeos doesn't have any pixels for those
edges. I guess it seems okay to support this in all subclasses, even if they
don't have a use for it; if anything it's a small amount of wasted computation.
Still, Scott might catch something I'm missing here, or suggest putting this in
an #if !defined(USE_ASH) or similar.

[sky, 2016/08/20 20:17:10]

For ash this doesn't matter as I believe the client area fills the entire bounds
(so this is always true), and for mus it should be ok.

+  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())
+    return false;
+
+  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);
+  if (rect_in_tabstrip_coords.bottom() > tabstrip->GetLocalBounds().bottom()) {

[msw, 2016/08/19 23:56:40]

nit: consider using the other impl here:
  return (rect_in_tabstrip_coords.y() <= tabstrip->height()) &&
          (!tabstrip->HitTestRect(rect_in_tabstrip_coords) ||
           tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords));

[Bret, 2016/08/22 18:29:42]

I like this one better, it's a lot more readable imo.

+    // |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;
+}
+
 void BrowserNonClientFrameView::OnProfileAdded(
     const base::FilePath& profile_path) {
   OnProfileAvatarChanged(profile_path);