Issue 21268: The Remake...

Issue 21268: The RemakeSee http://codereview.chromium.org/21268 for the original issue which was partially reverted due to a possible regression. Issue 8819 was created to track the part of the original fix which was reverted.A double-click come as an event which follows a single-click. The core of this change is to makesure that a double-click arrives at the object which handled the single-click which preceeded it.If it does not, then the double-click is transformed into a single-click at the dispatch level.The tricky part is that a single-click and the double-click which follows it may not both be clientor non-client. So, all double-clicks (client or non-client) are now centrally handled and replacedwhen needed by second single-clicks.Note that the behavior which was reverted was to discard a double-click rather than reducing it toa single-click. The bug filed as a regression involved using a double-middle-click as two single-clicks when destroying compressed tabs sequentially.BUG=8819
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12590

[idanan, 2009-03-19 17:39:41]

Hi Peter,

This code should be familiar to you and is a replacement for the change which
was reverted a few weeks ago. It is slightly simpler and that I now only discard
double-clicks with a mismatch between client and non-client with the
single-click part. For normal double-clicks, I transform them to single-clicks
during dispatch. So, finally, Chrome does not maximize itself accidentally.

[Peter Kasting, 2009-03-19 17:54:00]

I've changed reviewers to Ben since I think he has a better idea of what the
right fix for this issue should be.

[Ben Goodger (Google), 2009-03-25 18:49:55]

OK... I'm sufficiently scared. What bug are you fixing? The one you link to
doesn't reproduce for me anymore at ToT.

[Ben Goodger (Google), 2009-03-25 19:37:55]

The question I have then is why is the close button treated as non-client? This
sounds like an issue with our WM_NCHITTEST handling not the views system event
propagation.

[Ben Goodger (Google), 2009-03-25 20:49:38]

http://codereview.chromium.org/50017/diff/1/7
File chrome/views/event.h (right):

http://codereview.chromium.org/50017/diff/1/7#newcode68
Line 68: void SetFlags(int flags) {
unix_hacker.

http://codereview.chromium.org/50017/diff/1/5
File chrome/views/view.cc (right):

http://codereview.chromium.org/50017/diff/1/5#newcode466
Line 466: e.IsRightMouseButton()? context_menu_controller_ : 0;
space between ) and ?

http://codereview.chromium.org/50017/diff/1/4
File chrome/views/widget/root_view.cc (right):

http://codereview.chromium.org/50017/diff/1/4#newcode272
Line 272: static View* last_click_handler = 0;
I don't think static is what you want here. This is global to all
implementations of this function.

http://codereview.chromium.org/50017/diff/1/4#newcode281
Line 281: last_click_handler = 0;
we use NULL, not 0.

http://codereview.chromium.org/50017/diff/1/4#newcode321
Line 321: const bool handled = mouse_pressed_handler_->ProcessMousePressed(
no const.

[idanan, 2009-03-26 19:04:38]

All Done.

The static object was turned into a member variable, so that it is no longer
shared.
I thought static would prevent a click to happen in a window and the following
double-click
in another, such as double-clicking the X (close window icon) with it over
another chrome
window. After tests, I see that this does not happen, so windows appears to do
something
similar to what I implemented for its windows.

[idanan, 2009-03-26 19:05:35]

Thanks for the feedback. Everything done as requested.

[Ben Goodger (Google), 2009-03-26 19:09:53]

http://codereview.chromium.org/50017/diff/4001/4005
File chrome/views/widget/root_view.cc (right):

http://codereview.chromium.org/50017/diff/4001/4005#newcode283
Line 283: last_click_handler_ = 0;
NULL.

http://codereview.chromium.org/50017/diff/4001/4005#newcode366
Line 366: last_click_handler_ = 0;
NULL

http://codereview.chromium.org/50017/diff/4001/4004
File chrome/views/widget/root_view.h (right):

http://codereview.chromium.org/50017/diff/4001/4004#newcode289
Line 289: // The last view to handle a mouse click.
You might note why this is kept.

http://codereview.chromium.org/50017/diff/4001/4002
File chrome/views/widget/widget_win.cc (right):

http://codereview.chromium.org/50017/diff/4001/4002#newcode724
Line 724: View::ConvertPointToView(NULL, root_view_.get(), &converted_point);
What does NULL mean? What two views are you converting between?

[idanan, 2009-03-26 19:15:47]

http://codereview.chromium.org/50017/diff/4001/4005
File chrome/views/widget/root_view.cc (right):

http://codereview.chromium.org/50017/diff/4001/4005#newcode283
Line 283: last_click_handler_ = 0;
On 2009/03/26 19:09:53, Ben Goodger wrote:
> NULL.

Done.

http://codereview.chromium.org/50017/diff/4001/4005#newcode366
Line 366: last_click_handler_ = 0;
On 2009/03/26 19:09:53, Ben Goodger wrote:
> NULL

Done.

http://codereview.chromium.org/50017/diff/4001/4004
File chrome/views/widget/root_view.h (right):

http://codereview.chromium.org/50017/diff/4001/4004#newcode289
Line 289: // The last view to handle a mouse click.
On 2009/03/26 19:09:53, Ben Goodger wrote:
> You might note why this is kept.

Done.

http://codereview.chromium.org/50017/diff/4001/4002
File chrome/views/widget/widget_win.cc (right):

http://codereview.chromium.org/50017/diff/4001/4002#newcode724
Line 724: View::ConvertPointToView(NULL, root_view_.get(), &converted_point);
This is code Peter added actually because non-client coordinates are in
screen-space, which is why I think one of the view is NULL. Peter?

[Peter Kasting, 2009-03-26 19:19:44]

http://codereview.chromium.org/50017/diff/4001/4002
File chrome/views/widget/widget_win.cc (right):

http://codereview.chromium.org/50017/diff/4001/4002#newcode724
Line 724: View::ConvertPointToView(NULL, root_view_.get(), &converted_point);
Yes, that's correct.  This usage of NULL is documented on the
ConvertPointToView() declaration in its header.

[Ben Goodger (Google), 2009-03-26 19:23:27]

LGTM then