Omnibox refactor, moved OnResultChanged to OmniboxController

Omnibox refactor, moved OnResultChanged to OmniboxController

Follow up on https://codereview.chromium.org/13932034/

BUG=234733
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201839

[Mathieu, 2013-04-24 16:45:56]

https://chromiumcodereview.appspot.com/14358005/diff/1/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://chromiumcodereview.appspot.com/14358005/diff/1/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:76: if (popup_->IsOpen()) {
I've never liked this method name in the context of InstantExtended. Perhaps add
a TODO somewhere in this CL to modify the code so that we can avoid references
to the popup model when instant is enabled. I feel we should do it sooner than
later, as we will be decoupling normal and instant behaviors.

[beaudoin, 2013-04-30 15:30:40]

Still early stage, don't spend too much time reviewing this.

[sreeram, 2013-04-30 18:07:50]

On 2013/04/30 15:30:40, beaudoin wrote:
> Still early stage, don't spend too much time reviewing this.

Okay. Please let us know when you'd like us to take a look.

[beaudoin, 2013-05-03 18:57:27]

This is ready for review. Slowly moving more and more code into
OmniboxController.

[Mathieu, 2013-05-03 19:08:47]

On 2013/05/03 18:57:27, beaudoin wrote:
> This is ready for review. Slowly moving more and more code into
> OmniboxController.

A few nits, but lgtm overall! Over to sreeram.

[Mathieu, 2013-05-03 19:09:02]

Bleh, here are the nits.

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:43: //     instant extended,
remove this reference.
nit: necessary indent?

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:51: if (match != result.end()) {
Generally, don't hesitate to add comments if you feel you understand all this
code better now. I've always found these methods opaque and I would love more
comments such as "At this point we are in state X and this will do Y".

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.h (right):

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.h:46: // TODO: The edit and popup
should be siblings owned by the LocationBarView,
nit: the edit and popup what? be clearer, as it could be model, view,
controller...

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:976: DLOG(ERROR) << "A";
remove

[beaudoin, 2013-05-04 03:06:51]

Answered Mathieu's comments. As I was digging down to understand what's going on
I found an opportunity to move OnResultChanged to the OmniboxController which I
did.

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:43: //     instant extended,
remove this reference.
On 2013/05/03 19:09:02, Mathieu Perreault wrote:
> nit: necessary indent?

Done.

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:51: if (match != result.end()) {
On 2013/05/03 19:09:02, Mathieu Perreault wrote:
> Generally, don't hesitate to add comments if you feel you understand all this
> code better now. I've always found these methods opaque and I would love more
> comments such as "At this point we are in state X and this will do Y".

Did some digging and added some comment. Also refactored some stuff as I was
doing this. Let me know if you think other parts should be clarified.

Done.

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.h (right):

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.h:46: // TODO: The edit and popup
should be siblings owned by the LocationBarView,
On 2013/05/03 19:09:02, Mathieu Perreault wrote:
> nit: the edit and popup what? be clearer, as it could be model, view,
> controller...

Done.

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/14358005/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:976: DLOG(ERROR) << "A";
On 2013/05/03 19:09:02, Mathieu Perreault wrote:
> remove

/facepalm. :)

Done.

[beaudoin, 2013-05-04 03:22:36]

I meant: move OnPopupBoundsChanged to the OmniboxController.

[sreeram, 2013-05-22 15:33:45]

lgtm

https://codereview.chromium.org/14358005/diff/27001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/14358005/diff/27001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:263:
search_provider->ClearInstantSuggestion();
Why put back the check? I took it purposefully, because Instant should never be
active if there's no search provider.

[beaudoin, 2013-05-22 17:06:00]

+pkasting for OWNERS.

https://codereview.chromium.org/14358005/diff/27001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/14358005/diff/27001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:263:
search_provider->ClearInstantSuggestion();
On 2013/05/22 15:33:45, sreeram wrote:
> Why put back the check? I took it purposefully, because Instant should never
be
> active if there's no search provider.

Probably a merge mistake.

Done.

[Peter Kasting, 2013-05-22 23:56:54]

Rubber-stamp LGTM

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:91: // Accepts the temporary
text as the user text, because it makes little
Nit: Accepts -> Accept

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.h (right):

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.h:57: const AutocompleteResult&
result() const;
Lower-case getters like this have to be inlined or they shouldn't be
lower-cased.

It's OK to pull in autocompelte_result.h in order to do this.

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:92:
omnibox_controller_->set_popup_model(popup_model);
I have no idea whether this is possible but it'd be cool to nuke this and
instead pass the popup model to the omnibox controller at construction time or
something.

[beaudoin, 2013-05-23 14:16:41]

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:91: // Accepts the temporary
text as the user text, because it makes little
On 2013/05/22 23:56:54, Peter Kasting wrote:
> Nit: Accepts -> Accept

Done.

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.h (right):

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.h:57: const AutocompleteResult&
result() const;
On 2013/05/22 23:56:54, Peter Kasting wrote:
> Lower-case getters like this have to be inlined or they shouldn't be
> lower-cased.
> 
> It's OK to pull in autocompelte_result.h in order to do this.

Done.

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:92:
omnibox_controller_->set_popup_model(popup_model);
On 2013/05/22 23:56:54, Peter Kasting wrote:
> I have no idea whether this is possible but it'd be cool to nuke this and
> instead pass the popup model to the omnibox controller at construction time or
> something.

I'd love this, unfortunately they're on two totally different lifecycles and the
OmniboxController outlives the popup_model. It roughly works like this:
- OmniboxView owns OmniboxEditModel who owns OmniboxController
- OmniboxPopupView owns OmniboxPopupModel

I've been thinking of an OmniboxPopupModel that would outlive the PopupView (ie.
owned by OmniboxView or below) but I'm not sure it would be useful, besides
cleaning-up the lifecycle above.

[commit-bot: I haz the power, 2013-05-23 14:17:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/14358005/39001

[sreeram, 2013-05-23 14:18:45]

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/14358005/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:92:
omnibox_controller_->set_popup_model(popup_model);
On 2013/05/23 14:16:41, beaudoin wrote:
> On 2013/05/22 23:56:54, Peter Kasting wrote:
> > I have no idea whether this is possible but it'd be cool to nuke this and
> > instead pass the popup model to the omnibox controller at construction time
or
> > something.
> 
> I'd love this, unfortunately they're on two totally different lifecycles and
the
> OmniboxController outlives the popup_model. It roughly works like this:
> - OmniboxView owns OmniboxEditModel who owns OmniboxController
> - OmniboxPopupView owns OmniboxPopupModel
> 
> I've been thinking of an OmniboxPopupModel that would outlive the PopupView
(ie.
> owned by OmniboxView or below) but I'm not sure it would be useful, besides
> cleaning-up the lifecycle above.

I wouldn't worry too much about this. One of the goals of this refactor is to
allow us to get to a state where we can get rid of the popup model and view
entirely.

[commit-bot: I haz the power, 2013-05-23 18:50:28]

Change committed as 201839