Solved 2 bugs which caused Chrome to maximize itself when...

Solved 2 bugs which caused Chrome to maximize itself whendouble clicking, either on the new tab button, on the closetab button or on a single tab.BUG=2827
BUG=3787
The problem comes from the Windows event sequence upon adouble-click (simplified here):1 - hit-test2 - mouse-down4 - mouse-up/click5 - hit-test6 - mouse down7 - mouse up/double-clickThe 1st hit-test is always performed correctly, returningclient for tabs and non-client for the tab-strip (background).The 2nd hit test is not performed correctly to avoid crashesin Chromebot from events being processed while tabs are animating.Since we have no record of these crashes, Ben prefers we keepthis special-case, even though we are responding incorrectlyto the windows hit-test. So, when the tabs are animating wereturn a HTNOWHERE hit which the caller translates into anHTCAPTION hit. This even though a tab-control (new-tab/close-tab)may have been hit.The problem is that having returned HTCAPTION to Windows defaultmessage handling, we get a NON-CLIENT double-click event insteadof a standard one.To keep the behavior of the second hit-test AND prevent theChrome window from maximizing, this change simply declaresthe non-client double-click as handled when the tabs areanimating.Another trick we pulled in the hit-test is to return HTCAPTIONwhen a single tab is present. This allows the entire window to be dragged but causes the context menu to be wrong and the windowto maximize when double clicking on the single tab.The solution here is to correct return a client hit for a singletab and, upon handling a client single-click, delegate to thenon-client single-click default handler.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9953

[idanan, 2009-02-11 22:19:59]

Peter, I put you as a reviewer because I don't know any better ;) but feel free
to change it to someone else.

[Peter Kasting, 2009-02-11 22:27:39]

I don't understand the sentence "The 2nd hit test is not performed correctly to
avoid crashes in Chromebot from events being processed while tabs are
animating."  Are you saying we purposefully return HTNOWHERE for hit tests
during tab animations?

If so, then I'm not sure how this helps us avoid crashes.  What does this really
buy us?

So, it seems to me like making the hit test correct would be a good thing. 
However, that won't address the symptom here in at least some cases (e.g. first
click on last tab's close box, and second click on the blank area behind the
tabs as the tab animates closed).  For that it seems like there's another
relevant bug I filed a while ago, where we should (at the views level) discard a
double-click when the relevant single click wasn't also sent to that view.  For
an implementation of this, one could imagine the single click handler for views
keeping a process-global View* of the last view to receive a single click and
simply comparing before dispatching a double-click.

[Peter Kasting, 2009-02-12 17:46:03]

(Note: please reply on Rietveld since it doesn't scrape your emails)

It sounds to me like the right thing to do is in fact to fix the HTNOWHERE test,
but to do it as a completely separate patch, since as I said I don't think it
solves the problem on this bug anyway.  Then when we check it in we can watch
the stability numbers and see if things get worse.  Bandaids are already bad
enough when we know they help something, I don't think we should keep them in if
we can't recall why they're around.

For the patch here, I still suggest that the right fix for the double-click
issue is for Views to discard double-clicks sent to the wrong view.

[idanan, 2009-02-12 19:25:31]

As I said earlier, I agree that fixing the hit-test to return the correct answer
is the ideal solution. Since Ben was leery of that change, you'll have to
negotiate with him ;) It is a very simple change and I already showed it to him.

For the global state, it seems that it could be static in
RootView::OnMousePressed which handles before single-click and double-lick, as
well as finding the target view.

Do you see a problem with the point culling for double-clicks that would reach
different views as their single click component? Or did you have in mind
something in WidgetWin? Seems more tricky there because there are so many
variants of mouse-down/up/double-click.

[Peter Kasting, 2009-02-12 19:46:43]

> Since Ben was leery of that change, you'll have to
> negotiate with him ;) It is a very simple change and I already showed it to
him.

Please file a bug (and optionally submit a patch there, if you have it sitting
around).

> For the global state, it seems that it could be static in
> RootView::OnMousePressed which handles before single-click and double-lick, as
> well as finding the target view.

Sure, OK.  (I'm not as up on the Views event dispatch as Ben is.)

> Do you see a problem with the point culling for double-clicks that would reach
> different views as their single click component?

I'm confused.  Are you asking me if I see a problem with my own suggestion?  It
seems like the answer is "obviously no"... or were you asking me about the
existing solution in this patch (don't dispatch non-client double-clicks while
tabs are animating)?  If the latter, yes, just hit a keyboard shortcut that
triggers animations (e.g. ctrl-t) and, while it's animating, quickly
double-click the titlebar and watch it be discarded.

> Or did you have in mind
> something in WidgetWin? Seems more tricky there because there are so many
> variants of mouse-down/up/double-click.

Like I said, I'm not fluent enough in this code to know precisely where the
correct hook point would be.

[idanan, 2009-02-13 03:31:59]

> Please file a bug (and optionally submit a patch there, if you have it sitting
around).
In progress.

> > For the global state, it seems that it could be static in
> > RootView::OnMousePressed which handles before single-click and double-lick,
as
> > well as finding the target view.
> 
> Sure, OK.  (I'm not as up on the Views event dispatch as Ben is.)
> 
> > Do you see a problem with the point culling for double-clicks that would
reach
> > different views as their single click component?
> 
> I'm confused.  Are you asking me if I see a problem with my own suggestion? 
It
> seems like the answer is "obviously no"... or were you asking me about the
> existing solution in this patch (don't dispatch non-client double-clicks while
> tabs are animating)?  If the latter, yes, just hit a keyboard shortcut that
> triggers animations (e.g. ctrl-t) and, while it's animating, quickly
> double-click the titlebar and watch it be discarded.
> 
> > Or did you have in mind
> > something in WidgetWin? Seems more tricky there because there are so many
> > variants of mouse-down/up/double-click.
> 
> Like I said, I'm not fluent enough in this code to know precisely where the
> correct hook point would be.

That more-or-less answered my question. This turned out rather tricky but here
it is.
There may be over ways to factor this out but I aimed for the least amount of
code
duplication.

[Peter Kasting, 2009-02-13 19:44:57]

The idea of manually sending a WM_NCLBUTTONDOWN had not occurred to me... I had
assumed the dispatch code itself, rather than the tab, would do translation like
this.  Not sure if one way is better than the other, I guess the difference
would be that with my way we'd encode the "dragging one tab drags the window"
idea in the root view instead of the tab.  Thoughts?

Mostly looks good otherwise, minor comments below.

http://codereview.chromium.org/21268/diff/36/1008
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/21268/diff/36/1008#newcode162
Line 162: if (delegate_->IsLonelyTab()) {
Musing: Should this particular bit be in an IsLeftMouseButton(), instead of
IsOnlyLeftMouseButton()? ...  also, does this allow us to remove middle-click
handling from the nonclient handling code (which we needed to allow
middle-click-closing the last tab)?

http://codereview.chromium.org/21268/diff/36/1009
File chrome/browser/views/tabs/tab.h (right):

http://codereview.chromium.org/21268/diff/36/1009#newcode74
Line 74: virtual bool IsLonelyTab() const = 0;
Nit: "ContainsExactlyOneTab" would be better.

http://codereview.chromium.org/21268/diff/36/1011
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/21268/diff/36/1011#newcode1026
Line 1026: return GetTabCount() < 2;
Maybe this should be "== 1"?

http://codereview.chromium.org/21268/diff/36/1015
File chrome/views/event.h (right):

http://codereview.chromium.org/21268/diff/36/1015#newcode160
Line 160: EF_IS_DOUBLE_CLICK = 1 << 16, EF_IS_NON_CLIENT = 1 << 17
Nit: Put this on a separate line for clarity

http://codereview.chromium.org/21268/diff/36/1013
File chrome/views/root_view.cc (right):

http://codereview.chromium.org/21268/diff/36/1013#newcode271
Line 271: // we are not handling it quickly except for the double-click because
we
Nit: Remove trailing whitespace from this line

http://codereview.chromium.org/21268/diff/36/1013#newcode275
Line 275: (e.GetFlags() & MouseEvent::EF_IS_DOUBLE_CLICK) == 0) {
Nit: I'd prefer "!" to "== 0" for a bit-test, especially since it eliminates any
initial confusion as to whether the == 0 applies to both the previous flags
(what I thought at first glance)

http://codereview.chromium.org/21268/diff/36/1013#newcode349
Line 349: // for the double-click than we did for the single-click, then mark it
as
Nit: "than" -> "as"

http://codereview.chromium.org/21268/diff/36/1014
File chrome/views/widget_win.cc (right):

http://codereview.chromium.org/21268/diff/36/1014#newcode712
Line 712: bool dbl_click, bool non_client) {
Nit: I think Google style here would be each param on its own line?

http://codereview.chromium.org/21268/diff/36/1012
File chrome/views/widget_win.h (right):

http://codereview.chromium.org/21268/diff/36/1012#newcode466
Line 466: bool ProcessMousePressed(const CPoint& point, UINT flags,
Nit: I think Google style here would be each param on its own line?

[idanan, 2009-02-17 20:18:35]

On 2009/02/13 19:44:57, pkasting wrote:
> The idea of manually sending a WM_NCLBUTTONDOWN had not occurred to me... I
had
> assumed the dispatch code itself, rather than the tab, would do translation
like
> this.  Not sure if one way is better than the other, I guess the difference
> would be that with my way we'd encode the "dragging one tab drags the window"
> idea in the root view instead of the tab.  Thoughts?

Well, I hadn't thought of this either, so I tried it. What I found very tedious
is that we would need to abstract the concept of dragable-client-area all the
way to the root view and have the tab implement it. It would result in the same
behavior but there would be more code involved.

Note: Added a minor fix since the last patch for when we captured the mouse
_after_ dragging the window using the single tab and one mouse message got
swallowed by the tab.

> Mostly looks good otherwise, minor comments below.
Thanks.
 
> http://codereview.chromium.org/21268/diff/36/1008
> File chrome/browser/views/tabs/tab.cc (right):
> 
> http://codereview.chromium.org/21268/diff/36/1008#newcode162
> Line 162: if (delegate_->IsLonelyTab()) {
> Musing: Should this particular bit be in an IsLeftMouseButton(), instead of
> IsOnlyLeftMouseButton()? ...  also, does this allow us to remove middle-click
> handling from the nonclient handling code (which we needed to allow
> middle-click-closing the last tab)?

I could be convinced otherwise but I prefer to be more specific than general
here.
Perhaps one day we can define other behaviors with a combination of buttons, so
I
rather people learn to drag with only one button pressed.

> http://codereview.chromium.org/21268/diff/36/1009
> File chrome/browser/views/tabs/tab.h (right):
> 
> http://codereview.chromium.org/21268/diff/36/1009#newcode74
> Line 74: virtual bool IsLonelyTab() const = 0;
> Nit: "ContainsExactlyOneTab" would be better.
I can change it but a lonely tab parallels our sad tab. Note
that the lonely tab is not sad about being lonely ;)
 
> http://codereview.chromium.org/21268/diff/36/1011
> File chrome/browser/views/tabs/tab_strip.cc (right):
> 
> http://codereview.chromium.org/21268/diff/36/1011#newcode1026
> Line 1026: return GetTabCount() < 2;
> Maybe this should be "== 1"?
During init, there is a moment when GetTabCount() == 0, so I rather
take care of both cases here.
 
> http://codereview.chromium.org/21268/diff/36/1015
> File chrome/views/event.h (right):
> 
> http://codereview.chromium.org/21268/diff/36/1015#newcode160
> Line 160: EF_IS_DOUBLE_CLICK = 1 << 16, EF_IS_NON_CLIENT = 1 << 17
> Nit: Put this on a separate line for clarity
Done.

> http://codereview.chromium.org/21268/diff/36/1013
> File chrome/views/root_view.cc (right):
> 
> http://codereview.chromium.org/21268/diff/36/1013#newcode271
> Line 271: // we are not handling it quickly except for the double-click
because
> we
> Nit: Remove trailing whitespace from this line
Done.

> http://codereview.chromium.org/21268/diff/36/1013#newcode275
> Line 275: (e.GetFlags() & MouseEvent::EF_IS_DOUBLE_CLICK) == 0) {
> Nit: I'd prefer "!" to "== 0" for a bit-test, especially since it eliminates
any
> initial confusion as to whether the == 0 applies to both the previous flags
> (what I thought at first glance)
Done.

> http://codereview.chromium.org/21268/diff/36/1013#newcode349
> Line 349: // for the double-click than we did for the single-click, then mark
it
> as
> Nit: "than" -> "as"
Done.

> http://codereview.chromium.org/21268/diff/36/1014
> File chrome/views/widget_win.cc (right):
> 
> http://codereview.chromium.org/21268/diff/36/1014#newcode712
> Line 712: bool dbl_click, bool non_client) {
> Nit: I think Google style here would be each param on its own line?
Done.

> http://codereview.chromium.org/21268/diff/36/1012
> File chrome/views/widget_win.h (right):
> 
> http://codereview.chromium.org/21268/diff/36/1012#newcode466
> Line 466: bool ProcessMousePressed(const CPoint& point, UINT flags,
> Nit: I think Google style here would be each param on its own line?
Done.

[Peter Kasting, 2009-02-18 06:17:34]

On 2009/02/17 20:18:35, idanan wrote:
> > http://codereview.chromium.org/21268/diff/36/1008
> > File chrome/browser/views/tabs/tab.cc (right):
> > 
> > http://codereview.chromium.org/21268/diff/36/1008#newcode162
> > Line 162: if (delegate_->IsLonelyTab()) {
> > Musing: Should this particular bit be in an IsLeftMouseButton(), instead of
> > IsOnlyLeftMouseButton()?
> 
> I could be convinced otherwise but I prefer to be more specific than general
> here.
> Perhaps one day we can define other behaviors with a combination of buttons,
so
> I
> rather people learn to drag with only one button pressed.

My concern is in matching Windows native behavior.  Windows native allows you to
drag a window's titlebar with the left button even if you already had the middle
button down, fore xample.  I think we should match that.

> > http://codereview.chromium.org/21268/diff/36/1009
> > File chrome/browser/views/tabs/tab.h (right):
> > 
> > http://codereview.chromium.org/21268/diff/36/1009#newcode74
> > Line 74: virtual bool IsLonelyTab() const = 0;
> > Nit: "ContainsExactlyOneTab" would be better.
> I can change it but a lonely tab parallels our sad tab. Note
> that the lonely tab is not sad about being lonely ;)

I really would prefer it changed as when reading the code, "lonely tab" is a
real head-scratcher.

> > http://codereview.chromium.org/21268/diff/36/1011
> > File chrome/browser/views/tabs/tab_strip.cc (right):
> > 
> > http://codereview.chromium.org/21268/diff/36/1011#newcode1026
> > Line 1026: return GetTabCount() < 2;
> > Maybe this should be "== 1"?
> During init, there is a moment when GetTabCount() == 0, so I rather
> take care of both cases here.

But if GetTabCount() == 0, there are no tabs to click on, so we can't be calling
this, can we?  It seems like checking for exactly one tab is better.

The rest of this update looks OK.

[idanan, 2009-02-18 15:40:13]

On 2009/02/18 06:17:34, pkasting wrote:
> On 2009/02/17 20:18:35, idanan wrote:
> > > http://codereview.chromium.org/21268/diff/36/1008
> > > File chrome/browser/views/tabs/tab.cc (right):
> > > 
> > > http://codereview.chromium.org/21268/diff/36/1008#newcode162
> > > Line 162: if (delegate_->IsLonelyTab()) {
> > > Musing: Should this particular bit be in an IsLeftMouseButton(), instead
of
> > > IsOnlyLeftMouseButton()?
> > 
> > I could be convinced otherwise but I prefer to be more specific than general
> > here.
> > Perhaps one day we can define other behaviors with a combination of buttons,
> so
> > I
> > rather people learn to drag with only one button pressed.
> 
> My concern is in matching Windows native behavior.  Windows native allows you
to
> drag a window's titlebar with the left button even if you already had the
middle
> button down, fore xample.  I think we should match that.

Done. I didn't know about this behavior.

> > > http://codereview.chromium.org/21268/diff/36/1009
> > > File chrome/browser/views/tabs/tab.h (right):
> > > 
> > > http://codereview.chromium.org/21268/diff/36/1009#newcode74
> > > Line 74: virtual bool IsLonelyTab() const = 0;
> > > Nit: "ContainsExactlyOneTab" would be better.
> > I can change it but a lonely tab parallels our sad tab. Note
> > that the lonely tab is not sad about being lonely ;)
> 
> I really would prefer it changed as when reading the code, "lonely tab" is a
> real head-scratcher.

Done. The sad tab is lonely again now ;)

> > > http://codereview.chromium.org/21268/diff/36/1011
> > > File chrome/browser/views/tabs/tab_strip.cc (right):
> > > 
> > > http://codereview.chromium.org/21268/diff/36/1011#newcode1026
> > > Line 1026: return GetTabCount() < 2;
> > > Maybe this should be "== 1"?
> > During init, there is a moment when GetTabCount() == 0, so I rather
> > take care of both cases here.
> 
> But if GetTabCount() == 0, there are no tabs to click on, so we can't be
calling
> this, can we?  It seems like checking for exactly one tab is better.
> 
> The rest of this update looks OK.

Done.

[Peter Kasting, 2009-02-18 16:58:24]

LGTM.

http://codereview.chromium.org/21268/diff/2001/3002
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/21268/diff/2001/3002#newcode163
Line 163: SendMessage(GetWidget()->GetHWND(), WM_NCLBUTTONDOWN,
Nit: Please add a comment explaining what the effect of this block is.

http://codereview.chromium.org/21268/diff/2001/3009
File chrome/views/view.cc (right):

http://codereview.chromium.org/21268/diff/2001/3009#newcode456
Line 456: if (enabled && e.IsOnlyLeftMouseButton() && HitTest(e.location()))
Nit: Maybe this should also be changed to IsLeftMouseButton(), to match your
change elsewhere?  (and perhaps use ?: to match your change just below)

[idanan, 2009-02-18 18:02:22]

Thanks for all the feedback Peter!

http://codereview.chromium.org/21268/diff/2001/3002
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/21268/diff/2001/3002#newcode163
Line 163: SendMessage(GetWidget()->GetHWND(), WM_NCLBUTTONDOWN,
On 2009/02/18 16:58:25, pkasting wrote:
> Nit: Please add a comment explaining what the effect of this block is.

Done.

http://codereview.chromium.org/21268/diff/2001/3009
File chrome/views/view.cc (right):

http://codereview.chromium.org/21268/diff/2001/3009#newcode456
Line 456: if (enabled && e.IsOnlyLeftMouseButton() && HitTest(e.location()))
Tried to drag with both left and right buttons on Windows and that does not
work. So, to imitate windows better keep dragging on only one button down. (In
the case of having 2 boolean operations in the condition, I prefer using in if 
to maintain readability).

[Peter Kasting, 2009-02-18 18:04:30]

http://codereview.chromium.org/21268/diff/2001/3009
File chrome/views/view.cc (right):

http://codereview.chromium.org/21268/diff/2001/3009#newcode456
Line 456: if (enabled && e.IsOnlyLeftMouseButton() && HitTest(e.location()))
On 2009/02/18 18:02:23, idanan wrote:
> Tried to drag with both left and right buttons on Windows and that does not
> work. So, to imitate windows better keep dragging on only one button down.

Did you try left + middle?

[idanan, 2009-02-18 18:12:25]

http://codereview.chromium.org/21268/diff/2001/3009
File chrome/views/view.cc (right):

http://codereview.chromium.org/21268/diff/2001/3009#newcode456
Line 456: if (enabled && e.IsOnlyLeftMouseButton() && HitTest(e.location()))
Seems like Windows has some complicated logic there because left-middle works on
my Trackman (but not left-right or middle-right) but on the Thinkpad only
left-drag works (not left-middle or left-right).