linux: Add support for undo in the omnibox.

linux: Add support for undo in the omnibox.

GtkTextView does not support undo. So borrow the relevant code from
GtkSourceView. Fixes issue #18210.

BUG=18210
TEST=none

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

[Evan Martin, 2010-09-17 19:17:24]

I'm not sure we can use LGPL code in Chrome like this.

Maybe it'd be better if we built the extra bits as a separate .a via a directory
in third_party.

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

http://codereview.chromium.org/3387008/diff/1/2#newcode6
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:6: #include
"chrome/browser/autocomplete/undo_view.h"
this should go with the other headers below

see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

[sadrul, 2010-09-21 21:04:26]

> I'm not sure we can use LGPL code in Chrome like this.
> 
> Maybe it'd be better if we built the extra bits as a separate .a via a
directory
> in third_party.

I moved the borrowed code into third_part/undoview/

> http://codereview.chromium.org/3387008/diff/1/2#newcode6
> chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:6: #include
> "chrome/browser/autocomplete/undo_view.h"
> this should go with the other headers below

Done.

[sadrul, 2010-09-21 21:08:11]

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

http://codereview.chromium.org/3387008/diff/1/2#newcode6
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:6: #include
"chrome/browser/autocomplete/undo_view.h"
On 2010/09/17 19:17:24, Evan Martin wrote:
> this should go with the other headers below
> 
> see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

[Evan Martin, 2010-09-21 21:14:26]

Can you make the readme mention more about why this module exists?  e.g. say
"copied out of their source tree because we only want this one module" or
something

http://codereview.chromium.org/3387008/diff/5001/6001
File build/all.gyp (right):

http://codereview.chromium.org/3387008/diff/5001/6001#newcode92
build/all.gyp:92: '../third_party/undoview/undoview.gyp:*',
why is this needed?

http://codereview.chromium.org/3387008/diff/5001/6002
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/3387008/diff/5001/6002#newcode45
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:45: #include
"third_party/undoview/undo_view.h"
I'd put this underneath the net/base include, L35

http://codereview.chromium.org/3387008/diff/5001/6003
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3387008/diff/5001/6003#newcode40
chrome/chrome_browser.gypi:40: '../third_party/undoview/undoview.gyp:undoview',
shouldn't this be conditional on os=linux?

http://codereview.chromium.org/3387008/diff/5001/6009
File third_party/undoview/undo_view.h (right):

http://codereview.chromium.org/3387008/diff/5001/6009#newcode1
third_party/undoview/undo_view.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
Is this really code that you wrote?

http://codereview.chromium.org/3387008/diff/5001/6011
File third_party/undoview/undoview.gyp (right):

http://codereview.chromium.org/3387008/diff/5001/6011#newcode30
third_party/undoview/undoview.gyp:30: '<!@(<(pkg-config) --cflags gtk+-2.0)',
We already have a GTK dep elsewhere, you can depend on it...

build/linux/system.gyp

[tfarina, 2010-09-21 21:30:31]

http://codereview.chromium.org/3387008/diff/17001/18005
File third_party/undoview/README.chromium (right):

http://codereview.chromium.org/3387008/diff/17001/18005#newcode1
third_party/undoview/README.chromium:1: Name: undo_view
nit: undoview?

http://codereview.chromium.org/3387008/diff/17001/18005#newcode4
third_party/undoview/README.chromium:4: This cotains GtkUndoView, which is
GtkTextView with undo/redo support.
nit: contains

[sadrul, 2010-09-21 21:45:55]

http://codereview.chromium.org/3387008/diff/5001/6001
File build/all.gyp (right):

http://codereview.chromium.org/3387008/diff/5001/6001#newcode92
build/all.gyp:92: '../third_party/undoview/undoview.gyp:*',
On 2010/09/21 21:14:26, Evan Martin wrote:
> why is this needed?

I am not entirely sure whether listing this here is necessary or not. Most of
the .gyp stuff is still pretty black-magic to me. Should this line be removed?

http://codereview.chromium.org/3387008/diff/5001/6002
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/3387008/diff/5001/6002#newcode45
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:45: #include
"third_party/undoview/undo_view.h"
On 2010/09/21 21:14:26, Evan Martin wrote:
> I'd put this underneath the net/base include, L35

Done.

http://codereview.chromium.org/3387008/diff/5001/6003
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3387008/diff/5001/6003#newcode40
chrome/chrome_browser.gypi:40: '../third_party/undoview/undoview.gyp:undoview',
On 2010/09/21 21:14:26, Evan Martin wrote:
> shouldn't this be conditional on os=linux?

Moved this under 'OS="linux' condition.

http://codereview.chromium.org/3387008/diff/5001/6009
File third_party/undoview/undo_view.h (right):

http://codereview.chromium.org/3387008/diff/5001/6009#newcode1
third_party/undoview/undo_view.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
On 2010/09/21 21:14:26, Evan Martin wrote:
> Is this really code that you wrote?

The boilerplate code in the header, and some portions in the corresponding .c
file is generated by one of the online gobject generator code. The generated
code  does not include any license comments. I assumed I can decide the license
it goes under.

http://codereview.chromium.org/3387008/diff/5001/6011
File third_party/undoview/undoview.gyp (right):

http://codereview.chromium.org/3387008/diff/5001/6011#newcode30
third_party/undoview/undoview.gyp:30: '<!@(<(pkg-config) --cflags gtk+-2.0)',
On 2010/09/21 21:14:26, Evan Martin wrote:
> We already have a GTK dep elsewhere, you can depend on it...
> 
> build/linux/system.gyp

Done.

[Evan Martin, 2010-09-21 21:57:46]

LGTM, feel free to submit after fixing these minor comments

http://codereview.chromium.org/3387008/diff/22001/23001
File build/all.gyp (right):

http://codereview.chromium.org/3387008/diff/22001/23001#newcode92
build/all.gyp:92: '../third_party/undoview/undoview.gyp:*',
Yes, this line should be removed.
(entries here are only needed for top-level binaries)

http://codereview.chromium.org/3387008/diff/22001/23008
File third_party/undoview/undo_view.c (right):

http://codereview.chromium.org/3387008/diff/22001/23008#newcode3
third_party/undoview/undo_view.c:3: // found in the LICENSE file.
Can you note around here somewhere that this was a generated file?  So that
people know not to edit it...

http://codereview.chromium.org/3387008/diff/22001/23009
File third_party/undoview/undo_view.h (right):

http://codereview.chromium.org/3387008/diff/22001/23009#newcode3
third_party/undoview/undo_view.h:3: // found in the LICENSE file.
Can you note around here somewhere that this was a generated file?  So that
people know not to edit it...

http://codereview.chromium.org/3387008/diff/22001/23010
File third_party/undoview/undo_view_marshal.h (right):

http://codereview.chromium.org/3387008/diff/22001/23010#newcode3
third_party/undoview/undo_view_marshal.h:3: // found in the LICENSE file.
Can you note around here somewhere that this was a generated file?  So that
people know not to edit it...

[sadrul, 2010-09-21 22:14:26]

http://codereview.chromium.org/3387008/diff/22001/23001
File build/all.gyp (right):

http://codereview.chromium.org/3387008/diff/22001/23001#newcode92
build/all.gyp:92: '../third_party/undoview/undoview.gyp:*',
On 2010/09/21 21:57:47, Evan Martin wrote:
> Yes, this line should be removed.
> (entries here are only needed for top-level binaries)

Done.

http://codereview.chromium.org/3387008/diff/22001/23008
File third_party/undoview/undo_view.c (right):

http://codereview.chromium.org/3387008/diff/22001/23008#newcode3
third_party/undoview/undo_view.c:3: // found in the LICENSE file.
On 2010/09/21 21:57:47, Evan Martin wrote:
> Can you note around here somewhere that this was a generated file?  So that
> people know not to edit it...

It is actually OK for people to edit undo_view.c or .h. However, I have added
comments to note that the boilerplate was generated, and linked to the tool that
was used to generate the code.

http://codereview.chromium.org/3387008/diff/22001/23010
File third_party/undoview/undo_view_marshal.h (right):

http://codereview.chromium.org/3387008/diff/22001/23010#newcode3
third_party/undoview/undo_view_marshal.h:3: // found in the LICENSE file.
On 2010/09/21 21:57:47, Evan Martin wrote:
> Can you note around here somewhere that this was a generated file?  So that
> people know not to edit it...

Done (I actually removed this file since it's not actually needed)

[sadrul, 2010-09-22 16:44:36]

Also, I was wondering if GtkViewsTextView (in
views/controls/textfield/gtk_views_textview) should inherit this new GtkUndoView
instead of GtkTextView?

[Evan Martin, 2010-09-22 16:55:49]

LGTM++

I'm not sure about the views question, maybe ask someone closer to that code.

[tfarina, 2010-09-23 19:37:22]

On 2010/09/22 16:55:49, Evan Martin wrote:
> LGTM++
> 
> I'm not sure about the views question, maybe ask someone closer to that code.

Evan, sadrul doesn't have commit access. I can commit later tonight for him if
you want.

[rjkroege, 2010-09-23 21:39:06]

On 2010/09/23 19:37:22, tfarina wrote:
> On 2010/09/22 16:55:49, Evan Martin wrote:
> > LGTM++
> > 
> > I'm not sure about the views question, maybe ask someone closer to that
code.
> 
> Evan, sadrul doesn't have commit access. I can commit later tonight for him if
> you want.

I had offered to do it.

[tfarina, 2010-09-24 00:36:19]

On 2010/09/23 21:39:06, rjkroege wrote:
> I had offered to do it.

Are you going to land it? (If not, let me know and I can land it tonight).

[rjkroege, 2010-09-24 01:11:21]

On Thursday, September 23, 2010,  <tfarina@chromium.org> wrote:
> On 2010/09/23 21:39:06, rjkroege wrote:
>
> I had offered to do it.
>
>
> Are you going to land it? (If not, let me know and I can land it tonight).

Please go ahead and land it. Thanks.

>
> http://codereview.chromium.org/3387008/show
>

-- 
Robert Kroeger
rjkroege@google.com

[tfarina, 2010-09-24 01:26:43]

On 2010/09/24 01:11:21, rjkroege wrote:
> Please go ahead and land it. Thanks.

I will be landing it here: http://codereview.chromium.org/3429025/, when the
trybots passes.

[tfarina, 2010-09-24 01:45:16]

sadurl/Evan check deps failed:

http://build.chromium.org/buildbot/try-server/builders/linux/builds/47553/ste...

[sadrul, 2010-09-24 14:27:59]

On 2010/09/24 01:45:16, tfarina wrote:
> sadurl/Evan check deps failed:
> 
>
http://build.chromium.org/buildbot/try-server/builders/linux/builds/47553/ste...

I believe undoview should be added to DEPS to resolve this error? In this
particular case, however, I am not sure what should be added to DEPS, since this
undoview is *not* actually a third party library with its own repository.

I have added an entry in DEPS, however I am unsure if it is correct.

NOTE: The change in patchset 6 in chrome/chrome_browser.gypi against patchset 5
is because I had merged with the updated trunk. I haven't manually made any
changes in there for patchset 6.

[Evan Martin, 2010-09-24 20:01:37]

check_deps is a separate thing, actually.
you can run it:
  ./tools/checkdeps/checkdeps.py

the top of the script has info about how to fix the error it reports.

(your deps change is, as you guessed, not needed/correct)

[sadrul-g, 2010-09-27 13:41:18]

>   ./tools/checkdeps/checkdeps.py
> 
> the top of the script has info about how to fix the error it reports.

I have removed the entry I had added, and added an entry in include_dirs for
third_party/undoview.

[tfarina, 2010-09-27 14:00:28]

On 2010/09/27 13:41:18, sadrul-g wrote:
> >   ./tools/checkdeps/checkdeps.py
> > 
> > the top of the script has info about how to fix the error it reports.
> 
> I have removed the entry I had added, and added an entry in include_dirs for
> third_party/undoview.

Thanks, I have patched this in http://codereview.chromium.org/3429025/show and
sent to the trybots again.

[tfarina, 2010-09-27 14:27:12]

On 2010/09/27 13:41:18, sadrul-g wrote:
> I have removed the entry I had added, and added an entry in include_dirs for
> third_party/undoview.
Thanks again, that fixed the check deps issue. I will land it later tonight.

[tfarina, 2010-09-28 02:43:57]

Sorry for the delay, landed in r60744.