[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..7b325cc4591a2c02fb65d03b545945d7365fbd2e 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"
@@ -233,11 +234,9 @@ BlimpEngineSession::~BlimpEngineSession() {
 
   window_tree_host_->GetInputMethod()->RemoveObserver(this);
 
-  page_load_tracker_.reset();
-
-  // Ensure that all WebContents are torn down first, since teardown will
+  // Ensure that all tabs are torn down first, since teardown will

[Kevin M, 2016/06/06 21:44:38]

Not needed; placing the tab at the end of the field list ensures that it will be
torn down first.

[haibinlu, 2016/06/06 22:32:28]

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,
@@ -319,10 +318,10 @@ void BlimpEngineSession::RegisterFeatures() {
                                         &settings_feature_);
 }
 
-bool BlimpEngineSession::CreateWebContents(const int target_tab_id) {
+bool BlimpEngineSession::CreateTab(const int target_tab_id) {
   DVLOG(1) << "Create tab " << target_tab_id;
-  // TODO(haibinlu): Support more than one active WebContents (crbug/547231).
-  if (web_contents_) {
+  // TODO(haibinlu): Support more than one tab (crbug/547231).

[Kevin M, 2016/06/06 21:44:38]

We specify this issue multiple times, can we just use the .h comment for that?

[haibinlu, 2016/06/06 22:32:27]

Done.

+  if (tab_) {
     DLOG(WARNING) << "Tab " << target_tab_id << " already existed";

[Kevin M, 2016/06/06 21:44:38]

This warning doesn't apply - the issue is the pre-existence of *any* tab.

[haibinlu, 2016/06/06 22:32:27]

Done.

     return false;
   }
@@ -331,19 +330,14 @@ 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) {
+void BlimpEngineSession::CloseTab(const int target_tab_id) {
   DVLOG(1) << "Close tab " << target_tab_id;
-  DCHECK(web_contents_);
-  web_contents_->Close();
+  tab_.reset();
 }
 
 void BlimpEngineSession::HandleResize(float device_pixel_ratio,
@@ -351,49 +345,30 @@ 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();
+  if (tab_)
+    tab_->LoadUrl(url);

[Kevin M, 2016/06/06 21:44:38]

Why are we silently failing here? This is invalid input; we should drop the
connection and log an error code. Ditto for this and other navigation commands

[haibinlu, 2016/06/06 22:32:27]

log a info instead.

I argue we should not drop the connection. Due to the latency, it is possible
that when load-url command from client arrives, a tab has been closed by the
engine. 

 }
 
 void BlimpEngineSession::GoBack(const int target_tab_id) {
-  DVLOG(1) << "Back in tab " << target_tab_id;
-  web_contents_->GetController().GoBack();
+  if (tab_)
+    tab_->GoBack();
 }
 
 void BlimpEngineSession::GoForward(const int target_tab_id) {
-  DVLOG(1) << "Forward in tab " << target_tab_id;
-  web_contents_->GetController().GoForward();
+  if (tab_)
+    tab_->GoForward();
 }
 
 void BlimpEngineSession::Reload(const int target_tab_id) {
-  DVLOG(1) << "Reload in tab " << target_tab_id;
-  web_contents_->GetController().Reload(true);
+  if (tab_)
+    tab_->Reload();
 }
 
 void BlimpEngineSession::OnWebGestureEvent(
@@ -427,7 +402,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 =
@@ -440,7 +415,7 @@ void BlimpEngineSession::OnTextInputStateChanged(
   if (type == ui::TEXT_INPUT_TYPE_NONE)
     render_widget_feature_.SendHideImeRequest(
         kDummyTabId,
-        web_contents_->GetRenderWidgetHostView()->GetRenderWidgetHost());
+        tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost());
 }
 
 void BlimpEngineSession::OnInputMethodDestroyed(
@@ -449,13 +424,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_ || !tab_->web_contents()->GetRenderWidgetHostView() ||
       !window_tree_host_->GetInputMethod()->GetTextInputClient())
     return;
 
   render_widget_feature_.SendShowImeRequest(
       kDummyTabId,
-      web_contents_->GetRenderWidgetHostView()->GetRenderWidgetHost(),
+      tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost(),
       window_tree_host_->GetInputMethod()->GetTextInputClient());
 }
 
@@ -472,11 +447,11 @@ void BlimpEngineSession::ProcessMessage(
   if (message->has_tab_control()) {
     switch (message->tab_control().tab_control_case()) {
       case TabControlMessage::kCreateTab:
-        if (!CreateWebContents(message->target_tab_id()))
+        if (!CreateTab(message->target_tab_id()))
           result = net::ERR_FAILED;
         break;
       case TabControlMessage::kCloseTab:
-        CloseWebContents(message->target_tab_id());
+        CloseTab(message->target_tab_id());
       case TabControlMessage::kSize:
         HandleResize(message->tab_control().size().device_pixel_ratio(),
                      gfx::Size(message->tab_control().size().width(),
@@ -486,7 +461,7 @@ void BlimpEngineSession::ProcessMessage(
         NOTIMPLEMENTED();
         result = net::ERR_NOT_IMPLEMENTED;
     }
-  } else if (message->has_navigation() && web_contents_) {
+  } else if (message->has_navigation()) {

[Kevin M, 2016/06/06 21:44:38]

Check if the tab_ exists here?

[haibinlu, 2016/06/06 22:32:27]

Done.

     switch (message->navigation().type()) {
       case NavigationMessage::LOAD_URL:
         LoadUrl(message->target_tab_id(),
@@ -506,7 +481,7 @@ void BlimpEngineSession::ProcessMessage(
         result = net::ERR_NOT_IMPLEMENTED;
     }
   } else {
-    DVLOG(1) << "No WebContents for navigation control";
+    DVLOG(1) << "No tab for navigation control";

[Kevin M, 2016/06/06 21:44:38]

This error seems N/A for the current logic

[haibinlu, 2016/06/06 22:32:27]

Done.

     result = net::ERR_FAILED;
   }
 
@@ -555,9 +530,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();
   }
 }
 
@@ -577,91 +551,24 @@ void BlimpEngineSession::NavigationStateChanged(
     content::WebContents* source,
     content::InvalidateTypes changed_flags) {
   TRACE_EVENT0("blimp", "BlimpEngineSession::NavigationStateChanged");
-  if (source != web_contents_.get() || !changed_flags)
-    return;
-
-  NavigationMessage* navigation_message;
-  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();
-
-  if (changed_flags & content::InvalidateTypes::INVALIDATE_TYPE_URL)
-    details->set_url(source->GetURL().spec());
-
-  if (changed_flags & content::InvalidateTypes::INVALIDATE_TYPE_TAB) {
-    // TODO(dtrainor): Serialize the favicon? crbug.com/597094.
-    DVLOG(3) << "Tab favicon changed";
-  }
-
-  if (changed_flags & content::InvalidateTypes::INVALIDATE_TYPE_TITLE)
-    details->set_title(base::UTF16ToUTF8(source->GetTitle()));
-
-  if (changed_flags & content::InvalidateTypes::INVALIDATE_TYPE_LOAD)
-    details->set_loading(source->IsLoading());
-
-  navigation_message_sender_->ProcessMessage(std::move(message),
-                                             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());
+  if (source == tab_->web_contents())
+    tab_->NavigationStateChanged(changed_flags);
 }
 
 void BlimpEngineSession::PlatformSetContents(

[Kevin M, 2016/06/06 21:44:38]

What's the reason for having this be separate from CreateTab() ?

[haibinlu, 2016/06/06 22:32:27]

When BlimpEngineSession::AddNewContents is implemented, it will not create a new
webcontents (just use new_contents) but will invoke this method to set it up.

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