Streamline opening of omnibox.

Reuses existing view if one was around.

BUG=none

Review URL: https://codereview.chromium.org/1297733002

Cr-Commit-Position: refs/heads/master@{#343517}

10 files changed, 96 insertions, 77 deletions

diff --git a/mandoline/ui/browser/android/android_ui.cc b/mandoline/ui/browser/android/android_ui.cc
index ab8a49c..4423de7 100644
--- a/mandoline/ui/browser/android/android_ui.cc
+++ b/mandoline/ui/browser/android/android_ui.cc
@@ -29,6 +29,7 @@ void AndroidUI::Init(mojo::View* root) {
   browser_->content()->SetBounds(root_->bounds());
 }
 
+void AndroidUI::EmbedOmnibox(mojo::ApplicationConnection* connection) {}
 void AndroidUI::OnURLChanged() {}
 void AndroidUI::LoadingStateChanged(bool loading) {}
 void AndroidUI::ProgressChanged(double progress) {}
diff --git a/mandoline/ui/browser/android/android_ui.h b/mandoline/ui/browser/android/android_ui.h
index 440092a..c9bf6e0 100644
--- a/mandoline/ui/browser/android/android_ui.h
+++ b/mandoline/ui/browser/android/android_ui.h
@@ -27,6 +27,7 @@ class AndroidUI : public BrowserUI,
  private:
   // Overridden from BrowserUI:
   void Init(mojo::View* root) override;
+  void EmbedOmnibox(mojo::ApplicationConnection* connection) override;
   void OnURLChanged() override;
   void LoadingStateChanged(bool loading) override;
   void ProgressChanged(double progress) override;
diff --git a/mandoline/ui/browser/browser.cc b/mandoline/ui/browser/browser.cc
index c8cdac3..def4a9f 100644
--- a/mandoline/ui/browser/browser.cc
+++ b/mandoline/ui/browser/browser.cc
@@ -39,7 +39,6 @@ Browser::Browser(mojo::ApplicationImpl* app,
     : view_manager_init_(app, this, this),
       root_(nullptr),
       content_(nullptr),
-      omnibox_(nullptr),
       default_url_(default_url),
       navigator_host_(this),
       app_(app),
@@ -58,6 +57,23 @@ void Browser::ReplaceContentWithRequest(mojo::URLRequestPtr request) {
   Embed(request.Pass());
 }
 
+void Browser::ShowOmnibox() {
+  if (!omnibox_.get()) {
+    mojo::URLRequestPtr request(mojo::URLRequest::New());
+    request->url = mojo::String::From("mojo:omnibox");
+    omnibox_connection_ = app_->ConnectToApplication(request.Pass());
+    omnibox_connection_->AddService<ViewEmbedder>(this);
+    omnibox_connection_->ConnectToService(&omnibox_);
+    omnibox_connection_->SetRemoteServiceProviderConnectionErrorHandler(
+        [this]() {
+          // This will cause the connection to be re-established the next time
+          // we come through this codepath.
+          omnibox_.reset();
+        });
+  }
+  omnibox_->ShowForURL(mojo::String::From(current_url_.spec()));
+}
+
 mojo::ApplicationConnection* Browser::GetViewManagerConnectionForTesting() {
   return view_manager_init_.connection();
 }
@@ -142,17 +158,12 @@ void Browser::OnAccelerator(mojo::EventPtr event) {
   navigator_host_.RequestNavigateHistory(-1);
 }
 
-void Browser::OpenURL(const mojo::String& url) {
-  omnibox_->SetVisible(false);
-  mojo::URLRequestPtr request(mojo::URLRequest::New());
-  request->url = mojo::String::From(url);
-  ReplaceContentWithRequest(request.Pass());
-}
-
+// TODO(beng): Consider moving this to the UI object as well once the frame tree
+//             stuff is better encapsulated.
 void Browser::Embed(mojo::URLRequestPtr request) {
   const std::string string_url = request->url.To<std::string>();
   if (string_url == "mojo:omnibox") {
-    ShowOmnibox(request.Pass());
+    ui_->EmbedOmnibox(omnibox_connection_.get());
     return;
   }
 
@@ -208,21 +219,6 @@ void Browser::Create(mojo::ApplicationConnection* connection,
   view_embedder_bindings_.AddBinding(this, request.Pass());
 }
 
-void Browser::ShowOmnibox(mojo::URLRequestPtr request) {
-  if (!omnibox_) {
-    omnibox_ = root_->view_manager()->CreateView();
-    root_->AddChild(omnibox_);
-    omnibox_->SetBounds(root_->bounds());
-  }
-  mojo::ViewManagerClientPtr view_manager_client;
-  scoped_ptr<mojo::ApplicationConnection> connection =
-      app_->ConnectToApplication(request.Pass());
-  connection->AddService<ViewEmbedder>(this);
-  connection->ConnectToService(&view_manager_client);
-  omnibox_->Embed(view_manager_client.Pass());
-  omnibox_->SetVisible(true);
-}
-
 void Browser::RequestNavigate(Frame* source,
                               NavigationTargetType target_type,
                               Frame* target_frame,
diff --git a/mandoline/ui/browser/browser.h b/mandoline/ui/browser/browser.h
index dcca090..e55fa30 100644
--- a/mandoline/ui/browser/browser.h
+++ b/mandoline/ui/browser/browser.h
@@ -36,7 +36,6 @@ class FrameTree;
 
 class Browser : public mojo::ViewManagerDelegate,
                 public mojo::ViewManagerRootClient,
-                public OmniboxClient,
                 public ViewEmbedder,
                 public FrameTreeDelegate,
                 public mojo::InterfaceFactory<mojo::NavigatorHost>,
@@ -50,10 +49,12 @@ class Browser : public mojo::ViewManagerDelegate,
   void ReplaceContentWithRequest(mojo::URLRequestPtr request);
 
   mojo::View* content() { return content_; }
-  mojo::View* omnibox() { return omnibox_; }
 
   const GURL& current_url() const { return current_url_; }
 
+  // Starts the Omnibox application (if necessary) and shows it.
+  void ShowOmnibox();
+
  private:
   FRIEND_TEST_ALL_PREFIXES(BrowserTest, ClosingBrowserClosesAppConnection);
   FRIEND_TEST_ALL_PREFIXES(BrowserTest, TwoBrowsers);
@@ -75,9 +76,6 @@ class Browser : public mojo::ViewManagerDelegate,
   // Overridden from ViewManagerRootClient:
   void OnAccelerator(mojo::EventPtr event) override;
 
-  // Overridden from OmniboxClient:
-  void OpenURL(const mojo::String& url) override;
-
   // Overridden from ViewEmbedder:
   void Embed(mojo::URLRequestPtr request) override;
 
@@ -100,15 +98,12 @@ class Browser : public mojo::ViewManagerDelegate,
   void Create(mojo::ApplicationConnection* connection,
               mojo::InterfaceRequest<ViewEmbedder> request) override;
 
-  void ShowOmnibox(mojo::URLRequestPtr request);
-
   mojo::ViewManagerInit view_manager_init_;
 
   // Only support being embedded once, so both application-level
   // and embedding-level state are shared on the same object.
   mojo::View* root_;
   mojo::View* content_;
-  mojo::View* omnibox_;
   GURL default_url_;
   mojo::URLRequestPtr pending_request_;
 
@@ -118,6 +113,9 @@ class Browser : public mojo::ViewManagerDelegate,
 
   GURL current_url_;
 
+  OmniboxPtr omnibox_;
+  scoped_ptr<mojo::ApplicationConnection> omnibox_connection_;
+
   scoped_ptr<BrowserUI> ui_;
   mojo::ApplicationImpl* app_;
   BrowserDelegate* delegate_;
diff --git a/mandoline/ui/browser/browser_ui.h b/mandoline/ui/browser/browser_ui.h
index 5c94863..e22531d 100644
--- a/mandoline/ui/browser/browser_ui.h
+++ b/mandoline/ui/browser/browser_ui.h
@@ -6,6 +6,7 @@
 #define MANDOLINE_UI_BROWSER_BROWSER_UI_H_
 
 namespace mojo {
+class ApplicationConnection;
 class ApplicationImpl;
 class View;
 }
@@ -26,6 +27,11 @@ class BrowserUI {
   // them.
   virtual void Init(mojo::View* root) = 0;
 
+  // Embeds the Omnibox UI. The connection object passed is an existing
+  // connection to the Omnibox application from which a ViewManagerClient can be
+  // obtained.
+  virtual void EmbedOmnibox(mojo::ApplicationConnection* connection) = 0;
+
   virtual void OnURLChanged() = 0;
 
   virtual void LoadingStateChanged(bool loading) = 0;
diff --git a/mandoline/ui/browser/desktop/desktop_ui.cc b/mandoline/ui/browser/desktop/desktop_ui.cc
index 2e5737c..489e3c6 100644
--- a/mandoline/ui/browser/desktop/desktop_ui.cc
+++ b/mandoline/ui/browser/desktop/desktop_ui.cc
@@ -65,8 +65,8 @@ DesktopUI::DesktopUI(Browser* browser, mojo::ApplicationImpl* application_impl)
       omnibox_launcher_(nullptr),
       progress_bar_(nullptr),
       root_(nullptr),
-      client_binding_(browser) {
-}
+      content_(nullptr),
+      omnibox_(nullptr) {}
 
 DesktopUI::~DesktopUI() {}
 
@@ -75,6 +75,8 @@ DesktopUI::~DesktopUI() {}
 
 void DesktopUI::Init(mojo::View* root) {
   root_ = root;
+  omnibox_ = root_->view_manager()->CreateView();
+  root_->AddChild(omnibox_);
 
   views::WidgetDelegateView* widget_delegate = new views::WidgetDelegateView;
   widget_delegate->GetContentsView()->set_background(
@@ -99,6 +101,18 @@ void DesktopUI::Init(mojo::View* root) {
   root_->SetFocus();
 }
 
+void DesktopUI::EmbedOmnibox(mojo::ApplicationConnection* connection) {
+  mojo::ViewManagerClientPtr view_manager_client;
+  connection->ConnectToService(&view_manager_client);
+  omnibox_->Embed(view_manager_client.Pass());
+
+  // TODO(beng): This should be handled sufficiently by
+  //             OmniboxImpl::ShowWindow() but unfortunately view manager policy
+  //             currently prevents the embedded app from changing window z for
+  //             its own window.
+  omnibox_->MoveToFront();
+}
+
 void DesktopUI::OnURLChanged() {
   omnibox_launcher_->SetText(base::UTF8ToUTF16(browser_->current_url().spec()));
 }
@@ -136,30 +150,16 @@ void DesktopUI::Layout(views::View* host) {
   content_bounds_mojo.height =
       host->bounds().height() - content_bounds_mojo.y - 10;
   browser_->content()->SetBounds(content_bounds_mojo);
-
-  if (browser_->omnibox()) {
-    browser_->omnibox()->SetBounds(
-        mojo::TypeConverter<mojo::Rect, gfx::Rect>::Convert(host->bounds()));
-  }
+  omnibox_->SetBounds(
+      mojo::TypeConverter<mojo::Rect, gfx::Rect>::Convert(host->bounds()));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DesktopUI, views::ButtonListener implementation:
 
 void DesktopUI::ButtonPressed(views::Button* sender, const ui::Event& event) {
-  if (!omnibox_.get()) {
-    DCHECK(!client_binding_.is_bound());
-    mojo::URLRequestPtr request(mojo::URLRequest::New());
-    request->url = mojo::String::From("mojo:omnibox");
-    omnibox_connection_ =
-        application_impl_->ConnectToApplication(request.Pass());
-    omnibox_connection_->AddService<ViewEmbedder>(browser_);
-    omnibox_connection_->ConnectToService(&omnibox_);
-    OmniboxClientPtr client;
-    client_binding_.Bind(&client);
-    omnibox_->SetClient(client.Pass());
-  }
-  omnibox_->ShowForURL(mojo::String::From(browser_->current_url().spec()));
+  DCHECK_EQ(sender, omnibox_launcher_);
+  browser_->ShowOmnibox();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/mandoline/ui/browser/desktop/desktop_ui.h b/mandoline/ui/browser/desktop/desktop_ui.h
index 6f5ce3f..f301bcd 100644
--- a/mandoline/ui/browser/desktop/desktop_ui.h
+++ b/mandoline/ui/browser/desktop/desktop_ui.h
@@ -35,6 +35,7 @@ class DesktopUI : public BrowserUI,
  private:
   // Overridden from BrowserUI
   void Init(mojo::View* root) override;
+  void EmbedOmnibox(mojo::ApplicationConnection* connection) override;
   void OnURLChanged() override;
   void LoadingStateChanged(bool loading) override;
   void ProgressChanged(double progress) override;
@@ -46,15 +47,15 @@ class DesktopUI : public BrowserUI,
   // Overridden from views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
+  void ShowOmnibox();
+
   Browser* browser_;
   mojo::ApplicationImpl* application_impl_;
   views::LabelButton* omnibox_launcher_;
   ProgressView* progress_bar_;
   mojo::View* root_;
   mojo::View* content_;
-  OmniboxPtr omnibox_;
-  mojo::Binding<OmniboxClient> client_binding_;
-  scoped_ptr<mojo::ApplicationConnection> omnibox_connection_;
+  mojo::View* omnibox_;
 
   DISALLOW_COPY_AND_ASSIGN(DesktopUI);
 };
diff --git a/mandoline/ui/browser/public/interfaces/omnibox.mojom b/mandoline/ui/browser/public/interfaces/omnibox.mojom
index b69b543..01a3069 100644
--- a/mandoline/ui/browser/public/interfaces/omnibox.mojom
+++ b/mandoline/ui/browser/public/interfaces/omnibox.mojom
@@ -6,15 +6,7 @@ module mandoline;
 
 // TODO(beng): Could this be generalized as a "Chooser" with a specific type??
 
-// Interface implemented by the browser to respond to input from the omnibox
-// app.
-interface OmniboxClient {
-  OpenURL(string url);
-};
-
 // Interface implemented by an application providing a method for URL input.
 interface Omnibox {
-  SetClient(OmniboxClient client);
-
   ShowForURL(string url);
 };
diff --git a/mandoline/ui/omnibox/omnibox_impl.cc b/mandoline/ui/omnibox/omnibox_impl.cc
index eba94bb..2cb8e8b 100644
--- a/mandoline/ui/omnibox/omnibox_impl.cc
+++ b/mandoline/ui/omnibox/omnibox_impl.cc
@@ -21,9 +21,7 @@ namespace mandoline {
 // OmniboxImpl, public:
 
 OmniboxImpl::OmniboxImpl()
-    : app_impl_(nullptr),
-      edit_(nullptr) {
-}
+    : app_impl_(nullptr), root_(nullptr), edit_(nullptr) {}
 OmniboxImpl::~OmniboxImpl() {}
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -52,6 +50,8 @@ bool OmniboxImpl::ConfigureOutgoingConnection(
 // OmniboxImpl, mojo::ViewManagerDelegate implementation:
 
 void OmniboxImpl::OnEmbed(mojo::View* root) {
+  root_ = root;
+
   if (!aura_init_.get()) {
     aura_init_.reset(new AuraInit(root, app_impl_->shell()));
     edit_ = new views::Textfield;
@@ -80,13 +80,15 @@ void OmniboxImpl::OnEmbed(mojo::View* root) {
   widget->Show();
   widget->GetCompositor()->SetBackgroundColor(
       SkColorSetA(SK_ColorBLACK, kOpacity));
-  root->SetFocus();
   edit_->SetText(url_.To<base::string16>());
   edit_->SelectAll(false);
   edit_->RequestFocus();
+
+  ShowWindow();
 }
 
 void OmniboxImpl::OnViewManagerDestroyed(mojo::ViewManager* view_manager) {
+  root_ = nullptr;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -111,7 +113,10 @@ bool OmniboxImpl::HandleKeyEvent(views::Textfield* sender,
                                  const ui::KeyEvent& key_event) {
   if (key_event.key_code() == ui::VKEY_RETURN) {
     // TODO(beng): call back to browser.
-    client_->OpenURL(mojo::String::From<base::string16>(sender->text()));
+    mojo::URLRequestPtr request(mojo::URLRequest::New());
+    request->url = mojo::String::From<base::string16>(sender->text());
+    view_embedder_->Embed(request.Pass());
+    HideWindow();
     return true;
   }
   return false;
@@ -122,21 +127,38 @@ bool OmniboxImpl::HandleKeyEvent(views::Textfield* sender,
 
 void OmniboxImpl::Create(mojo::ApplicationConnection* connection,
                          mojo::InterfaceRequest<Omnibox> request) {
+  // TODO(beng): methinks this doesn't work well across multiple browser
+  //             windows...
   bindings_.AddBinding(this, request.Pass());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxImpl, Omnibox implementation:
 
-void OmniboxImpl::SetClient(OmniboxClientPtr client) {
-  client_ = client.Pass();
-}
-
 void OmniboxImpl::ShowForURL(const mojo::String& url) {
   url_ = url;
-  mojo::URLRequestPtr request(mojo::URLRequest::New());
-  request->url = mojo::String::From("mojo:omnibox");
-  view_embedder_->Embed(request.Pass());
+  if (root_) {
+    ShowWindow();
+  } else {
+    mojo::URLRequestPtr request(mojo::URLRequest::New());
+    request->url = mojo::String::From("mojo:omnibox");
+    view_embedder_->Embed(request.Pass());
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// OmniboxImpl, private:
+
+void OmniboxImpl::ShowWindow() {
+  DCHECK(root_);
+  root_->SetVisible(true);
+  root_->SetFocus();
+  root_->MoveToFront();
+}
+
+void OmniboxImpl::HideWindow() {
+  DCHECK(root_);
+  root_->SetVisible(false);
 }
 
 }  // namespace mandoline
diff --git a/mandoline/ui/omnibox/omnibox_impl.h b/mandoline/ui/omnibox/omnibox_impl.h
index 06bec66..3f0514a 100644
--- a/mandoline/ui/omnibox/omnibox_impl.h
+++ b/mandoline/ui/omnibox/omnibox_impl.h
@@ -57,12 +57,14 @@ class OmniboxImpl : public mojo::ApplicationDelegate,
               mojo::InterfaceRequest<Omnibox> request) override;
 
   // Overridden from Omnibox:
-  void SetClient(OmniboxClientPtr client) override;
   void ShowForURL(const mojo::String& url) override;
 
-  OmniboxClientPtr client_;
+  void HideWindow();
+  void ShowWindow();
+
   scoped_ptr<AuraInit> aura_init_;
   mojo::ApplicationImpl* app_impl_;
+  mojo::View* root_;
   mojo::String url_;
   views::Textfield* edit_;
   mojo::WeakBindingSet<Omnibox> bindings_;