[Mac] Support undo in the omnibox.

[Mac] Support undo in the omnibox.

BUG=http://crbug.com/18084
TEST=You should be able to undo and redo typing and pasting in the omnibox, unless autocomplete has kicked in or you selected/highlighted an entry in the popup.

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

[rohitrao (ping after 24h), 2009-10-26 22:08:15]

Scott, do you want to try patching this in and seeing how it feels?  This is
essentially just creating a custom undo manager for the omnibox, giving us a way
to clear the undo stack when needed.

I still need to write tests to make sure we're clearing the undo stack
correctly.

[rohitrao (ping after 24h), 2009-11-09 22:45:36]

Moved the clearUndoChain functionality into the field, rather than in the edit
view.

Couple comments/concerns:
1) I'm not 100% sure we really need a separate NSUndoManager instance here, or
if we can get away with using the stock manager.
2) I don't know if I've hit all the cases where we should be clearing the undo
chain, but doing it in these two locations seems to make undo behave as I want
it to.

#1 doesn't concern me because it's not wrong, just potentially mildly wasteful. 
#2 doesn't concern me too much because this is still better than what we have
now and we can always iterate.

[Scott Hess - ex-Googler, 2009-11-09 23:01:53]

http://codereview.chromium.org/246009/diff/5001/6001
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/246009/diff/5001/6001#newcode474
Line 474: [field_ clearUndoChain];
Did you consider having LocationBarViewMac::OnChanged() call -clearUndoChain?  I
suggest it because that might cover additional cases which aren't part of the
autocomplete logic.

http://codereview.chromium.org/246009/diff/5001/6002
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/246009/diff/5001/6002#newcode20
Line 20: undoManager_.reset([[NSUndoManager alloc] init]);
In the interests of NTP performance, maybe create this lazily?

http://codereview.chromium.org/246009/diff/5001/6002#newcode36
Line 36: }
spare line between methods, here.  And maybe put this next to -clearUndoChain,
unless there's an interesting ordering goal.

http://codereview.chromium.org/246009/diff/5001/6004
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/246009/diff/5001/6004#newcode350
Line 350: [autocompleteTextFieldEditor_ setAllowsUndo:YES];
This is sort of a hard-to-find occurance of this.  Would prefer to push it
either into AutocompleteTextFieldEditor -init* method of some sort, or maybe
-allowsUndo can be overridden.

[rohitrao (ping after 24h), 2009-11-10 00:22:41]

Most comments addressed.  I'm worried that the calls to clearUndoChain are
destined to be scattered about.  The new addition in RevertAll() covers cases
where we switch tabs, press escape to dismiss the popup, or navigate to a new
page.  The existing calls covered keying up/down through the popup and inline
autocomplete.

http://codereview.chromium.org/246009/diff/5001/6001
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/246009/diff/5001/6001#newcode474
Line 474: [field_ clearUndoChain];
On 2009/11/09 23:01:53, shess wrote:
> Did you consider having LocationBarViewMac::OnChanged() call -clearUndoChain? 
I
> suggest it because that might cover additional cases which aren't part of the
> autocomplete logic.

Can't put it in OnChanged() because that is called every time a key is pressed.

http://codereview.chromium.org/246009/diff/5001/6002
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/246009/diff/5001/6002#newcode20
Line 20: undoManager_.reset([[NSUndoManager alloc] init]);
On 2009/11/09 23:01:53, shess wrote:
> In the interests of NTP performance, maybe create this lazily?

Done.

http://codereview.chromium.org/246009/diff/5001/6002#newcode36
Line 36: }
On 2009/11/09 23:01:53, shess wrote:
> spare line between methods, here.  And maybe put this next to -clearUndoChain,
> unless there's an interesting ordering goal.

Done.

http://codereview.chromium.org/246009/diff/5001/6004
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/246009/diff/5001/6004#newcode350
Line 350: [autocompleteTextFieldEditor_ setAllowsUndo:YES];
On 2009/11/09 23:01:53, shess wrote:
> This is sort of a hard-to-find occurance of this.  Would prefer to push it
> either into AutocompleteTextFieldEditor -init* method of some sort, or maybe
> -allowsUndo can be overridden.

Moved to -[AutocompleteTextFieldEditor init].

http://codereview.chromium.org/246009/diff/8001/8004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/246009/diff/8001/8004#newcode170
Line 170: LOG(ERROR) << "clearUndoChain";
This is gone locally, but I didn't want to reupload and retry.

[Scott Hess - ex-Googler, 2009-11-11 23:25:08]

lgtm.

Could it really be this easy?