[Blimp] Creates engine tab class to handle tab related operations.

blimp/engine/session/blimp_engine_session.cc

Index: blimp/engine/session/blimp_engine_session.cc
diff --git a/blimp/engine/session/blimp_engine_session.cc b/blimp/engine/session/blimp_engine_session.cc
index bf1f7cfa2e0f5eae92b9069c4d1c81a2d586bf3d..de90f1b7250a56ff197f291c344c159d7ab0982e 100644
--- a/blimp/engine/session/blimp_engine_session.cc
+++ b/blimp/engine/session/blimp_engine_session.cc
@@ -23,6 +23,7 @@
 #include "blimp/engine/app/ui/blimp_window_tree_host.h"
 #include "blimp/engine/common/blimp_browser_context.h"
 #include "blimp/engine/common/blimp_user_agent.h"
+#include "blimp/engine/session/tab_blimp.h"
 #include "blimp/net/blimp_connection.h"
 #include "blimp/net/blimp_connection_statistics.h"
 #include "blimp/net/blimp_message_multiplexer.h"
@@ -215,8 +216,8 @@ BlimpEngineSession::BlimpEngineSession(
       browser_context_(std::move(browser_context)),
       engine_config_(engine_config),
       settings_manager_(settings_manager),
-      settings_feature_(settings_manager_),
-      render_widget_feature_(settings_manager_),
+      settings_feature_(new EngineSettingsFeature(settings_manager_)),
+      render_widget_feature_(new EngineRenderWidgetFeature(settings_manager_)),
       net_components_(
           new EngineNetworkComponents(net_log,
                                       QuitCurrentMessageLoopClosure())) {
@@ -225,19 +226,17 @@ BlimpEngineSession::BlimpEngineSession(
   screen_->UpdateDisplayScaleAndSize(kDefaultScaleFactor,
                                      gfx::Size(kDefaultDisplayWidth,
                                                kDefaultDisplayHeight));
-  render_widget_feature_.SetDelegate(kDummyTabId, this);
+  render_widget_feature_->SetDelegate(kDummyTabId, this);
 }
 
 BlimpEngineSession::~BlimpEngineSession() {
-  render_widget_feature_.RemoveDelegate(kDummyTabId);
+  render_widget_feature_->RemoveDelegate(kDummyTabId);
 
   window_tree_host_->GetInputMethod()->RemoveObserver(this);
 
-  page_load_tracker_.reset();
-
   // Ensure that all WebContents are torn down first, since teardown will

[Kevin M, 2016/06/02 18:38:50]

Revise this comment?

[haibinlu, 2016/06/02 20:24:36]

Done.

   // trigger RenderViewDeleted callbacks to their observers.
-  web_contents_.reset();
+  tab_.reset();
 
   // Safely delete network components on the IO thread.
   content::BrowserThread::DeleteSoon(content::BrowserThread::IO, FROM_HERE,
@@ -300,29 +299,29 @@ void BlimpEngineSession::RegisterFeatures() {
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kTabControl, this);
   navigation_message_sender_ =
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kNavigation, this);
-  render_widget_feature_.set_render_widget_message_sender(
+  render_widget_feature_->set_render_widget_message_sender(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kRenderWidget,
-                                            &render_widget_feature_));
-  render_widget_feature_.set_input_message_sender(
+                                            render_widget_feature_.get()));
+  render_widget_feature_->set_input_message_sender(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kInput,
-                                            &render_widget_feature_));
-  render_widget_feature_.set_compositor_message_sender(
+                                            render_widget_feature_.get()));
+  render_widget_feature_->set_compositor_message_sender(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kCompositor,
-                                            &render_widget_feature_));
-  render_widget_feature_.set_ime_message_sender(
+                                            render_widget_feature_.get()));
+  render_widget_feature_->set_ime_message_sender(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kIme,
-                                            &render_widget_feature_));
+                                            render_widget_feature_.get()));
 
   // The Settings feature does not need an outgoing message processor, since we
   // don't send any messages to the client right now.
   thread_pipe_manager_->RegisterFeature(BlimpMessage::kSettings,
-                                        &settings_feature_);
+                                        settings_feature_.get());
 }
 
 bool BlimpEngineSession::CreateWebContents(const int target_tab_id) {

[Kevin M, 2016/06/02 18:38:50]

Why is this called CreateWebContents, when it creates a tab?

[haibinlu, 2016/06/02 20:24:36]

Done.

   DVLOG(1) << "Create tab " << target_tab_id;
   // TODO(haibinlu): Support more than one active WebContents (crbug/547231).
-  if (web_contents_) {
+  if (tab_) {
     DLOG(WARNING) << "Tab " << target_tab_id << " already existed";
     return false;
   }
@@ -331,19 +330,15 @@ bool BlimpEngineSession::CreateWebContents(const int target_tab_id) {
                                                    nullptr);
   std::unique_ptr<content::WebContents> new_contents =
       base::WrapUnique(content::WebContents::Create(create_params));
-  PlatformSetContents(std::move(new_contents));
-
-  // Transfer over the user agent override. The default user agent does not
-  // have client IO info.
-  web_contents_->SetUserAgentOverride(GetBlimpEngineUserAgent());
+  PlatformSetContents(std::move(new_contents), target_tab_id);
 
   return true;
 }
 
 void BlimpEngineSession::CloseWebContents(const int target_tab_id) {
   DVLOG(1) << "Close tab " << target_tab_id;
-  DCHECK(web_contents_);
-  web_contents_->Close();
+  DCHECK(tab_);
+  tab_->Close();
 }
 
 void BlimpEngineSession::HandleResize(float device_pixel_ratio,
@@ -351,49 +346,26 @@ void BlimpEngineSession::HandleResize(float device_pixel_ratio,
   DVLOG(1) << "Resize to " << size.ToString() << ", " << device_pixel_ratio;
   screen_->UpdateDisplayScaleAndSize(device_pixel_ratio, size);
   window_tree_host_->SetBounds(gfx::Rect(size));
-  if (web_contents_) {
-    const gfx::Size size_in_dips = screen_->GetPrimaryDisplay().bounds().size();
-    web_contents_->GetNativeView()->SetBounds(gfx::Rect(size_in_dips));
-  }
-
-  if (web_contents_ && web_contents_->GetRenderViewHost() &&
-      web_contents_->GetRenderViewHost()->GetWidget()) {
-    web_contents_->GetRenderViewHost()->GetWidget()->WasResized();
+  if (tab_) {
+    tab_->Resize(device_pixel_ratio,
+                 screen_->GetPrimaryDisplay().bounds().size());
   }
 }
 
 void BlimpEngineSession::LoadUrl(const int target_tab_id, const GURL& url) {
-  TRACE_EVENT1("blimp", "BlimpEngineSession::LoadUrl", "URL", url.spec());
-  DVLOG(1) << "Load URL " << url << " in tab " << target_tab_id;
-  if (!url.is_valid()) {
-    VLOG(1) << "Dropping invalid URL " << url;
-    return;
-  }
-
-  // TODO(dtrainor, haibinlu): Fix up the URL with url_fixer.h.  If that doesn't
-  // produce a valid spec() then try to build a search query?
-  content::NavigationController::LoadURLParams params(url);
-  params.transition_type = ui::PageTransitionFromInt(
-      ui::PAGE_TRANSITION_TYPED | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
-  params.override_user_agent =
-    content::NavigationController::UA_OVERRIDE_TRUE;
-  web_contents_->GetController().LoadURLWithParams(params);
-  web_contents_->Focus();
+  tab_->LoadUrl(url);

[Kevin M, 2016/06/02 18:38:50]

Won't this segfault if tab_ is closed (and nulled) beforehand? Ditto for all
functions that reference tab_ that originate from network data.

[haibinlu, 2016/06/02 20:24:36]

added check. we have been depending on client does not send in invalid command.
But it's better to check first.

 }
 
 void BlimpEngineSession::GoBack(const int target_tab_id) {
-  DVLOG(1) << "Back in tab " << target_tab_id;
-  web_contents_->GetController().GoBack();
+  tab_->GoBack();
 }
 
 void BlimpEngineSession::GoForward(const int target_tab_id) {
-  DVLOG(1) << "Forward in tab " << target_tab_id;
-  web_contents_->GetController().GoForward();
+  tab_->GoForward();
 }
 
 void BlimpEngineSession::Reload(const int target_tab_id) {
-  DVLOG(1) << "Reload in tab " << target_tab_id;
-  web_contents_->GetController().Reload(true);
+  tab_->Reload();
 }
 
 void BlimpEngineSession::OnWebGestureEvent(
@@ -427,7 +399,7 @@ void BlimpEngineSession::OnCaretBoundsChanged(
 //  - the TextInputType of the TextInputClient changes
 void BlimpEngineSession::OnTextInputStateChanged(
     const ui::TextInputClient* client) {
-  if (!web_contents_->GetRenderWidgetHostView())
+  if (!tab_->web_contents()->GetRenderWidgetHostView())
     return;
 
   ui::TextInputType type =
@@ -438,9 +410,9 @@ void BlimpEngineSession::OnTextInputStateChanged(
   // changes to ui::TEXT_INPUT_TYPE_NONE. For other text input types,
   // OnShowImeIfNeeded is used instead to send show IME request to client.
   if (type == ui::TEXT_INPUT_TYPE_NONE)
-    render_widget_feature_.SendHideImeRequest(
+    render_widget_feature_->SendHideImeRequest(
         kDummyTabId,
-        web_contents_->GetRenderWidgetHostView()->GetRenderWidgetHost());
+        tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost());
 }
 
 void BlimpEngineSession::OnInputMethodDestroyed(
@@ -449,13 +421,13 @@ void BlimpEngineSession::OnInputMethodDestroyed(
 // Called when a user input should trigger showing the IME.
 void BlimpEngineSession::OnShowImeIfNeeded() {
   TRACE_EVENT0("blimp", "BlimpEngineSession::OnShowImeIfNeeded");
-  if (!web_contents_->GetRenderWidgetHostView() ||
+  if (!tab_->web_contents()->GetRenderWidgetHostView() ||
       !window_tree_host_->GetInputMethod()->GetTextInputClient())
     return;
 
-  render_widget_feature_.SendShowImeRequest(
+  render_widget_feature_->SendShowImeRequest(
       kDummyTabId,
-      web_contents_->GetRenderWidgetHostView()->GetRenderWidgetHost(),
+      tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost(),
       window_tree_host_->GetInputMethod()->GetTextInputClient());
 }
 
@@ -486,7 +458,7 @@ void BlimpEngineSession::ProcessMessage(
         NOTIMPLEMENTED();
         result = net::ERR_NOT_IMPLEMENTED;
     }
-  } else if (message->has_navigation() && web_contents_) {
+  } else if (message->has_navigation() && tab_) {
     switch (message->navigation().type()) {
       case NavigationMessage::LOAD_URL:
         LoadUrl(message->target_tab_id(),
@@ -555,9 +527,8 @@ void BlimpEngineSession::RequestToLockMouse(content::WebContents* web_contents,
 }
 
 void BlimpEngineSession::CloseContents(content::WebContents* source) {
-  if (source == web_contents_.get()) {
-    Observe(nullptr);
-    web_contents_.reset();
+  if (source == tab_->web_contents()) {
+    tab_.reset();
   }
 }
 
@@ -569,15 +540,15 @@ void BlimpEngineSession::ForwardCompositorProto(
     content::RenderWidgetHost* render_widget_host,
     const std::vector<uint8_t>& proto) {
   TRACE_EVENT0("blimp", "BlimpEngineSession::ForwardCompositorProto");
-  render_widget_feature_.SendCompositorMessage(kDummyTabId, render_widget_host,
-                                               proto);
+  render_widget_feature_->SendCompositorMessage(kDummyTabId, render_widget_host,
+                                                proto);
 }
 
 void BlimpEngineSession::NavigationStateChanged(
     content::WebContents* source,
     content::InvalidateTypes changed_flags) {
   TRACE_EVENT0("blimp", "BlimpEngineSession::NavigationStateChanged");
-  if (source != web_contents_.get() || !changed_flags)
+  if (source != tab_->web_contents() || !changed_flags)
     return;
 
   NavigationMessage* navigation_message;
@@ -605,63 +576,20 @@ void BlimpEngineSession::NavigationStateChanged(
                                              net::CompletionCallback());
 }
 
-void BlimpEngineSession::RenderViewCreated(
-    content::RenderViewHost* render_view_host) {
-  render_widget_feature_.OnRenderWidgetCreated(kDummyTabId,
-                                               render_view_host->GetWidget());
-}
-
-void BlimpEngineSession::RenderViewHostChanged(
-    content::RenderViewHost* old_host,
-    content::RenderViewHost* new_host) {
-  // Informs client that WebContents swaps its visible RenderViewHost with
-  // another one.
-  render_widget_feature_.OnRenderWidgetInitialized(kDummyTabId,
-                                                   new_host->GetWidget());
-}
-
-void BlimpEngineSession::RenderViewDeleted(
-    content::RenderViewHost* render_view_host) {
-  render_widget_feature_.OnRenderWidgetDeleted(kDummyTabId,
-                                               render_view_host->GetWidget());
-}
-
-void BlimpEngineSession::SendPageLoadStatusUpdate(PageLoadStatus load_status) {
-  bool page_load_completed = false;
-
-  switch (load_status) {
-    case PageLoadStatus::LOADING:
-      page_load_completed = false;
-      break;
-    case PageLoadStatus::LOADED:
-      page_load_completed = true;
-      break;
-  }
-
-  NavigationMessage* navigation_message = nullptr;
-  std::unique_ptr<BlimpMessage> message =
-      CreateBlimpMessage(&navigation_message, kDummyTabId);
-  navigation_message->set_type(NavigationMessage::NAVIGATION_STATE_CHANGED);
-  NavigationStateChangeMessage* details =
-      navigation_message->mutable_navigation_state_changed();
-  details->set_page_load_completed(page_load_completed);
-
-  navigation_message_sender_->ProcessMessage(std::move(message),
-                                             net::CompletionCallback());
-}
-
 void BlimpEngineSession::PlatformSetContents(

[Kevin M, 2016/06/02 18:38:50]

I don't see any other callers of this besides CreateWebContents. Can we inline
it? This seems to be chopping up the execution flow arbitrarily.

[haibinlu, 2016/06/02 20:24:37]

AddNewContents will use it. But we have not implemented AddNewContents. 

-    std::unique_ptr<content::WebContents> new_contents) {
+    std::unique_ptr<content::WebContents> new_contents,
+    const int target_tab_id) {
   new_contents->SetDelegate(this);
-  Observe(new_contents.get());
-  web_contents_ = std::move(new_contents);
 
-  page_load_tracker_.reset(new PageLoadTracker(web_contents_.get(), this));
   aura::Window* parent = window_tree_host_->window();
-  aura::Window* content = web_contents_->GetNativeView();
+  aura::Window* content = new_contents->GetNativeView();
   if (!parent->Contains(content))

[Kevin M, 2016/06/02 18:38:50]

The WebContents and its view are brand new, is it even possible that the parent
contains it?

[haibinlu, 2016/06/02 20:24:36]

that is the view hierarchy. WindowTreeHost is the root.

     parent->AddChild(content);
   content->Show();
+
+  tab_ = base::WrapUnique(new TabBlimp(std::move(new_contents), target_tab_id,
+                                       render_widget_feature_.get(),
+                                       navigation_message_sender_.get()));
 }
 
 }  // namespace engine