Make the maximize button more easily hittable when there is a huge amount of tabs present

Make the maximize button more easily hittable when there is a huge amount of tabs present

BUG=223703
TEST=Manual, see bug

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213482

[pkotwicz, 2013-07-12 21:29:03]

James, PTAL

[James Cook, 2013-07-12 23:12:12]

https://codereview.chromium.org/19115003/diff/2001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/19115003/diff/2001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:269: if
(!tab_strip_->HitTestPoint(tabstrip_point))
Is there some other way to do this?  In particular, you're changing the
semantics of this function to mean something like "Is this position, which I
know is above the bottom of the tab strip, really overlapping the tab strip or
is it in the window caption?"  While the only *current* caller of the function
filters out points beneath the tab strip, I think it's very easy for someone to
assume they'll get a correct answer calling this function for any arbitrary
point.

Could the checking be pushed up into BrowserNonClientFrameViewAsh?  Or explicit
checks added somewhere for the size/close buttons?

(Also, what about tab strip visibility?)

[pkotwicz, 2013-07-16 01:55:40]

James, can you please take another look?

Most of this CL is making NonClientView::NonClientHitTest() const.

HOWEVER, I am pretty stressed about this CL.

I switched BrowserNonClientFrameViewAsh::HitTestRect() to use
BrowserNonClientFrameViewAsh::NonClientHitTest() and same for
OpaqueBrowserFrameView.
The reason that this change stresses me is that NonClientHitTest() never returns
HTCLIENT when the resize cursor is drawn. (Whenever the mouse is 1px inside the
edge of the browser window on CrOS and may be over the client view).
This has an effect for mouse moves (mouse presses are always consumed by
ToplevelWindowEventHandler::HandleMousePressed())

I THINK that the change in behavior is ok.

Another option I have considered is for
BrowserNonClientFrameViewAsh::HitTestRect() to use
ClientView::NonClientHitTest() instead.

[James Cook, 2013-07-16 19:25:52]

I'm sorry, but I can't assure myself that this patch is correct. I don't
understand the flow of hit tests through the various layers of the window well
enough.

Can you explain to me, either here or in a comment, what the typical flow is of
hit tests for a browser window?

This seems like a good cleanup -- it appears to eliminate some special case code
for testing for hits against the tab strip -- but it makes me nervous too.  I
don't know if we have good test coverage for clicks around the browser window
(for example, clicks one pixel left of the new tab button, or in the little
space between tabs).

This seems like a lot of effort for such a small bug.  I would be OK with a
one-off fix to hit test explicitly against the window caption buttons.

I would also be OK with proceeding with this approach, but I might ask sky to
take a look at the next round.

WDYT?

https://codereview.chromium.org/19115003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (left):

https://codereview.chromium.org/19115003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:278: if
(rect.bottom() > tabstrip_bounds.bottom())
I don't understand how we can get away with removing this logic.

https://codereview.chromium.org/19115003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/19115003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:271: return
NonClientHitTest(browser_view_point) != HTCLIENT;
Won't this return true if frame_painter_->NonClientHitTest returns HTNOWHERE
because the point is outside the window?

https://codereview.chromium.org/19115003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/19115003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:441: //
NonClientHitTest() returns HTCLIENT for the avatar menu button and avatar
I presume this block of code moved from somewhere else - where did it come from?

[pkotwicz, 2013-07-17 17:18:58]

James, can you please take another look? I switched this CL to be a one off fix
for the sake of expediency.

[James Cook, 2013-07-17 21:44:34]

This seems safer to me. A few questions.

Sorry this is going round and round. I'm just super-nervous about touching the
core hit-testing code for the frame, especially since it affects Windows.

https://codereview.chromium.org/19115003/diff/13001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/19115003/diff/13001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:297: return
rect_in_tabstrip_coords.bottom() <=
This doesn't seem to match the comment above.  Does this work because at this
point we know we are outside the tab strip's bounds?

https://codereview.chromium.org/19115003/diff/13001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/19115003/diff/13001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:472:
browser_view()->window_switcher_button();
Does anything with the window switcher button need to happen in
GlassBrowserFrameView?

https://codereview.chromium.org/19115003/diff/13001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:486: return
rect_in_tabstrip_coords.bottom() <=
Again, maybe the comment could be clearer?

[pkotwicz, 2013-07-18 14:38:29]

James, can you please take another look?

https://codereview.chromium.org/19115003/diff/13001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/19115003/diff/13001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:472:
browser_view()->window_switcher_button();
GlassBrowserFrameView does not need to worry about the WindowSwitcherButton.
GlassBrowserFrameView::HitTestRect() returns false for any rects intersecting
BrowserView.

OpaqueBrowserFrameView is always used when win8::IsSingleWindowMetroMode()
returns true. (In particular, this is affected by the return value of
BrowserFrameWin::ShouldUseNativeFrame())

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:452: if
(!frame()->client_view()->HitTestRect(rect_in_client_view_coords))
I fixed this from the pervious patch set. In the previous patch set I was not
converting |rect| to the client view bounds.

As a side note, I think it is finally time to add
views::View::ConvertRectToTarget() but I want to do that in a separate CL

[James Cook, 2013-07-18 18:39:45]

LGTM!  Thanks for hanging in there on several rewrites.

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:452: if
(!frame()->client_view()->HitTestRect(rect_in_client_view_coords))
On 2013/07/18 14:38:29, pkotwicz wrote:
> I fixed this from the pervious patch set. In the previous patch set I was not
> converting |rect| to the client view bounds.
> 
> As a side note, I think it is finally time to add
> views::View::ConvertRectToTarget() but I want to do that in a separate CL

Agreed that ConvertRectToTarget both sounds like a good idea and should be done
in another patch.

[tdanderson, 2013-07-18 20:47:04]

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:273:
View::ConvertPointToTarget(this, frame()->client_view(),
Also add a TODO for me here.

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:287:
View::ConvertPointToTarget(this, tabstrip,
Also add a TODO for me here.

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:448:
View::ConvertPointToTarget(this, frame()->client_view(),
Please add a "TODO(tdanderson): Update this once ConvertRectToTarget() is
implemented."

But I am in the process of writing a CL for views fuzzing (crbug.com/129794),
which will consider non-unit rectangles representing tap areas, so I will be
implementing ConvertRectToTarget() in my CL.

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:462:
View::ConvertPointToTarget(this, tabstrip,
Also add a TODO for me here.

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:469: return false;
Have you tested that this return is hit at the right times? (i.e., are you
confident that the existing code at line 453, 'if (rect.bottom() >
tabstrip_bounds.bottom())', is incorrect?)

https://codereview.chromium.org/19115003/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:487:
View::ConvertPointToTarget(this, window_switcher_button,
Also add a TODO for me here.

[commit-bot: I haz the power, 2013-07-24 16:57:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/19115003/31001

[commit-bot: I haz the power, 2013-07-24 19:19:28]

Change committed as 213482