Omnibox refactor, introduced OmniboxController.

chrome/browser/ui/omnibox/omnibox_controller.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_CONTROLLER_H_
+#define CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_CONTROLLER_H_
+
+#include "base/basictypes.h"

[Peter Kasting, 2013/04/25 18:24:55]

Is this #include needed?

[beaudoin, 2013/04/25 19:14:57]

Gone. Done.

+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/autocomplete/autocomplete_controller_delegate.h"
+
+class AutocompleteController;
+class OmniboxEditModel;
+class Profile;
+
+// This class sits between the OmniboxEditModel and AutocompleteController.

[Peter Kasting, 2013/04/25 18:24:55]

Nit: This comment isn't really informative about what the class is/means/does. 
Instead say what the class _is_, from a high-level perspective.

[beaudoin, 2013/04/25 20:34:51]

I was torn between describing what this class will do once the refactor is
complete and what it does now. I've decided to clarify the comment and expand
the TODO so that I keep tracking the comment as functionality moves in.

Done.

+// TODO(beaudoin): Keep on expanding this class so that OmniboxEditModel no
+//     longer needs to hold any reference to AutocompleteController. Also make
+//     this the point of contact between InstantController and OmniboxEditModel.
+class OmniboxController : public AutocompleteControllerDelegate {
+
+ public:
+  OmniboxController(OmniboxEditModel* omnibox_edit_model, Profile* profile);
+  virtual ~OmniboxController();
+
+  AutocompleteController* autocomplete_controller() const {

[Peter Kasting, 2013/04/25 18:24:55]

Const methods must not return non-const pointers.

[beaudoin, 2013/04/25 20:34:51]

The exact same method is defined in omnibox_edit_model. I agree it's nasty and
we should solve it everywhere.

Making it return const breaks things all over the place so I made it non-const.
I've added a TODO to see if we can constify it, let me know if you think it's
worth it.

Done.

[Peter Kasting, 2013/04/25 20:52:09]

I'd probably skip the TODO.  I doubt we can avoid the need for a non-const
return pointer.

[beaudoin, 2013/04/25 23:51:33]

Done.

+    return autocomplete_controller_.get();
+  }
+
+  // AutocompleteControllerDelegate:

[Peter Kasting, 2013/04/25 18:24:55]

I suggest putting overrides above non-overrides.

[beaudoin, 2013/04/25 20:34:51]

Done.

+  virtual void OnResultChanged(bool default_match_changed) OVERRIDE;
+
+ private:
+  // Weak, it owns us.
+  // TODO(beaudoin): When we have a clearer picture of the interface between
+  //     this and OmniboxEditModel define and use a Delegate interface instead.

[Peter Kasting, 2013/04/25 18:24:55]

Why?  Is there a real win in defining such an interface when only one class will
ever implement it?  It seems like it would just add indirection.

[beaudoin, 2013/04/25 20:34:51]

Such an interface may make unit tests easier. In the follow-up patch my unit
test setup is significantly more complicated because I have to instantiate the
full OmniboxEditModel.

Far from sure it's worth it though, so for now I've removed the TODO.

Done.

[Peter Kasting, 2013/04/25 20:52:09]

Ah.  That's a reasonable justification.  Maybe just leave the TODO as saying
something like "consider defining a Delegate interface to ease unittesting" or
something.

+  OmniboxEditModel* omnibox_edit_model_;
+
+  scoped_ptr<AutocompleteController> autocomplete_controller_;
+
+  DISALLOW_COPY_AND_ASSIGN(OmniboxController);
+};
+
+#endif  // CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_CONTROLLER_H_