Added restore and add tab to the system menu so that these operations...

Added restore and add tab to the system menu so that these operations
can be done from there.

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

[idanan, 2009-04-07 20:20:40]

No magic :)

[Glen Murphy, 2009-04-07 20:31:57]

The menu item should be disabled if there's nothing to restore. This may require
changing the way the menu is constructed.

http://codereview.chromium.org/62113/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/62113/diff/1/2#newcode575
Line 575: &Restore Tab
Let's make it 'Undo Close Tab' - there's already a 'Restore' in this menu.

[idanan, 2009-04-07 20:36:55]

On 2009/04/07 20:31:57, Glen Murphy wrote:
> The menu item should be disabled if there's nothing to restore. This may
require
> changing the way the menu is constructed.

Actually, while trying to write that code I found out that CTRL-SHIFT-T produces
an new empty window when there is nothing to restore, so I thought the behavior
should be identical.

> http://codereview.chromium.org/62113/diff/1/2
> File chrome/app/generated_resources.grd (right):
> 
> http://codereview.chromium.org/62113/diff/1/2#newcode575
> Line 575: &Restore Tab
> Let's make it 'Undo Close Tab' - there's already a 'Restore' in this menu.

OK. BTW, I noticed the name is a bit misleading because it will undo also a
close window (and restore ALL the tabs present when the window was closed).

[Glen Murphy, 2009-04-07 20:46:08]

> Actually, while trying to write that code I found out that 
> CTRL-SHIFT-T produces an new empty window when there is
> nothing to restore, so I thought the behavior should be
> identical.

Eek! That's a bug. Scott and I can't repro, but if you can, please file. There
should be code somewhere that detects whether there's anything to restore and
stops the command firing if there isn't.

> OK. BTW, I noticed the name is a bit misleading because it will undo also a
> close window (and restore ALL the tabs present when the window was closed).

Mmm, good point. It really is an 'Undo Last Close', though that's a horrible
name. Let's just leave it as "Undo Close Tab" for now, as that matches the menu
in other products, then come back to it.

[Peter Kasting, 2009-04-07 20:49:00]

Drive-by:
* I don't have much of an opinion on the system-menu placement (it seems weird
to me, but whatever), but it seems like these options should be available on the
per-tab context menu ("Add Tab" is already present, but "Undo Close Tab" would
be added alongside it).  I think that's actually what the bug is asking for.

* I agree with Glen that ctrl-shift-t should not "restore" nothing.

[idanan, 2009-04-07 21:00:05]

On 2009/04/07 20:49:00, pkasting wrote:
> Drive-by:
> * I don't have much of an opinion on the system-menu placement (it seems weird
> to me, but whatever), but it seems like these options should be available on
the
> per-tab context menu ("Add Tab" is already present, but "Undo Close Tab" would
> be added alongside it).  I think that's actually what the bug is asking for.

This time I disagree. I really think the Undo close tab has nothing to do with
any
particular tab. I know that in the case of the new tab, Ben said the option is
there
because it inserts the new tab relative to the tab on which the operation is
done.
With the undo tab, it does not work like this.

> * I agree with Glen that ctrl-shift-t should not "restore" nothing.

This I agree too and, rejoice, it does not actually happen. My test was flawed
because I had not cleared my history (just closed an empty browser window), so
the undo exactly undoes what was there last.

[idanan, 2009-04-07 21:00:38]

All done.

[Peter Kasting, 2009-04-07 21:05:36]

On 2009/04/07 21:00:05, idanan wrote:
> This time I disagree. I really think the Undo close tab has nothing to do with
> any
> particular tab.

I agree that it has nothing to do with any particular tab.  Right-clicking on a
tab is ambiguous: you're clicking on a tab within a tabstrip, so do you want
options for the tab or the tabstrip?  I claim both, not just one or the other.

Not only does it make sense (as a tabstrip option, not a tab option), but
practically, triggering the system menu is generally harder as once you have
more than one or two tabs most of the "tabstrip" space is filled with tabs.  If
this option isn't present there it's hard to find and use.

[Glen Murphy, 2009-04-07 21:33:32]

LGTM, with nit.

http://codereview.chromium.org/62113/diff/5/1009
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/62113/diff/5/1009#newcode421
Line 421: ((command != IDC_RESTORE_TAB) ||
I think this should be a four-pixel indent.

Though how about just splitting this into an if statement rather than bundling
it all up. My brain is slow.

[Peter Kasting, 2009-04-07 21:48:02]

BTW, Ben officially OK'd putting Undo Close Tab onto the per-tab menu (I just
talked to him), but feel free to do that in a separate CL since the menu is
separate (unless it makes your life nicer to do it here).

[idanan, 2009-04-08 13:33:27]

BTW, I officially disagree ;) because the context menu are normally actions
applicable to the object directly not its parent. Check the context menu of the
Windows desktop for example versus the one of icons on it. On the desktop you
can 'Arrange Icons' but not on the icon itself. That make sense to me, the same
way you cannot "Undo Tab Close" from a tab because that tab has nothing to do
with the closed tab's closing.

That being said, I am not one to object adding extra functionality that does not
clutter the UI. I know how to do this, so I will add it in a separate CL since
it will touch a different set of files.

[idanan, 2009-04-08 14:15:18]

All done, Glen. Thanks for the speedy review.