Instant extended API: Make arrow up/down work.

Instant extended API: Make arrow up/down work.

BUG=none
R=sky@chromium.org,pkasting@chromium.org
TEST=In extended API, arrow up/down should work. Pressing Enter should
use the selected match.


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

[sreeram, 2012-09-17 22:15:49]

@sky for everything
@pkasting for chrome/browser/ui/omnibox/*

[Peter Kasting, 2012-09-17 23:02:28]

In the instant extended mode, we still have what appears to be a popup.  Why
can't we have a popup model that contains the correct bits?  Then no changes to
the edit are necessary, and more importantly, the core concepts and information
silos are the same.

It seems like all we really need is a layer inserted between the controller and
its listeners to allow instant to receive and modulate the set of local
suggestions before they're passed on to the edit and popup, no?

[sreeram, 2012-09-17 23:39:34]

On 2012/09/17 23:02:28, Peter Kasting wrote:
> In the instant extended mode, we still have what appears to be a popup.  Why
> can't we have a popup model that contains the correct bits?  Then no changes
to
> the edit are necessary, and more importantly, the core concepts and
information
> silos are the same.

This is currently not possible because the page doesn't tell us about the list
it's showing. It tells us only the top suggestion. It can perhaps be coerced
(for the time being) into telling us the entire list (so we can populate the
popup model correctly), but in the future, the page wants the flexibility to
present suggestions that may not easily map to our popup model structure. For
example, maybe it will show two columns of suggestions, or suggestions that have
sub-suggestions (that may only be retrieved or displayed after the user selects
and hits right-arrow), or image suggestions.

So, our proposal for handling that is to completely decouple the popup model
from the omnibox edit. Shishir is still working on that. Until that happens,
this seems like a workable intermediate solution.

> It seems like all we really need is a layer inserted between the controller
and
> its listeners to allow instant to receive and modulate the set of local
> suggestions before they're passed on to the edit and popup, no?

Instant's suggestions are async, so I don't think such a layer can be easily
inserted today, but yes, Shishir's redesign decouples the autocomplete results
from the omnibox edit. Briefly, the main point of the redesign is that the
OmniboxEditModel and OmniboxView don't really need to know much about the
results or the popup or even Instant, except for the "top match" (aka, whatever
the omnibox should be showing).

[Peter Kasting, 2012-09-18 00:59:37]

On 2012/09/17 23:39:34, sreeram wrote:
> On 2012/09/17 23:02:28, Peter Kasting wrote:
> > In the instant extended mode, we still have what appears to be a popup.  Why
> > can't we have a popup model that contains the correct bits?  Then no changes
> to
> > the edit are necessary, and more importantly, the core concepts and
> information
> > silos are the same.
> 
> This is currently not possible because the page doesn't tell us about the list
> it's showing. It tells us only the top suggestion. It can perhaps be coerced
> (for the time being) into telling us the entire list (so we can populate the
> popup model correctly), but in the future, the page wants the flexibility to
> present suggestions that may not easily map to our popup model structure. For
> example, maybe it will show two columns of suggestions, or suggestions that
have
> sub-suggestions (that may only be retrieved or displayed after the user
selects
> and hits right-arrow), or image suggestions.

FWIW, I don't think any of these really pose any problem.  The edit already
knows fairly little about the popup.  Much of the implementation of the popup
model is only necessary for the popup view, which you'll be doing away with. 
The few things the edit needs to know about the model can be represented as a
base class that, if instant extended becomes the only implementation, can become
the only model class.

It should be really easy to do this particular bit of separation right away and
would obviate the need to make any changes in the edit.

I don't know what more beyond this you have Shishir doing.  You may want to have
me review implementation proposals before you spend a bunch of time on them and
then have me ask for a completely different approach :)

[sreeram, 2012-09-18 03:17:25]

On 2012/09/18 00:59:37, Peter Kasting wrote:
> FWIW, I don't think any of these really pose any problem.  The edit already
> knows fairly little about the popup.  Much of the implementation of the popup
> model is only necessary for the popup view, which you'll be doing away with. 
> The few things the edit needs to know about the model can be represented as a
> base class that, if instant extended becomes the only implementation, can
become
> the only model class.
> 
> It should be really easy to do this particular bit of separation right away
and
> would obviate the need to make any changes in the edit.
> 
> I don't know what more beyond this you have Shishir doing.  You may want to
have
> me review implementation proposals before you spend a bunch of time on them
and
> then have me ask for a completely different approach :)

I agree with you about the way things ought to be. And yes, when we have a
design, we'll certainly run it by you before implementing it. The thing is,
omnibox_edit_model.cc is a lot more intertwined than was initially apparent. It
makes reference to AutocompleteResult and OmniboxPopupModel in many places. (And
OmniboxPopupModel and the View also reach directly into AutocompleteResult.) The
edit deals with SearchProvider, KeywordProvider and Instant directly. It has
nitty gritties to do with various key presses (arrows, Control, Escape, etc).

Added to all that, we need a design that would work, at runtime, in both the old
style (native popup) as well as the new style (renderer popup), since the new
style is not yet the default and can be switched on/off with a flag. So, it has
been taking some time to flesh out all the details in the design. Shishir has
been working diligently on this.

In the interim, this one piece of functionality (arrowing up/down) has been
holding back the extended API dogfood for quite a while already. Hence the
request to accept this interim solution while we work towards the final
solution.

[Peter Kasting, 2012-09-18 18:06:08]

LGTM then, but add comments on your member saying that it's a temporary hack
that should be ripped out when ____.

[sreeram, 2012-09-18 18:12:55]

On 2012/09/18 18:06:08, Peter Kasting wrote:
> LGTM then, but add comments on your member saying that it's a temporary hack
> that should be ripped out when ____.

Thanks! Done.

[sky, 2012-09-18 19:44:21]

http://codereview.chromium.org/10918289/diff/10001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10918289/diff/10001/chrome/common/render_messa...
chrome/common/render_messages.h:307: int /* count */)
Should this be a bool?

[sreeram, 2012-09-18 19:54:37]

http://codereview.chromium.org/10918289/diff/10001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10918289/diff/10001/chrome/common/render_messa...
chrome/common/render_messages.h:307: int /* count */)
On 2012/09/18 19:44:21, sky wrote:
> Should this be a bool?

No. For example, if the user presses PageDown, we get a count larger than 1
(similarly, count < -1 for PageUp).

[sky, 2012-09-18 19:58:03]

On Tue, Sep 18, 2012 at 12:54 PM,  <sreeram@chromium.org> wrote:
>
>
http://codereview.chromium.org/10918289/diff/10001/chrome/common/render_messa...
> File chrome/common/render_messages.h (right):
>
>
http://codereview.chromium.org/10918289/diff/10001/chrome/common/render_messa...
> chrome/common/render_messages.h:307: int /* count */)
> On 2012/09/18 19:44:21, sky wrote:
>>
>> Should this be a bool?
>
>
> No. For example, if the user presses PageDown, we get a count larger
> than 1 (similarly, count < -1 for PageUp).
>
> http://codereview.chromium.org/10918289/

The code doesn't do that though. We should wait until you actually need that.

  -Scott

[sreeram, 2012-09-18 20:12:21]

On 2012/09/18 19:58:03, sky wrote:
> > No. For example, if the user presses PageDown, we get a count larger
> > than 1 (similarly, count < -1 for PageUp).
> 
> The code doesn't do that though. We should wait until you actually need that.

Wow, you're right. PageUp & PageDown on ChromeOS don't do anything. This sounds
like a bug that should be fixed. The other platforms (Win, Mac, Linux) all set
count > 1.

So, should I make this a bool, only to (perhaps very soon) turn it back into int
when the PageUp/PageDown thing is fixed? Or, leave it as int, and it will
automatically work when the bug is fixed (or when the extended mode is enabled
on other platforms).

[Peter Kasting, 2012-09-18 20:18:29]

I would leave as an int and make sure there is a bug about pgup/pgdn not working
on CrOS that someone is fixing.

[sreeram, 2012-09-18 20:40:19]

On 2012/09/18 20:18:29, Peter Kasting wrote:
> I would leave as an int and make sure there is a bug about pgup/pgdn not
working
> on CrOS that someone is fixing.

It turns out CrOS devices (at least the current crop) don't have these keys.
Bugs have been filed to emulate PageUp/PageDown: http://crbug.com/147628 and
http://crbug.com/148800.

[sky, 2012-09-18 22:20:29]

LGTM

[commit-bot: I haz the power, 2012-09-18 22:28:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/10918289/10001

[commit-bot: I haz the power, 2012-09-19 00:40:30]

Change committed as 157466