Make Linux restore Omnibox contents on tab switch.

Make Linux restore Omnibox contents on tab switch.

Tested as follows:

Create a new window with two tabs.  Type a bunch of 'a' characters into tab A's omnibox and a bunch of 'b's into tab B.  Then,

1. Select tab A.  Left-click in the omnibox and highlight a few characters in the middle.  Left-click in the empty space to the right of the text to unhighlight it, then middle-click to make sure that the previously-highlighted text still gets pasted.

2. Repeat 1, but hit a key to remove the selection instead of clicking the mouse.  The old highlighted text should still be available as the primary selection.

3. Highlight A's text as in 1.  Select tab B and see that the omnibox is updated to B's string and highlighting is removed.  Middle-click and confirm that the previously-highlighted text from A is pasted.

4. Select A and left-click in the middle of its string to position the cursor there.  Click on tab B and then back on A to check that A's cursor position is restored.

5. Highlight text in A.  Select tab B and then highlight text from a different window.  Switch back to A and middle-click.  The text from the different window, rather than A's previously-highlighted text, should be pasted.

BUG=9225

[Evan Martin, 2009-06-29 02:06:33]

dean to review

[Dean McNamee, 2009-06-29 08:46:06]

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

http://codereview.chromium.org/151006/diff/1/2#newcode179
Line 179: GtkTextBuffer* buffer =
gtk_text_view_get_buffer(GTK_TEXT_VIEW(text_view_));
text_buffer_ ?

http://codereview.chromium.org/151006/diff/1/2#newcode194
Line 194: SavePrimarySelection();
Comment about what you're doing here with the save selection.

http://codereview.chromium.org/151006/diff/1/2#newcode224
Line 224: gtk_text_view_get_buffer(GTK_TEXT_VIEW(text_view_));
text_buffer_ ?

http://codereview.chromium.org/151006/diff/1/2#newcode235
Line 235: buffer, gtk_text_buffer_get_selection_bound(buffer), &iter);
gtk_text_buffer_select_range ?

http://codereview.chromium.org/151006/diff/1/2#newcode698
Line 698: GtkTextBuffer* buffer =
gtk_text_view_get_buffer(GTK_TEXT_VIEW(text_view_));
text_buffer_ ?

http://codereview.chromium.org/151006/diff/1/2#newcode702
Line 702: }
no { } for a single line if.

http://codereview.chromium.org/151006/diff/1/2#newcode705
Line 705: gtk_clipboard_get(GDK_SELECTION_PRIMARY), text, strlen(text));
depending when this happens, gtk_clipboard_get can return NULL.  I had seen this
code getting called before the widget was up (although then I guess the above
selection bounds would have failed).

Anyway, there is already code that uses and caches primary_clipboard_.  I would
rather you hoist that code so they can all share the primary_clipboard_ cache.

Basically something like CachePrimaryClipboard() or something, which will return
true if primary_clipboard_ is not-null.

http://codereview.chromium.org/151006/diff/1/3
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):

http://codereview.chromium.org/151006/diff/1/3#newcode42
Line 42: };
Does this need to be in the header?  Can it not be in an anonymous namespace in
the .cc ?

[Dan Erat, 2009-06-30 01:01:50]

To keep things from getting really complicated, I'm no longer restoring the
highlighting when you switch back to a tab.  I think that everything pretty much
works now.  There's still a bit of weirdness with the initial left-click that
focuses the omnibox grabbing the primary selection when it selects all of the
text, but I'm not sure how (or if) this should be resolved -- it can be useful
when you want to copy the URL, but it's annoying when you were clicking there to
paste something.

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

http://codereview.chromium.org/151006/diff/1/2#newcode179
Line 179: GtkTextBuffer* buffer =
gtk_text_view_get_buffer(GTK_TEXT_VIEW(text_view_));
On 2009/06/29 08:46:06, Dean McNamee wrote:
> text_buffer_ ?

Done.

http://codereview.chromium.org/151006/diff/1/2#newcode194
Line 194: SavePrimarySelection();
On 2009/06/29 08:46:06, Dean McNamee wrote:
> Comment about what you're doing here with the save selection.

Done.

http://codereview.chromium.org/151006/diff/1/2#newcode224
Line 224: gtk_text_view_get_buffer(GTK_TEXT_VIEW(text_view_));
On 2009/06/29 08:46:06, Dean McNamee wrote:
> text_buffer_ ?

Done.

http://codereview.chromium.org/151006/diff/1/2#newcode235
Line 235: buffer, gtk_text_buffer_get_selection_bound(buffer), &iter);
On 2009/06/29 08:46:06, Dean McNamee wrote:
> gtk_text_buffer_select_range ?

Ah, nice.  Thanks.

http://codereview.chromium.org/151006/diff/1/2#newcode698
Line 698: GtkTextBuffer* buffer =
gtk_text_view_get_buffer(GTK_TEXT_VIEW(text_view_));
On 2009/06/29 08:46:06, Dean McNamee wrote:
> text_buffer_ ?

Done.

http://codereview.chromium.org/151006/diff/1/2#newcode702
Line 702: }
On 2009/06/29 08:46:06, Dean McNamee wrote:
> no { } for a single line if.

Done.

http://codereview.chromium.org/151006/diff/1/2#newcode705
Line 705: gtk_clipboard_get(GDK_SELECTION_PRIMARY), text, strlen(text));
On 2009/06/29 08:46:06, Dean McNamee wrote:
> depending when this happens, gtk_clipboard_get can return NULL.  I had seen
this
> code getting called before the widget was up (although then I guess the above
> selection bounds would have failed).
> 
> Anyway, there is already code that uses and caches primary_clipboard_.  I
would
> rather you hoist that code so they can all share the primary_clipboard_ cache.
> 
> Basically something like CachePrimaryClipboard() or something, which will
return
> true if primary_clipboard_ is not-null.

Sure, I've added a GetPrimaryClipboard() method for accessing
primary_clipboard_.  Would you rather that primary_clipboard_ be a singleton or
a static var within GetPrimaryClipboard()?

I wasn't sure if there was something special about gtk_widget_get_clipboard()
(since it doesn't make sense to me for an X selection to be associated with a
particular widget, unless you're writing code that connects to multiple displays
or something).  Looks like it does what I want, though.

http://codereview.chromium.org/151006/diff/1/3
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):

http://codereview.chromium.org/151006/diff/1/3#newcode42
Line 42: };
On 2009/06/29 08:46:06, Dean McNamee wrote:
> Does this need to be in the header?  Can it not be in an anonymous namespace
in
> the .cc ?

I was trying to match the Windows version of this file, but I've renamed it to
ViewState and moved it to the .cc file now.  I'm using a CharRange struct within
it now, so I had to make CharRange be a public rather than private child class
of AutocompleteEditViewGtk, unfortunately.

[Evan Stade, 2009-06-30 02:20:28]

http://codereview.chromium.org/151006/diff/9/1004
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/151006/diff/9/1004#newcode502
Line 502: SavePrimarySelection();
so what happens if the user breaks the selection via the keyboard (e.g., hits
right arrow)? Won't the clipboard be cleared in that case as well?

http://codereview.chromium.org/151006/diff/9/1004#newcode610
Line 610: primary_clipboard_ = gtk_widget_get_clipboard(text_view_,
You don't need to cache this in |primary_clipboard_|, because
gtk_widget_get_clipboard() only creates the clipboard if it doesn't already
exist.

[Dan Erat, 2009-06-30 04:44:50]

http://codereview.chromium.org/151006/diff/9/1004
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/151006/diff/9/1004#newcode502
Line 502: SavePrimarySelection();
On 2009/06/30 02:20:28, Evan Stade wrote:
> so what happens if the user breaks the selection via the keyboard (e.g., hits
> right arrow)? Won't the clipboard be cleared in that case as well?

Ah, good point.  I'll try to find a solution that covers that as well.

http://codereview.chromium.org/151006/diff/9/1004#newcode610
Line 610: primary_clipboard_ = gtk_widget_get_clipboard(text_view_,
On 2009/06/30 02:20:28, Evan Stade wrote:
> You don't need to cache this in |primary_clipboard_|, because
> gtk_widget_get_clipboard() only creates the clipboard if it doesn't already
> exist.

Makes sense.  I've replaced the two calls to GetPrimaryClipboard() with direct
calls to gtk_widget_get_clipboard().  (Not uploaded to the review server yet.)

[Dean McNamee, 2009-06-30 08:42:31]

LG.  This is what I had ended up doing in my patch (not restoring the
selection).

I would additionally be more paranoid about getting a NULL clipboard, and I
would NULL check it everywhere we use the clipboard.  I can imagine we'll see
something in the wild where we don't get a clipboard at all for whatever reason.

Thanks

http://codereview.chromium.org/151006/diff/9/1004
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/151006/diff/9/1004#newcode613
Line 613: DCHECK(primary_clipboard_);
Just so you know, gtk_widget_get_clipboard will return NULL if the widget isn't
connected to the tree yet (not attached to a toplevel / display).  It depends
how early your code is called, but this happens, for example, in some of the tab
save / restore methods.

http://codereview.chromium.org/151006/diff/9/1004#newcode721
Line 721: gtk_clipboard_set_text(clipboard, text, strlen(text));
cache the strlen?  It would be even nicer to avoid it all together.

[Dean McNamee, 2009-06-30 08:47:38]

Btw, about gtk_widget_get_clipboard, the reason I cached it is just that GTK
does a bit more work to pull out the clipboard from the clipboard list, etc. 
It's a premature optimization, but the pointer doesn't change, and I already had
it, so I figured why not save the extra work.

On 2009/06/30 08:42:31, Dean McNamee wrote:
> LG.  This is what I had ended up doing in my patch (not restoring the
> selection).
> 
> I would additionally be more paranoid about getting a NULL clipboard, and I
> would NULL check it everywhere we use the clipboard.  I can imagine we'll see
> something in the wild where we don't get a clipboard at all for whatever
reason.
> 
> Thanks
> 
> http://codereview.chromium.org/151006/diff/9/1004
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> 
> http://codereview.chromium.org/151006/diff/9/1004#newcode613
> Line 613: DCHECK(primary_clipboard_);
> Just so you know, gtk_widget_get_clipboard will return NULL if the widget
isn't
> connected to the tree yet (not attached to a toplevel / display).  It depends
> how early your code is called, but this happens, for example, in some of the
tab
> save / restore methods.
> 
> http://codereview.chromium.org/151006/diff/9/1004#newcode721
> Line 721: gtk_clipboard_set_text(clipboard, text, strlen(text));
> cache the strlen?  It would be even nicer to avoid it all together.

[Evan Stade, 2009-06-30 18:17:38]

On 2009/06/30 08:47:38, Dean McNamee wrote:
> Btw, about gtk_widget_get_clipboard, the reason I cached it is just that GTK
> does a bit more work to pull out the clipboard from the clipboard list, etc. 
> It's a premature optimization, but the pointer doesn't change, and I already
had
> it, so I figured why not save the extra work.

true, it's not a big deal one way or the other, I just thought it created more
extra code than it was worth. I am fine with either approach.

[Dan Erat, 2009-06-30 21:24:16]

Okay, do you guys mind taking one more look?  I'm using changes to the text
marks now as a trigger for saving the selection, which appears to handle the
case where the keyboard is used to clear the selection.

Let me know if you can think of a simpler way of doing all this -- copying the
selection every time it changes doesn't seem ideal, but I don't see any obvious
signal I can use to grab the selection before GTK wipes it out.

http://codereview.chromium.org/151006/diff/9/1004
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/151006/diff/9/1004#newcode613
Line 613: DCHECK(primary_clipboard_);
On 2009/06/30 08:42:31, Dean McNamee wrote:
> Just so you know, gtk_widget_get_clipboard will return NULL if the widget
isn't
> connected to the tree yet (not attached to a toplevel / display).  It depends
> how early your code is called, but this happens, for example, in some of the
tab
> save / restore methods.

Done.

http://codereview.chromium.org/151006/diff/9/1004#newcode721
Line 721: gtk_clipboard_set_text(clipboard, text, strlen(text));
On 2009/06/30 08:42:31, Dean McNamee wrote:
> cache the strlen?  It would be even nicer to avoid it all together.

Done.

[Dean McNamee, 2009-06-30 21:35:06]

I will review this tomorrow...

http://codereview.chromium.org/151006/diff/1012/13
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/151006/diff/1012/13#newcode638
Line 638: selected_text_ = std::string(text, text_len);
this copies the text.  If you care enough to avoid copying it, you could use a
StringPiece.  Up to you.

http://codereview.chromium.org/151006/diff/1012/13#newcode751
Line 751: clipboard, selected_text.c_str(), selected_text.size());
.c_str should technically be .data()

[Dan Erat, 2009-06-30 22:10:23]

http://codereview.chromium.org/151006/diff/1012/13
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/151006/diff/1012/13#newcode638
Line 638: selected_text_ = std::string(text, text_len);
On 2009/06/30 21:35:06, Dean McNamee wrote:
> this copies the text.  If you care enough to avoid copying it, you could use a
> StringPiece.  Up to you.

The GTK docs don't say anything about the lifetime of the string returned by
gtk_text_iter_get_text().  I had assumed that it's just until the buffer gets
updated*, but from a quick look at gtktextiter.c and gtktextbtree.c, it looks
like it's allocating new memory.  I'll free it after constructing the string
instead (let me know if you'd rather that I just hold onto the returned pointer
and manage it manually).

* but from reading
http://library.gnome.org/devel/gtk/2.15/gtk-question-index.html, I see that all
non-const strings should be freed

http://codereview.chromium.org/151006/diff/1012/13#newcode751
Line 751: clipboard, selected_text.c_str(), selected_text.size());
On 2009/06/30 21:35:06, Dean McNamee wrote:
> .c_str should technically be .data()

Done.

[Evan Stade, 2009-07-01 19:49:30]

lg to me

[Evan Stade, 2009-07-01 19:51:31]

does this also fix <crbug.com/15464>?

[Dan Erat, 2009-07-01 22:00:33]

On 2009/07/01 19:51:31, Evan Stade wrote:
> does this also fix <crbug.com/15464>?

No, I doesn't (I've added a comment to that bug).