Fixed issue where middle click doesn't activiate when not over its tab....

Fixed issue where middle click doesn't activiate when not over its tab.Since SetCaptured is set, the only way fixing this is checking its boundsand mouse bounds.
BUG=1062
(http://crbug.com/1062)

[Mohamed Mansour (USE mhm), 2008-11-30 08:33:25]

I believe the bug has been fixed, review please :) Thanks for aboodman from IRC
to inform me about the bounds() method from View

[ultraklon, 2008-11-30 14:23:18]

I'm not a reviewer, just a question. Will the compiler optimize that temporary
variables mouseX, mouseY, boundX, boundY to make them inline or is better to
write it like

if (mouseX <= bounds().width() && mouseY <= bounds().height() &&
event.location().x() >= 0 && event.location().y() >= 0)

i know, is less readable, but is it faster?

[Mohamed Mansour (USE mhm), 2008-11-30 15:04:19]

Ah, there is a method within the View called HitTest which you send it  a point.
It works great! 

  if (event.IsMiddleMouseButton() && HitTest(event.location())) {
      delegate_->CloseTab(this);
  }


Upated it

[Evan Martin, 2008-11-30 18:56:34]

+beng, master of tabs

[Evan Martin, 2008-11-30 18:56:56]

try two on email address

[Mohamed Mansour (USE mhm), 2008-11-30 18:58:44]

On 2008/11/30 18:56:56, Evan Martin wrote:
> try two on email address

What do you mean try two on email address?

[Peter Kasting, 2008-12-01 00:32:35]

Does this fix the similar issue with middle-click on the tab's close button (and
release off the tab)?

http://codereview.chromium.org/12816/diff/206/10
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/12816/diff/206/10#newcode159
Line 159: // Check if middle mouse button is pressed within the bounds of the
tab.
Nit: A better comment might be "Close tabs on middle click, but only if the
button is released over the tab (normal windows behavior is to discard presses
of a UI element where the releases happen off the element)."

Should maybe add a newline above this too.

http://codereview.chromium.org/12816/diff/206/10#newcode160
Line 160: if (event.IsMiddleMouseButton() && HitTest(event.location())) {
Nits: Don't add braces; don't change 2-space indent to 4-space.

[Ben Goodger (Google), 2008-12-01 00:35:37]

LGTM with Peter's nits fixed.

On 2008/12/01 00:32:35, pkasting wrote:
> Does this fix the similar issue with middle-click on the tab's close button
(and
> release off the tab)?
> 
> http://codereview.chromium.org/12816/diff/206/10
> File chrome/browser/views/tabs/tab.cc (right):
> 
> http://codereview.chromium.org/12816/diff/206/10#newcode159
> Line 159: // Check if middle mouse button is pressed within the bounds of the
> tab.
> Nit: A better comment might be "Close tabs on middle click, but only if the
> button is released over the tab (normal windows behavior is to discard presses
> of a UI element where the releases happen off the element)."
> 
> Should maybe add a newline above this too.
> 
> http://codereview.chromium.org/12816/diff/206/10#newcode160
> Line 160: if (event.IsMiddleMouseButton() && HitTest(event.location())) {
> Nits: Don't add braces; don't change 2-space indent to 4-space.

[Mohamed Mansour (USE mhm), 2008-12-01 01:15:13]

Okay, thanks guys, I updated the recommendations. I really like how you guys
explain your comments. :)

[Peter Kasting, 2008-12-01 04:34:50]

On 2008/12/01 01:15:13, Mohamed Mansour wrote:
> Okay, thanks guys, I updated the recommendations.

The patch LGTM.  Did you check the behavior when the down-click is on the tab's
close button, rather than the rest of the tab?  The button is a separate element
so I'm curious whether we need additional patches.  The desired behavior is that
when clicking on the button:
* Releasing on the button closes the tab
* Releasing off the button, but on the tab, closes the tab
* Releasing off the tab is ignored

The second of these points is weird if you think of the button and tab as
distinct elements, but from the user's point of view, when middle-clicking a
tab, the button and the rest of the tab are all part of one unit.

Before your patch, the behavior is that all three actions close the tab.  Your
patch might make things Just Work but it would be good to test now instead of
missing this edge case.

[Mohamed Mansour (USE mhm), 2008-12-01 05:01:03]

Hey Peter, you make a valid point, I verified that those behaviors work. The
button belongs within that tab area, last time I looked. And the HitTest checks
if the mouse hits those boundaries. So I guess its LGTM :)

[Mohamed Mansour (USE mhm), 2008-12-01 05:08:01]

Ben, the AUTHORS file in E:\chromium\src ?  Something went wrong with my build,
so I am going to update everything and rebuild it. Will do it tomorrow.

[Peter Kasting, 2008-12-01 23:54:34]

Committed in r6184.  Closing.