Make extensions icons easier to target with gestures

Make extensions icons easier to target with gestures

The bounds of a BrowserActionView and the bounds
of its child view |button_| have the following
properties:

- Both share a common center point.
- The bounds of |button_| are square, but the 
bounds of BAV are not.
- The bounds of |button_| are contained within
the bounds of BAV.
- Both are small enough to be easily covered
by a finger.

This is a flaw in rect-based targeting
making it difficult to correctly target
|button_| with a tap gesture. To obtain the
desired behavior, use the distance
between the center of the touch and the center
of the view (instead of the center line of
the view) to break ties among rect-based
targeting candidates.

The use of the distance to center line was
to avoid biases against wide or tall views. 
But this is not necessary because any view 
which can have at least 60% of its area 
covered by a touch cannot be sufficiently wide 
or tall enough to benefit from such a metric.

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

[tdanderson, 2013-12-16 03:17:52]

Sadrul, can you please have a first look over this patch and suggest the best
way to test?

[sadrul, 2013-12-17 04:21:20]

https://codereview.chromium.org/94003003/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/94003003/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar/browser_action_view.cc:113: return
button_->GetEventHandlerForRect(rect_in_child_coords);
What does this return if |rect_in_child_coords| is empty (i.e. none of |rect|
falls inside the button)?

Why does this fail with the fuzzing logic?

Should you just return button_ when is_touch_down()?

[tdanderson, 2013-12-17 20:07:27]

Sadrul, please see my comments below.

https://codereview.chromium.org/94003003/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/94003003/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar/browser_action_view.cc:113: return
button_->GetEventHandlerForRect(rect_in_child_coords);
On 2013/12/17 04:21:20, sadrul wrote:
> What does this return if |rect_in_child_coords| is empty (i.e. none of |rect|
> falls inside the button)?

This will return |this| based on how View::GetEventHandlerForRect()
is currently implemented. I checked the old implementation of
View::GetEventHandlerForPoint() (before I landed rect-based targeting) and it
also returns |this| regardless of the location of |point| when called on a leaf
view:

https://src.chromium.org/viewvc/chrome/branches/1700/src/ui/views/view.cc?rev...

Both seem wrong, but in practice this call should never be made because of the
hit test against the children's bounds in View::GetEventHandlerForRect()... if
the hit test is false, the recursive call is never made on the child.

So it seems that an implied precondition of GetEventHandlerForRect() is that the
hit test for |this| against |rect| should always be true. It would be nice to
have a DCHECK for this at the top of the function, but:

- This would mean lots more hit testing, and
- It seems unnecessary since in practice all calls to GetEventHandlerForRect()
must have originally stemmed from the call in RootView::DispatchGestureEvent()
and therefore subsequent calls are only made after a successful hit test against
the child's bounds.
 
> Why does this fail with the fuzzing logic?

I attached a diagram to the bug to explain:
https://code.google.com/p/chromium/issues/detail?id=328182#c7
 
> Should you just return button_ when is_touch_down()?

It seems more correct to do it by calling 
button_->GetEventHandlerForRect(). Are you just suggesting this for simplicity?
If so, perhaps views::UsePointBasedTargeting() is better to call.

[tdanderson, 2013-12-18 18:35:01]

Scott, can you please take a look (and suggest the preferred way to add test
coverage)?

[sky, 2013-12-18 21:57:46]

I think you're working around a flaw in the logic of View and not fixing the
central bug. Subclasses shouldn't have to worry about this if they happen to
position children like this.

[tdanderson, 2013-12-19 00:53:23]

On 2013/12/18 21:57:46, sky wrote:
> I think you're working around a flaw in the logic of View and not fixing the
> central bug. Subclasses shouldn't have to worry about this if they happen to
> position children like this.

Yes, you're right. I thought about it some more and it looks like the way
forward is to simply measure distances between center points instead
of between the center point of the touch and the center line of the
view. It not only fixes this bug, but it turns out that using the center
line wasn't even needed to begin with (this was an oversight on my part).
I was using it to prevent wide views such as the omnibox from being at
a disadvantage, but distance measurements only matter for views which
have at least 60% of their area covered by a touch region, and the omnibox
would not fall into this category.

Can you please have another look? If this LGTY, I will update
https://sites.google.com/a/chromium.org/dev/developers/design-documents/views...
accordingly.

[sky, 2013-12-19 16:28:26]

This seems fine to me. Do you have test coverage of the case BAV and its button_
were triggering though?

[tdanderson, 2013-12-19 18:44:32]

On 2013/12/19 16:28:26, sky wrote:
> This seems fine to me. Do you have test coverage of the case BAV and its
button_
> were triggering though?

More test coverage added. I also added a link to the design document in the
test file and in view.h. Can you please have another look?

[sky, 2013-12-19 20:52:01]

LGTM

[commit-bot: I haz the power, 2013-12-19 20:56:34]

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

[commit-bot: I haz the power, 2013-12-19 22:44:12]

Change committed as 241955