Mac: implement dispositions for back/forward buttons (not in drop-downs).

Mac: implement dispositions for back/forward buttons (not in drop-downs).

That is, holding Cmd while pressing back/forward will open in a new background tab and holding Cmd+Shift will open in a new foreground tab. Theoretically, it can support holding Shift to open in a new window, but Browser::GoBack()/GoForward() doesn't support this disposition (due to a technical "limitation" of the implementation?).

This does *not* implement dispositions in the drop-down menus.

BUG=25319
TEST=Navigate to build up some history. Then press back/forward while holding down Cmd or Cmd+Shift.

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

[viettrungluu, 2009-10-23 22:58:50]

Self-review ... (new version uploaded).

http://codereview.chromium.org/330002/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/330002/diff/1/2#newcode609
Line 609: if (disposition != CURRENT_TAB) {
Oops. Browser::GoBack/Forward() only properly supports NEW_FOREGROUND_TAB,
NEW_BACKGROUND_TAB, and CURRENT_TAB dispositions, so we should check for the
former two and let any others -- e.g., NEW_WINDOW -- fall through the normal
code path for CURRENT_TAB.

[Nico, 2009-10-24 20:34:05]

http://codereview.chromium.org/330002/diff/1002/1003
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/330002/diff/1002/1003#newcode611
Line 611: // fall through to the default (current tab) case.
I don't like how this knows about the internals of ECWD().

Is there any harm in always or never calling Revert()? Windows seems to call it
always (c/b/views/toolbar_view.cc, ButtonPressed() ), Gtk never
(c/b/gtk/back_forward_button_gtk.cc , OnClick() ). Should
ExecuteCommandWithDisposition() contain the Revert() logic? Then it would work
consistently across platforms and it looks like browser_ has access to the
location bar via window_->GetLocationBar().

[viettrungluu, 2009-10-24 20:56:29]

http://codereview.chromium.org/330002/diff/1002/1003
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/330002/diff/1002/1003#newcode611
Line 611: // fall through to the default (current tab) case.
On 2009/10/24 20:34:05, Nico wrote:
> I don't like how this knows about the internals of ECWD().

Me neither, in particular the fact that dispositions other than
{BACK,FORE}GROUND_TAB fall through to current tab.

> Is there any harm in always or never calling Revert()? Windows seems to call
it
> always (c/b/views/toolbar_view.cc, ButtonPressed() ), Gtk never
> (c/b/gtk/back_forward_button_gtk.cc , OnClick() ). Should
> ExecuteCommandWithDisposition() contain the Revert() logic? Then it would work
> consistently across platforms and it looks like browser_ has access to the
> location bar via window_->GetLocationBar().

Both Windows and Linux have slightly broken behaviour (IMO). Test:
- navigate;
- replace text in omnibox with "foo" (say); and
- compare what happens to the text on clicking the back button,
command/control-clicking it, command/control+shift-clicking it, and
shift-clicking it.

(1) Clearly, just clicking it should reset the contents of the omnibox. I
suppose this happens even without the Revert() since navigations always reset
the contents. Since shift-clicking is (currently) equivalent to just clicking,
it should do the same thing.
(2) IMO, command/control-clicking it should not affect the contents of the
(current tab's) omnibox.
(3) Command/control+shift-clicking is not as obvious, but since we allow tabs to
be switched without resetting the contents of the omnibox, I don't think it
should reset the content of the current tab's omnibox.

As you said, Windows always calls it, which breaks (2) and (3). For reasons
unknown to me (I haven't looked at the code), Linux has a broken (2) (but not
(3)).

Theoretically, not calling Revert() should do the right thing ... let me try
that.

[viettrungluu, 2009-10-24 21:08:20]

Simplified (never call Revert()). Please re-review.

[Nico, 2009-10-24 21:15:04]

LG.