Issue 358: Can't open a new tab by middle-clicking home button...

Issue 6477: Support modifier clicks on UI elements

Mouse event flags transformed to WindoOpenDisposition so that
browser commands can all inteterpert event modifiers. Implemented
home, forward and backwards middle-click andshift middle click.

Previously working on issue 358 which was market a duplicate
and concerned with the home and tabstrip subset of this behavior:
Can't open a new tab by middle-clicking home button.

No functionality has been lost or altered, only the use ofmodifiers
give access to increased functionality. 

BUG=6477
TBR=ben@chromium.org

[Ben Goodger (Google), 2009-01-15 19:07:16]

http://codereview.chromium.org/17428/diff/1/4
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/17428/diff/1/4#newcode707
Line 707: return (e.GetFlags() & MouseEvent::EF_MIDDLE_BUTTON_DOWN) != 0;
What's this change? This doesn't seem related to the change. I'm very leery of
middle clicking on static area doing anything. After a brief discussion with the
UI folk I don't think we want this part of the change.

http://codereview.chromium.org/17428/diff/1/6
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/17428/diff/1/6#newcode72
Line 72: return mouse_event_flags() & MK_MBUTTON ?  IDC_NEW_HOME_TAB: IDC_HOME;
I don't think we should have a specialization just for the home button. Since
this is likely to be wanted elsewhere can't we make the Button class provide
some means for this? What I mean is maybe ButtonListener's ButtonPressed method
could have an additional parameter that is the event flags.

[Peter Kasting, 2009-01-15 20:46:01]

On 2009/01/15 19:07:16, Ben Goodger wrote:
> I don't think we should have a specialization just for the home button.

I agree.  This has come up in other contexts.  The right fix for this bug should
plumb clicks on various UI buttons through DispositionForAction().  Filed issue
6477 about this and duped a lot of other bugs against it.  We don't want to do a
middle-click-specific fix, or a home-button-specific fix.

[Itai, 2009-01-15 22:04:20]

http://codereview.chromium.org/17428/diff/1/4
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/17428/diff/1/4#newcode707
Line 707: return (e.GetFlags() & MouseEvent::EF_MIDDLE_BUTTON_DOWN) != 0;
Well, it as not in the top issue but it was suggested as part of the bug
discussion and got a positive feedback and no objections.

I understand that like other shortcuts (keyboard ones for example) it is not
discoverable except by people who expect it there. The good thing is that this
has zero impact (just like the existence of CTRL-T shortcut or middle-click on a
tab to remove it) to those who do not use it, plus it does provide useful
additional functionality.

It has been commented in a few areas that the new tab button is small and an
easier way to create tabs would be useful. Just like middle-clicking a tab says
that it should be destroyed, middle-clicking the tab-strip can say that a new
tab should be created there. 

I'll copy this information on the bug discussion to see if there can be an
unknown reason why it would have a negative impact.

http://codereview.chromium.org/17428/diff/1/6
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/17428/diff/1/6#newcode72
Line 72: return mouse_event_flags() & MK_MBUTTON ?  IDC_NEW_HOME_TAB: IDC_HOME;
Since this functionality is desired in other locations, yes. Do we also collapse
the functionality of the Forward/Backward buttons too? They have their own
specialization to support a drop-down in right-click. If the modifiers are
carried, I do not think that would be needed anymore.

[Itai, 2009-02-03 17:34:16]

Hi Ben,

Here is the code for the generalized version of Issue 358
which is logged as Issue 6477. It transforms mouse-event 
flags into WindowOpenDispostion and routes this data through 
all the layers until the command handlers.

Once the disposition is obtained at the command handler site,
the execution function is responsible for knowing what to do
with it and which modifiers to support. I've defaulted handling
to the CURRENT_TAB disposition which was the old behavior for
modifiers which were let through.

Functionality added in this change:
- Backwards, Forwards, Home and Go support CURRENT_TAB (as previously),
NEW_BACKGROUND_TAB (as originally requested) and NEW_FOREGROUND_TAB
(as implied by Issue 6477).

To limit the scope of the change and reduce potential conflicts,
I have preserved the old APIs (non-disposition) in most locations.
It would be nice to remove the old APIs in areas like NotifyClick
for simplicity reasons when the code is being finalized. Let me 
know if you have a particular preference about this.

Please ignore the automatically reported Lint warnings. I have
corrected them locally but didn't feel the need to upload a new
patch for a few blank spaces.

- Itai

http://codereview.chromium.org/17428/diff/1/4
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/17428/diff/1/4#newcode707
Line 707: return (e.GetFlags() & MouseEvent::EF_MIDDLE_BUTTON_DOWN) != 0;
Obviously the ultimate decision is yours, but this part of the change means a
lot to me. I've been using this feature in my own builds of Chrome for a while
now and it saves me lots of time day-after-day. The tab-strip already responds
to the two other mouse buttons and this functionality does not displace any
other. So, if the only objection is to refuse it on principle, I'd like to have
this reconsidered. It can be removed in the future and clearly won't affect
those who do not use this feature, but for those who do it will be a very
welcome addition.

http://codereview.chromium.org/17428/diff/1/6
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/17428/diff/1/6#newcode72
Line 72: return mouse_event_flags() & MK_MBUTTON ?  IDC_NEW_HOME_TAB: IDC_HOME;
On 2009/01/15 19:07:16, Ben Goodger wrote:
> I don't think we should have a specialization just for the home button. Since
> this is likely to be wanted elsewhere can't we make the Button class provide
> some means for this? What I mean is maybe ButtonListener's ButtonPressed
method
> could have an additional parameter that is the event flags.

Done.

[Peter Kasting, 2009-02-03 17:44:21]

On 2009/02/03 17:34:16, Itai wrote:
> Functionality added in this change:
> - Backwards, Forwards, Home and Go support CURRENT_TAB (as previously),
> NEW_BACKGROUND_TAB (as originally requested) and NEW_FOREGROUND_TAB
> (as implied by Issue 6477).

Any reason not to go ahead and support the rest of the dispositions?  You've hit
on the most useful ones, I think, but I can't see a benefit to arbitrarily
restricting the others.

[Ben Goodger (Google), 2009-02-08 21:58:42]

http://codereview.chromium.org/17428/diff/1/4
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/17428/diff/1/4#newcode707
Line 707: return (e.GetFlags() & MouseEvent::EF_MIDDLE_BUTTON_DOWN) != 0;
Our principles define the type of product we build. For Chrome, this means "less
is more" unless there's a really good reason. This functionality is not
discoverable and atypical for a browser.

http://codereview.chromium.org/17428/diff/601/602
File chrome/browser/browser.h (right):

http://codereview.chromium.org/17428/diff/601/602#newcode229
Line 229: void GoBack(WindowOpenDisposition);
Please supply a parameter name here too.

http://codereview.chromium.org/17428/diff/601/602#newcode328
Line 328: virtual void ExecuteCommandWithDisposition(int id,
WindowOpenDisposition);
I don't like this wiring. A good number of the commands handled by
ExecuteCommand don't have a WindowOpenDisposition. There must be some other way
of doing this.

[Ben Goodger (Google), 2009-03-17 19:41:31]

http://codereview.chromium.org/17428/diff/4001/4016
File chrome/browser/views/frame/browser_frame.cc (right):

http://codereview.chromium.org/17428/diff/4001/4016#newcode188
Line 188: browser_view_->tabstrip()->model()->AddBlankTab(true);
Please remove this.

http://codereview.chromium.org/17428/diff/4001/4011
File chrome/browser/views/go_button.cc (right):

http://codereview.chromium.org/17428/diff/4001/4011#newcode75
Line 75: command_updater_->ExecuteCommandWithDisposition(
see comment in BrowserToolbarView::ButtonPressed. Call the Browser fn directly
from here.

http://codereview.chromium.org/17428/diff/4001/4014
File chrome/browser/views/go_button.h (right):

http://codereview.chromium.org/17428/diff/4001/4014#newcode36
Line 36: virtual bool IsTriggerableEvent(const views::MouseEvent& e);
No need for this, use the property I describe in BrowserToolbarView.

http://codereview.chromium.org/17428/diff/4001/4010
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/17428/diff/4001/4010#newcode74
Line 74: class HomeButton : public views::ImageButton {
Rather than create these subclasses, I think you should augment CustomButton
with a property that is whether or not the middle button triggers the button
action. The CustomButton saves the mouse flags in its mouse_event_flags()
accessor which you can check in the ButtonPressed handler.

http://codereview.chromium.org/17428/diff/4001/4010#newcode849
Line 849: sender->tag(), event_utils::DispositionFromEventFlags(flags));
OK I think I know how this should work now.

For the affected buttons (back, forward, home), you should call the Browser's
impl of their functions directly with the disposition rather than going through
ExecuteCommand. This avoids having to add an extra param to ExecuteCommand that
is mostly ignored. The ExecuteCommand handling for IDC_BACK etc should call the
Browser impl with the default disposition (CURRENT_TAB?)

You will want to verify that this doesn't get triggered when the button is
disabled. It shouldn't be, but please test.

http://codereview.chromium.org/17428/diff/4001/4019
File chrome/views/controls/button/button.h (right):

http://codereview.chromium.org/17428/diff/4001/4019#newcode23
Line 23: virtual void ButtonPressedWithFlags(Button* sender, int flags);
No need to add this method. flags is carried on the button via
mouse_event_flags().

[idanan, 2009-03-19 13:30:38]

On 2009/03/17 19:41:31, Ben Goodger wrote:
> http://codereview.chromium.org/17428/diff/4001/4016
> File chrome/browser/views/frame/browser_frame.cc (right):
> 
> http://codereview.chromium.org/17428/diff/4001/4016#newcode188
> Line 188: browser_view_->tabstrip()->model()->AddBlankTab(true);
> Please remove this.
Removed.

> http://codereview.chromium.org/17428/diff/4001/4011
> File chrome/browser/views/go_button.cc (right):
> 
> http://codereview.chromium.org/17428/diff/4001/4011#newcode75
> Line 75: command_updater_->ExecuteCommandWithDisposition(
> see comment in BrowserToolbarView::ButtonPressed. Call the Browser fn directly
> from here.
Done.

> http://codereview.chromium.org/17428/diff/4001/4014
> File chrome/browser/views/go_button.h (right):
> 
> http://codereview.chromium.org/17428/diff/4001/4014#newcode36
> Line 36: virtual bool IsTriggerableEvent(const views::MouseEvent& e);
> No need for this, use the property I describe in BrowserToolbarView.
Removed.

> http://codereview.chromium.org/17428/diff/4001/4010
> File chrome/browser/views/toolbar_view.cc (right):
> 
> http://codereview.chromium.org/17428/diff/4001/4010#newcode74
> Line 74: class HomeButton : public views::ImageButton {
> Rather than create these subclasses, I think you should augment CustomButton
> with a property that is whether or not the middle button triggers the button
> action. The CustomButton saves the mouse flags in its mouse_event_flags()
> accessor which you can check in the ButtonPressed handler.
Done.

Do note that I feel that putting the flags of the last event (which is rather
transient)
in the button itself is wrong. They should be passed down along the event chain
because
they do not belong to the button's state. It makes the code statefull and
modified the
data-members when the conception state of the button has not changed, thus
making it
harder to understand.

> http://codereview.chromium.org/17428/diff/4001/4010#newcode849
> Line 849: sender->tag(), event_utils::DispositionFromEventFlags(flags));
> OK I think I know how this should work now.
> 
> For the affected buttons (back, forward, home), you should call the Browser's
> impl of their functions directly with the disposition rather than going
through
> ExecuteCommand. This avoids having to add an extra param to ExecuteCommand
that
> is mostly ignored. The ExecuteCommand handling for IDC_BACK etc should call
the
> Browser impl with the default disposition (CURRENT_TAB?)

OK. I fixed things so that the CommandUpdater and its delegate do not change 
their interface. The ExecuteCommand virtual function remains the interface to
the browser. I did however keep ExecuteCommandWithDisposition as an internal
browser function (non-virtual) so that I do not need to duplicate the switch
dispatch (and special handling in ExecuteCommand for corner cases) which is 
needed by the toolbar/location bar when dispatching button events. When a
dispatch is not necesary, then I call the browser function directly. Duplicating
the switch-dispatch from ExecuteCommand would clearly make it more error prone.

> You will want to verify that this doesn't get triggered when the button is
> disabled. It shouldn't be, but please test.
Done.

> http://codereview.chromium.org/17428/diff/4001/4019
> File chrome/views/controls/button/button.h (right):
> 
> http://codereview.chromium.org/17428/diff/4001/4019#newcode23
> Line 23: virtual void ButtonPressedWithFlags(Button* sender, int flags);
> No need to add this method. flags is carried on the button via
> mouse_event_flags().

Done.

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

Feel free to file a bug about removing the stateful nature of mouse event flags,
I don't want to insert a new API that is only partially used in a changelist.

http://codereview.chromium.org/17428/diff/5001/5004
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/17428/diff/5001/5004#newcode554
Line 554: void Browser::GoBack(WindowOpenDisposition disp) {
avoid abbrieviations except where necessary.

http://codereview.chromium.org/17428/diff/5001/5004#newcode1121
Line 1121: int id, WindowOpenDisposition disposition) {
You don't need this function. This should only get called for
IDC_BACK/FORWARD/HOME etc from keybindings which should use the default
disposition.

http://codereview.chromium.org/17428/diff/5001/5003
File chrome/browser/browser.h (right):

http://codereview.chromium.org/17428/diff/5001/5003#newcode252
Line 252: void GoBack(WindowOpenDisposition);
provide parameter names.

http://codereview.chromium.org/17428/diff/5001/5003#newcode367
Line 367: void ExecuteCommandWithDisposition(int id, WindowOpenDisposition);
Remove this.

http://codereview.chromium.org/17428/diff/5001/5011
File chrome/views/controls/button/custom_button.h (right):

http://codereview.chromium.org/17428/diff/5001/5011#newcode47
Line 47: triggerable_event_flags_ = triggerable_event_flags;
unix_hacker style for setters and getters.

[idanan, 2009-03-20 13:55:10]

On 2009/03/19 23:59:03, Ben Goodger wrote:
> Feel free to file a bug about removing the stateful nature of mouse event
flags,
> I don't want to insert a new API that is only partially used in a changelist.

Will do.
 
> http://codereview.chromium.org/17428/diff/5001/5004
> File chrome/browser/browser.cc (right):
> 
> http://codereview.chromium.org/17428/diff/5001/5004#newcode554
> Line 554: void Browser::GoBack(WindowOpenDisposition disp) {
> avoid abbrieviations except where necessary.

Done. Not sure when it is necessary though???

> http://codereview.chromium.org/17428/diff/5001/5004#newcode1121
> Line 1121: int id, WindowOpenDisposition disposition) {
> You don't need this function. This should only get called for
> IDC_BACK/FORWARD/HOME etc from keybindings which should use the default
> disposition.

Wrong, it is called by toolbar view which handles button events. See
line 827. Yes, the function can be removed, IF I duplicate the switch
statement and special case handling from the browser function but that
would certainly be the wrong thing to do. It would not be robust and
easy to maintain, I am sure you can understand why.

> http://codereview.chromium.org/17428/diff/5001/5003
> File chrome/browser/browser.h (right):
> 
> http://codereview.chromium.org/17428/diff/5001/5003#newcode252
> Line 252: void GoBack(WindowOpenDisposition);
> provide parameter names.
Done.

> http://codereview.chromium.org/17428/diff/5001/5003#newcode367
> Line 367: void ExecuteCommandWithDisposition(int id, WindowOpenDisposition);
> Remove this.
See above comment.
 
> http://codereview.chromium.org/17428/diff/5001/5011
> File chrome/views/controls/button/custom_button.h (right):
> 
> http://codereview.chromium.org/17428/diff/5001/5011#newcode47
> Line 47: triggerable_event_flags_ = triggerable_event_flags;
> unix_hacker style for setters and getters.
Done.

[idanan, 2009-03-24 19:18:25]

Made this CL TBR, so that you can look at it when you have more time, as I've
already addressed your comments except the one where you incorrectly assumed the
function was called from one place since last Thursday.

I'll address any constructive comments in a subsequent CL.