Update internal state on undo to avoid a crash.

Update internal state on undo to avoid a crash.

Update actions_in_current_group when removing an action so that it doesn't crash
afterwards.

BUG=61607, 62609
TEST=manual (see bug)

Do not set text if it didn't change, because it registers a delete/insert undo-able action-couple.

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

[Evan Martin, 2010-11-08 17:56:50]

Why doesn't this affect the upstream undoview?

Can we send this patch upstream?

[James Su, 2010-11-08 18:27:11]

http://codereview.chromium.org/4683002/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/4683002/diff/1/2#newcode1687
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1687: }
First, you need to update your local working code. The GetTextFromBuffer()
method has been removed in the latest code. Use GetText() instead.
Second, having this check here is good. But I'm wondering if undo_manager can
handle this situation correctly if we don't do this check here?

[sadrul, 2010-11-08 19:57:37]

http://codereview.chromium.org/4683002/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/4683002/diff/1/2#newcode1687
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1687: }
On 2010/11/08 18:27:11, James Su wrote:
> First, you need to update your local working code. The GetTextFromBuffer()
> method has been removed in the latest code. Use GetText() instead.

Done. Thanks.

> Second, having this check here is good. But I'm wondering if undo_manager can
> handle this situation correctly if we don't do this check here?

The problem is, the GtkTextBuffer code deletes the existing text first, and then
sets the text. So that registers a 'delete' action, and then an 'insert' action.
By the time these actions reach the undo-manager, it is not possible to
determine if it's caused by this (i.e. setting the same text), or by an actual
series of 'delete' and 'insert' action.

[sadrul, 2010-11-08 20:01:13]

On 2010/11/08 17:56:50, Evan Martin wrote:
> Why doesn't this affect the upstream undoview?
> 
> Can we send this patch upstream?

It may be the case that the way chromium uses the text-buffer triggers this
particular bug. I will try to reproduce the problem in the upstream project and
submit the patch for review. While I attempt doing that, should I add a TODO
here so that the P1 bug can be resolved?

[James Su, 2010-11-08 23:01:49]

On 2010/11/08 19:57:37, sadrul wrote:
> http://codereview.chromium.org/4683002/diff/1/2
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> 
> http://codereview.chromium.org/4683002/diff/1/2#newcode1687
> chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1687: }
> On 2010/11/08 18:27:11, James Su wrote:
> > First, you need to update your local working code. The GetTextFromBuffer()
> > method has been removed in the latest code. Use GetText() instead.
> 
> Done. Thanks.
> 
> > Second, having this check here is good. But I'm wondering if undo_manager
can
> > handle this situation correctly if we don't do this check here?
> 
> The problem is, the GtkTextBuffer code deletes the existing text first, and
then
> sets the text. So that registers a 'delete' action, and then an 'insert'
action.
> By the time these actions reach the undo-manager, it is not possible to
> determine if it's caused by this (i.e. setting the same text), or by an actual
> series of 'delete' and 'insert' action.
For this case, although it's not good to have extra delete and insert actions
recorded, at least the undo manager should not crash. Please help check it and
if it crashes, then we may fix it or report it upstream.

[sadrul, 2010-11-09 14:58:12]

[snip]
> > The problem is, the GtkTextBuffer code deletes the existing text first, and
> then
> > sets the text. So that registers a 'delete' action, and then an 'insert'
> action.
> > By the time these actions reach the undo-manager, it is not possible to
> > determine if it's caused by this (i.e. setting the same text), or by an
actual
> > series of 'delete' and 'insert' action.
> For this case, although it's not good to have extra delete and insert actions
> recorded, at least the undo manager should not crash. Please help check it and
> if it crashes, then we may fix it or report it upstream.

This change does not affect the crash (neither causes nor fixes it). It's mostly
to make the undo behaviour somewhat predictable.

[James Su, 2010-11-09 16:59:21]

On 2010/11/09 14:58:12, sadrul wrote:
> [snip]
> > > The problem is, the GtkTextBuffer code deletes the existing text first,
and
> > then
> > > sets the text. So that registers a 'delete' action, and then an 'insert'
> > action.
> > > By the time these actions reach the undo-manager, it is not possible to
> > > determine if it's caused by this (i.e. setting the same text), or by an
> actual
> > > series of 'delete' and 'insert' action.
> > For this case, although it's not good to have extra delete and insert
actions
> > recorded, at least the undo manager should not crash. Please help check it
and
> > if it crashes, then we may fix it or report it upstream.
> 
> This change does not affect the crash (neither causes nor fixes it). It's
mostly
> to make the undo behaviour somewhat predictable.
Thanks. Then this CL looks good to me.

[sadrul, 2010-11-10 00:40:25]

[snip]
> Thanks. Then this CL looks good to me.

The trybots are happy. Evan, LGTY?

[evanm, 2010-11-10 00:48:58]

LGTM

[sadrul, 2010-11-10 01:00:23]

On 2010/11/10 00:48:58, evanm wrote:
> LGTM

Thanks. Committed and pushed!