Modify hit test masks for tabs and tab close buttons

Modify hit test masks for tabs and tab close buttons

In preparation for landing rect-based event targeting 
(issue 129794), the tab close button will need a hit 
test mask and the hit test mask for a tab will need 
to be modified. This will take into consideration the 
possibility where the tab close button of one tab is 
occluded by another tab when in stacked tab / touch 
layout mode.

BUG=306186
TEST=TabStripTest.TabHitTestMaskWhenStacked

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

[tdanderson, 2013-10-24 20:56:23]

Hi Scott, can you please take a look?

https://codereview.chromium.org/32463003/diff/330001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/330001/ui/views/view.h#newcode978
ui/views/view.h:978: enum HitTestSource {
Is there a better place for this enum? I needed to make it public so that it
could be accessed by the unit test.

[sky, 2013-10-24 22:13:38]

https://codereview.chromium.org/32463003/diff/330001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/32463003/diff/330001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:179: HitTestSource source =
HIT_TEST_SOURCE_MOUSE) const OVERRIDE;
style guide says not parameters with default values.

https://codereview.chromium.org/32463003/diff/330001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/330001/ui/views/view.h#newcode978
ui/views/view.h:978: enum HitTestSource {
On 2013/10/24 20:56:24, tdanderson wrote:
> Is there a better place for this enum? I needed to make it public so that it
> could be accessed by the unit test.

I think this is the best place, but move it above struct
ViewHierarchyChangedDetails (style guide says enums at beginning of section).
And do we really only use these two? I'm surprised there aren't other random
bits of code that use it.

[tdanderson, 2013-10-25 00:10:51]

Can you please take another look? 

Note I also made some changes to the tab strip test to hopefully avoid what
appears to be some precision errors on some trybots (as a result of using
ToEnclosedRect()).

https://codereview.chromium.org/32463003/diff/330001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/32463003/diff/330001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:179: HitTestSource source =
HIT_TEST_SOURCE_MOUSE) const OVERRIDE;
On 2013/10/24 22:13:38, sky wrote:
> style guide says not parameters with default values.

Done.

https://codereview.chromium.org/32463003/diff/330001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/330001/ui/views/view.h#newcode978
ui/views/view.h:978: enum HitTestSource {
On 2013/10/24 22:13:38, sky wrote:
> On 2013/10/24 20:56:24, tdanderson wrote:
> > Is there a better place for this enum? I needed to make it public so that it
> > could be accessed by the unit test.
> 
> I think this is the best place, but move it above struct
> ViewHierarchyChangedDetails (style guide says enums at beginning of section).
> And do we really only use these two? I'm surprised there aren't other random
> bits of code that use it.

Done. And yes, as far as I know this is all we need.

[tdanderson, 2013-10-25 17:02:25]

https://codereview.chromium.org/32463003/diff/570001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://codereview.chromium.org/32463003/diff/570001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:310: // TODO(tdanderson):
Uncomment this line once crbug.com/311609 is resolved.
@sky: It turns out that the off-by-one errors were not due to precision issues
as I had originally thought. They are due to the insets of the tab close
button's contents bounds being slightly different across platforms, which I am
tracking at crbug.com/311609.

[sky, 2013-10-28 14:26:00]

https://codereview.chromium.org/32463003/diff/570001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/32463003/diff/570001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:313: HitTestSource source) const
OVERRIDE;
align HitTestSource with openning '(' on previous line.

https://codereview.chromium.org/32463003/diff/570001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/570001/ui/views/view.h#newcode1139
ui/views/view.h:1139: virtual void GetHitTestMask(gfx::Path* mask, HitTestSource
source) const;
out params should be last, so GetHitTestMask(source, mask)

[tdanderson, 2013-10-28 15:40:08]

Scott, can you please take another look?

https://codereview.chromium.org/32463003/diff/570001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/32463003/diff/570001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:313: HitTestSource source) const
OVERRIDE;
On 2013/10/28 14:26:00, sky wrote:
> align HitTestSource with openning '(' on previous line.

Done.

https://codereview.chromium.org/32463003/diff/570001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/570001/ui/views/view.h#newcode1139
ui/views/view.h:1139: virtual void GetHitTestMask(gfx::Path* mask, HitTestSource
source) const;
On 2013/10/28 14:26:00, sky wrote:
> out params should be last, so GetHitTestMask(source, mask)

Done.

[sky, 2013-10-28 19:50:44]

LGTM

[commit-bot: I haz the power, 2013-10-28 20:14:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/32463003/710001

[Ben Goodger (Google), 2013-10-28 20:23:29]

https://codereview.chromium.org/32463003/diff/710001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/710001/ui/views/view.h#newcode116
ui/views/view.h:116: enum HitTestSource {
let's not add this to views. Instead we should delegate more of this to the
client.

[Ben Goodger (Google), 2013-10-28 20:46:07]

+sadrul, with whom I have been discussing event stuff for some time.

Sadrul, my ideal interface for View removes pretty much all of this hit testing
logic from the baseline. Instead the RootView (the EventDispatcher) somehow asks
the EventTargeter (new concept, perhaps also implemented at RootView to begin
with?) to return a target for an event. The default EventTargeter may consult
the client code to assist/completely replace the default logic.

Hit testing can be arbitrarily complex depending on the UI you're building. I'd
like to move almost all of it out of core Views (and Aura, too), esp. e.g. the
Mask stuff.

[tdanderson, 2013-10-29 17:30:00]

Can you please take another look?

Ben, I moved HitTestSource out of View for now and will move it (and my other
rect-based targeting utility functions) to ui/events in a follow up patch
crbug.com/305795. With that said, does this patch look OK to land?

https://codereview.chromium.org/32463003/diff/710001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/710001/ui/views/view.h#newcode116
ui/views/view.h:116: enum HitTestSource {
On 2013/10/28 20:23:29, Ben Goodger (Google) wrote:
> let's not add this to views. Instead we should delegate more of this to the
> client.

I moved this to ui/views/rect_based_targeting_utils.h for the time being, and
will be moving all of rect_based_targeting_utils to ui/events (so they are in
the ui:: namespace) in a follow up patch: crbug.com/305795

[Ben Goodger (Google), 2013-10-30 17:46:54]

https://codereview.chromium.org/32463003/diff/930001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/32463003/diff/930001/ui/views/view.h#newcode1130
ui/views/view.h:1130: virtual void GetHitTestMask(HitTestSource source,
gfx::Path* mask) const;
I didn't just mean move the enum out of View, I meant not wire this through. I'd
rather see all of this hit testing logic moved out of View entirely than add
more client-specific pass-thrus here.

[Ben Goodger (Google), 2013-11-04 20:14:03]

lgtm

https://codereview.chromium.org/32463003/diff/930001/ui/views/rect_based_targ...
File ui/views/rect_based_targeting_utils.h (right):

https://codereview.chromium.org/32463003/diff/930001/ui/views/rect_based_targ...
ui/views/rect_based_targeting_utils.h:22: enum HitTestSource {
just go ahead and inline this into View now, as you had before. Now that
Sadrul's shared your plan for refactoring we can do this for time time being.
Please make sure to add a TODO that this is obsoleted by EventTargeter.

[tdanderson, 2013-11-04 20:36:06]

Ben, I added the TODO as requested and am going to put this into the CQ. Thanks
a lot for following up with me on this.

https://codereview.chromium.org/32463003/diff/930001/ui/views/rect_based_targ...
File ui/views/rect_based_targeting_utils.h (right):

https://codereview.chromium.org/32463003/diff/930001/ui/views/rect_based_targ...
ui/views/rect_based_targeting_utils.h:22: enum HitTestSource {
On 2013/11/04 20:14:04, Ben Goodger (Google) wrote:
> just go ahead and inline this into View now, as you had before. Now that
> Sadrul's shared your plan for refactoring we can do this for time time being.
> Please make sure to add a TODO that this is obsoleted by EventTargeter.

Done.

[commit-bot: I haz the power, 2013-11-04 20:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/32463003/1200001

[commit-bot: I haz the power, 2013-11-04 21:14:37]

Change committed as 232797