[Linux] Improve preedit string and Instant suggestion support in omnibox.

[Linux] Improve preedit string and Instant suggestion support in omnibox.

This CL contains following major changes:
1. Uses GtkLabel instead of GtkTextView for displaying instant
suggestion.
2. Attaches the instant view to a child anchor in the text view.
3. Treats preedit string as part of user input, so that autocomplete match can take effect with uncommitted preedit string. 

BUG=27547
TEST=Instant suggestion should work as normal. And preedit string should trigger autocomplete match and work with Instant suggestion.

[Evan Stade, 2010-10-28 01:19:08]

I'm applying this to test it out, but this seems a lot more complicated, which
is what I was hoping to avoid. Is it not plausible to keep the current code but
update the position of the instant view when the preedit string changes?

OK now I've tested it, and it doesn't get the vertical position correct for me
with default fonts and settings on ubuntu. The "rise" hack seems to be off by a
pixel. Maybe sometimes it will be correct, but in my case it is off by one.

[James Su, 2010-10-28 01:37:09]

On 2010/10/28 01:19:08, Evan Stade wrote:
> I'm applying this to test it out, but this seems a lot more complicated, which
> is what I was hoping to avoid. Is it not plausible to keep the current code
but
> update the position of the instant view when the preedit string changes?
The only way to get the location information of the preedit string I can think
about is to insert a special character after the preedit string and query the
position of that character. But IMHO this approach is even more hacky than this
anchor one.

Besides the preedit string issue, this anchor solution is probably also easier
for RTL layout.

> 
> OK now I've tested it, and it doesn't get the vertical position correct for me
> with default fonts and settings on ubuntu. The "rise" hack seems to be off by
a
> pixel. Maybe sometimes it will be correct, but in my case it is off by one.

Can you send a screenshot to me?

[James Su, 2010-10-28 03:19:23]

The alignment issue should have been fixed. Please try again.

[James Su, 2010-10-28 03:57:17]

I'll try to fix the preedit string issue based on the original solution. If that
works, then I'm ok to keep the original solution.

[James Su, 2010-10-28 17:47:49]

Turns out that these two approaches both have problems supporting preedit
string.
For the original solution, it's not possible to retrieve the end location of the
preedit string, unless there is more content after it.
For the the anchor solution, GtkTextView has a bug that can not align the
preedit string and the instant widget correctly if there is no other content
before or after the preedit string. Although I managed to work around this bug
by inserting a ZWSP just before the instant anchor, it's really hacky.

[Evan Stade, 2010-10-28 21:44:51]

ok. So do you think the anchor approach is the best way forward? How awful is
the alignment bug? How awful is the workaround?

[James Su, 2010-10-28 22:57:01]

Patch Set 4 integrates the fix for issue 27547. You may try it with an input
method to see how awful is the alignment bug.
Patch Set 5 works around the bug.

I believe that a similar trick (using a mark and a ZWSP character) can also be
used with your original solution to solve the preedit string problem. But then
both solutions will be equally tricky.

On 2010/10/28 21:44:51, Evan Stade wrote:
> ok. So do you think the anchor approach is the best way forward? How awful is
> the alignment bug? How awful is the workaround?

[Evan Stade, 2010-10-29 00:08:20]

sorry, I meant how bad is the preedit string alignment thing, not the original
alignment bug, which I assumed you had fixed.

[James Su, 2010-10-29 00:29:28]

On 2010/10/29 00:08:20, Evan Stade wrote:
> sorry, I meant how bad is the preedit string alignment thing, not the original
> alignment bug, which I assumed you had fixed.

Yes I know. Without the workaround introduced in Patch Set 5, the GtkTextView
will render the preedit string about 4 pixels below the instant text, if there
is no other content before or after the preedit string.

This CL now includes all fixes for issue 27547 and the workaround of
GtkTextView's bug.

[Evan Stade, 2010-10-29 01:43:33]

seems to work great. Thanks for your patience. LGTM

http://codereview.chromium.org/4202005/diff/17002/22001
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/4202005/diff/17002/22001#newcode568
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:568: // the text.
delete this line?

http://codereview.chromium.org/4202005/diff/17002/22001#newcode572
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:572: #if
GTK_CHECK_VERSION(2,20,0)
nit: spaces after commas (many places in this file)

also, can you minimize code duplication like this:

bool no_inline_autocomplete = std::max(sel.cp_max, sel.cp_min) <
GetTextLength();
#if bla
no_inline_autocomplete = no_inline_autocomplete || preedit_.size();
#endif

I think it makes it easier to read.

http://codereview.chromium.org/4202005/diff/17002/22001#newcode1528
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1528: #if
GTK_CHECK_VERSION(2,20,0)
ditto

[James Su, 2010-10-29 04:51:55]

Thanks for your review. But I just found that undo doesn't work correctly with
this CL. I'll look into it tomorrow.

On 2010/10/29 01:43:33, Evan Stade wrote:
> seems to work great. Thanks for your patience. LGTM
> 
> http://codereview.chromium.org/4202005/diff/17002/22001
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> 
> http://codereview.chromium.org/4202005/diff/17002/22001#newcode568
> chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:568: // the text.
> delete this line?
> 
> http://codereview.chromium.org/4202005/diff/17002/22001#newcode572
> chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:572: #if
> GTK_CHECK_VERSION(2,20,0)
> nit: spaces after commas (many places in this file)
> 
> also, can you minimize code duplication like this:
> 
> bool no_inline_autocomplete = std::max(sel.cp_max, sel.cp_min) <
> GetTextLength();
> #if bla
> no_inline_autocomplete = no_inline_autocomplete || preedit_.size();
> #endif
> 
> I think it makes it easier to read.
> 
> http://codereview.chromium.org/4202005/diff/17002/22001#newcode1528
> chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1528: #if
> GTK_CHECK_VERSION(2,20,0)
> ditto

[James Su, 2010-10-29 05:15:24]

CL has been updated to fix the undo issue. Well, it doesn't really fix all
issues of undo, which doesn't work well even without this CL. For example, you
can input something that triggers instant suggestion, then undo the input, but
the instant suggestion is still there. Anyway, that can be fixed in separated
CL.

http://codereview.chromium.org/4202005/diff/17002/22001
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/4202005/diff/17002/22001#newcode568
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:568: // the text.
On 2010/10/29 01:43:34, Evan Stade wrote:
> delete this line?

Done.

http://codereview.chromium.org/4202005/diff/17002/22001#newcode572
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:572: #if
GTK_CHECK_VERSION(2,20,0)
On 2010/10/29 01:43:34, Evan Stade wrote:
> nit: spaces after commas (many places in this file)
> 
> also, can you minimize code duplication like this:
> 
> bool no_inline_autocomplete = std::max(sel.cp_max, sel.cp_min) <
> GetTextLength();
> #if bla
> no_inline_autocomplete = no_inline_autocomplete || preedit_.size();
> #endif
> 
> I think it makes it easier to read.
> 

Done.

http://codereview.chromium.org/4202005/diff/17002/22001#newcode1528
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1528: #if
GTK_CHECK_VERSION(2,20,0)
On 2010/10/29 01:43:34, Evan Stade wrote:
> ditto

Done.

[Daniel Erat, 2010-10-29 16:39:58]

(removing myself as a reviewer)

[Evan Stade, 2010-10-29 19:42:27]

yea the undo seems crazy broken, but that's true even without your patch.

I thought this would work for RTL but it doesn't appear to(?). At least I can't
get it to show an instant suggestion for Hebrew Chrome.

[James Su, 2010-10-29 20:11:54]

On 2010/10/29 19:42:27, Evan Stade wrote:
> yea the undo seems crazy broken, but that's true even without your patch.
> 
> I thought this would work for RTL but it doesn't appear to(?). At least I
can't
> get it to show an instant suggestion for Hebrew Chrome.

I'm also wondering how to trigger instant suggestion for RTL languages.

[Evan Stade, 2010-10-29 20:20:22]

On 2010/10/29 20:11:54, James Su wrote:
> On 2010/10/29 19:42:27, Evan Stade wrote:
> > yea the undo seems crazy broken, but that's true even without your patch.
> > 
> > I thought this would work for RTL but it doesn't appear to(?). At least I
> can't
> > get it to show an instant suggestion for Hebrew Chrome.
> 
> I'm also wondering how to trigger instant suggestion for RTL languages.

oh, is the problem that the InstantController isn't setting any suggestion? If
that's the case, I wouldn't worry about it for now (it's probably the same on
Windows).

In general though, this seems like quite the rabbit hole of hacks. How  would it
look if we just didn't show the instant preview at all when the preedit string
is non-empty?

[James Su, 2010-10-29 20:26:37]

On 2010/10/29 20:20:22, Evan Stade wrote:
> On 2010/10/29 20:11:54, James Su wrote:
> > On 2010/10/29 19:42:27, Evan Stade wrote:
> > > yea the undo seems crazy broken, but that's true even without your patch.
> > > 
> > > I thought this would work for RTL but it doesn't appear to(?). At least I
> > can't
> > > get it to show an instant suggestion for Hebrew Chrome.
> > 
> > I'm also wondering how to trigger instant suggestion for RTL languages.
> 
> oh, is the problem that the InstantController isn't setting any suggestion? If
I think so.

> that's the case, I wouldn't worry about it for now (it's probably the same on
> Windows).
> 
> In general though, this seems like quite the rabbit hole of hacks. How  would
it
> look if we just didn't show the instant preview at all when the preedit string
> is non-empty?

I don't know. But then the behavior would be different than Google Instant.

[Evan Stade, 2010-10-29 20:52:21]

fair enough. Would it be at all possible to write unit tests? I'm just worried
about adding such intricate behavior to a class that's already nearing 2000
lines.

[James Su, 2010-10-29 21:20:58]

On 2010/10/29 20:52:21, Evan Stade wrote:
> fair enough. Would it be at all possible to write unit tests? I'm just worried
> about adding such intricate behavior to a class that's already nearing 2000
> lines.

I have been thinking about it for quite a long time. In order to test behaviors
related to input method (preedit string), we need to implement a mock
gtkimmodule and load it in the test. But it would be very complicated. For
example, I'm still not sure how to deploy a gtkimmodule and ask gtk to load it
at runtime. Anyway I'll look into it when I have time, as it's quite useful for
testing not only omnibox but also browser/webkit.
Testing undo/redo without involving input method and instant would be quite
straightforward. We can just add tests in autocomplete_edit_view_browsertest.cc
For instant related features, the key issue is to have a mock instant controller
to provide testing data for us.

[Evan Stade, 2010-10-29 21:34:46]

I would prefer to see unit tests over browser tests if possible (although both
is even better). For now, I'd be happy if you could add some simple tests for
the new functionality you are adding; for example, testing that deleting or
setting or getting text doesn't add it past the instant mark.

[James Su, 2010-10-29 21:48:20]

On 2010/10/29 21:34:46, Evan Stade wrote:
> I would prefer to see unit tests over browser tests if possible (although both
> is even better). For now, I'd be happy if you could add some simple tests for
> the new functionality you are adding; for example, testing that deleting or
> setting or getting text doesn't add it past the instant mark.
As AutocompleteEditViewGtk depends on several external objects, such as
AutocompleteEditController, AutocompleteEditModel, ToolbarModel, etc. Mocking
all of these classes seems not feasible at all. Looks like the most
straightforward way is to write a browsertest for it. What do you think?

[Evan Stade, 2010-10-29 21:54:52]

I guess that's true. An in-process browser test wouldn't be that bad. I was
thinking of ui tests.

[James Su, 2010-10-29 21:56:51]

On 2010/10/29 21:54:52, Evan Stade wrote:
> I guess that's true. An in-process browser test wouldn't be that bad. I was
> thinking of ui tests.

Thanks. I'll implement some simple tests asap. Stay tuned.

[James Su, 2010-10-30 00:00:09]

I added two simple tests in autocomplete_edit_view_browsertest.cc. Actually many
cases are already covered by existing tests.
The new UndoRedoLinux test triggers an assert in undo_manager.cc. I still don't
know why.

[Evan Stade, 2010-11-02 02:20:26]

thanks. LGTM

http://codereview.chromium.org/4202005/diff/8002/45001
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/4202005/diff/8002/45001#newcode750
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:750: // The
cursor should at the end.
missing a verb

http://codereview.chromium.org/4202005/diff/8002/45001#newcode774
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:774: // Undo
delete every thing.
nit: everything

[James Su, 2010-11-02 18:15:10]

I filed a separated issue for the assertion failure: http://crbug.com/61607

I'll commit this CL and address that issue later, as it won't affect the testing
result.

http://codereview.chromium.org/4202005/diff/8002/45001
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/4202005/diff/8002/45001#newcode750
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:750: // The
cursor should at the end.
On 2010/11/02 02:20:26, Evan Stade wrote:
> missing a verb

Done.

http://codereview.chromium.org/4202005/diff/8002/45001#newcode774
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:774: // Undo
delete every thing.
On 2010/11/02 02:20:26, Evan Stade wrote:
> nit: everything

Done.