Tab Strip WebUI: Get tab data from TabStripModel and TabRendererData

This CL begins the migration of the calls to get tab data from the
Extensions API to the UI handler such that the data can be supplemented
with additional data that the Extensions API does not provide, such as
TabAlertStates. This CL attempts to keep all data mostly the same, so that
the custom element TabElement can remain unchanged for now, and does not
add any new data that the WebUI Tab Strip does not yet use.

Upcoming CLs will update the Tab data to include more data from the
TabStripModel and TabRendererData and less from the Extensions API.

Bug: 1006946
Change-Id: Icd727416da3ebd1a5118cc49569fb7b649ada4d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1846653Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: John Lee <johntlee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703924}

chrome/browser/resources/tab_strip/tab_list.js

@@ -45,17 +45,6 @@ class TabListElement extends CustomElement {
      */
     this.animationPromises = Promise.resolve();
 
-    /**
-     * Attach and detach callbacks require async requests and therefore may
-     * cause race conditions in which the async requests complete after another
-     * event has been dispatched. Therefore, this object is necessary to keep
-     * track of the recent attached or detached state of each tab to ensure
-     * elements are not created when they should not be. A truthy value
-     * signifies the tab is attached to the current window.
-     * @private {!Object<number, boolean>}
-     */
-    this.attachmentStates_ = {};
-
     /**
      * The TabElement that is currently being dragged.
      * @private {!TabElement|undefined}
@@ -76,17 +65,11 @@ class TabListElement extends CustomElement {
     /** @private {!TabsApiProxy} */
     this.tabsApi_ = TabsApiProxy.getInstance();
 
-    /** @private {!Object} */
-    this.tabsApiHandler_ = this.tabsApi_.callbackRouter;
-
     /** @private {!Element} */
     this.tabsContainerElement_ =
         /** @type {!Element} */ (
             this.shadowRoot.querySelector('#tabsContainer'));
 
-    /** @private {number} */
-    this.windowId_;
-
     addWebUIListener('theme-changed', () => this.fetchAndUpdateColors_());
     this.tabStripViewProxy_.observeThemeChanges();
 
@@ -113,39 +96,17 @@ class TabListElement extends CustomElement {
 
   connectedCallback() {
     this.fetchAndUpdateColors_();
-    this.tabsApi_.getCurrentWindow().then((currentWindow) => {
-      this.windowId_ = currentWindow.id;
-
-      // TODO(johntlee): currentWindow.tabs is guaranteed to be defined because
-      // `populate: true` is passed in as part of the arguments to the API.
-      // Once the closure compiler is able to type `assert` to return a truthy
-      // type even when being used with modules, the conditionals should be
-      // replaced with `assert` (b/138729777).
-      if (currentWindow.tabs) {
-        for (const tab of currentWindow.tabs) {
-          if (tab) {
-            this.onTabCreated_(tab);
-          }
-        }
-
-        this.moveOrScrollToActiveTab_();
-      }
+    this.tabsApi_.getTabs().then(tabs => {
+      tabs.forEach(tab => this.onTabCreated_(tab));
+      this.moveOrScrollToActiveTab_();
 
-      this.tabsApiHandler_.onAttached.addListener(
-          (tabId, attachedInfo) => this.onTabAttached_(tabId, attachedInfo));
-      this.tabsApiHandler_.onActivated.addListener(
-          (activeInfo) => this.onTabActivated_(activeInfo));
-      this.tabsApiHandler_.onCreated.addListener(
-          (tab) => this.onTabCreated_(tab));
-      this.tabsApiHandler_.onDetached.addListener(
-          (tabId, detachInfo) => this.onTabDetached_(tabId, detachInfo));
-      this.tabsApiHandler_.onMoved.addListener(
-          (tabId, moveInfo) => this.onTabMoved_(tabId, moveInfo));
-      this.tabsApiHandler_.onRemoved.addListener(
-          (tabId, removeInfo) => this.onTabRemoved_(tabId, removeInfo));
-      this.tabsApiHandler_.onUpdated.addListener(
-          (tabId, changeInfo, tab) =>
-              this.onTabUpdated_(tabId, changeInfo, tab));
+      addWebUIListener('tab-created', tab => this.onTabCreated_(tab));
+      addWebUIListener(
+          'tab-moved', (tabId, newIndex) => this.onTabMoved_(tabId, newIndex));
+      addWebUIListener('tab-removed', tabId => this.onTabRemoved_(tabId));
+      addWebUIListener('tab-updated', tab => this.onTabUpdated_(tab));
+      addWebUIListener(
+          'tab-active-changed', tabId => this.onTabActivated_(tabId));
     });
   }
 
@@ -281,21 +242,17 @@ class TabListElement extends CustomElement {
   }
 
   /**
-   * @param {!TabActivatedInfo} activeInfo
+   * @param {number} tabId
    * @private
    */
-  onTabActivated_(activeInfo) {
-    if (activeInfo.windowId !== this.windowId_) {
-      return;
-    }
-
+  onTabActivated_(tabId) {
     const previouslyActiveTab = this.getActiveTab_();
     if (previouslyActiveTab) {
       previouslyActiveTab.tab = /** @type {!Tab} */ (
           Object.assign({}, previouslyActiveTab.tab, {active: false}));
     }
 
-    const newlyActiveTab = this.findTabElement_(activeInfo.tabId);
+    const newlyActiveTab = this.findTabElement_(tabId);
     if (newlyActiveTab) {
       newlyActiveTab.tab = /** @type {!Tab} */ (
           Object.assign({}, newlyActiveTab.tab, {active: true}));
@@ -303,33 +260,11 @@ class TabListElement extends CustomElement {
     }
   }
 
-  /**
-   * @param {number} tabId
-   * @param {!TabAttachedInfo} attachInfo
-   * @private
-   */
-  async onTabAttached_(tabId, attachInfo) {
-    if (attachInfo.newWindowId !== this.windowId_) {
-      return;
-    }
-
-    this.attachmentStates_[tabId] = true;
-    const tab = await this.tabsApi_.getTab(tabId);
-    if (this.attachmentStates_[tabId] && !this.findTabElement_(tabId)) {
-      const tabElement = this.createTabElement_(tab);
-      this.insertTabOrMoveTo_(tabElement, attachInfo.newPosition);
-    }
-  }
-
   /**
    * @param {!Tab} tab
    * @private
    */
   onTabCreated_(tab) {
-    if (tab.windowId !== this.windowId_) {
-      return;
-    }
-
     const tabElement = this.createTabElement_(tab);
 
     if (tab.active && !tab.pinned &&
@@ -351,34 +286,13 @@ class TabListElement extends CustomElement {
 
   /**
    * @param {number} tabId
-   * @param {!TabDetachedInfo} detachInfo
+   * @param {number} newIndex
    * @private
    */
-  onTabDetached_(tabId, detachInfo) {
-    if (detachInfo.oldWindowId !== this.windowId_) {
-      return;
-    }
-
-    this.attachmentStates_[tabId] = false;
-    const tabElement = this.findTabElement_(tabId);
-    if (tabElement) {
-      tabElement.remove();
-    }
-  }
-
-  /**
-   * @param {number} tabId
-   * @param {!TabMovedInfo} moveInfo
-   * @private
-   */
-  onTabMoved_(tabId, moveInfo) {
-    if (moveInfo.windowId !== this.windowId_) {
-      return;
-    }
-
+  onTabMoved_(tabId, newIndex) {
     const movedTab = this.findTabElement_(tabId);
     if (movedTab) {
-      this.insertTabOrMoveTo_(movedTab, moveInfo.toIndex);
+      this.insertTabOrMoveTo_(movedTab, newIndex);
       if (movedTab.tab.active) {
         this.scrollToTab_(movedTab);
       }
@@ -387,54 +301,34 @@ class TabListElement extends CustomElement {
 
   /**
    * @param {number} tabId
-   * @param {!WindowRemoveInfo} removeInfo
    * @private
    */
-  onTabRemoved_(tabId, removeInfo) {
-    if (removeInfo.windowId !== this.windowId_) {
-      return;
-    }
-
+  onTabRemoved_(tabId) {
     const tabElement = this.findTabElement_(tabId);
     if (tabElement) {
-      this.addAnimationPromise_(new Promise(async resolve => {
-        await tabElement.slideOut();
-        resolve();
-      }));
+      this.addAnimationPromise_(tabElement.slideOut());
     }
   }
 
   /**
-   * @param {number} tabId
-   * @param {!Tab} changeInfo
    * @param {!Tab} tab
    * @private
    */
-  onTabUpdated_(tabId, changeInfo, tab) {
-    if (tab.windowId !== this.windowId_) {
+  onTabUpdated_(tab) {
+    const tabElement = this.findTabElement_(tab.id);
+    if (!tabElement) {
       return;
     }
 
-    const tabElement = this.findTabElement_(tabId);
-    if (tabElement) {
-      // While a tab may go in and out of a loading state, the Extensions API
-      // only dispatches |onUpdated| events up until the first time a tab
-      // reaches a non-loading state. Therefore, the UI should ignore any
-      // updates to a |status| of a tab unless the API specifically has
-      // dispatched an event indicating the status has changed.
-      if (!changeInfo.status) {
-        tab.status = tabElement.tab.status;
-      }
-
-      tabElement.tab = tab;
+    const previousTab = tabElement.tab;
+    tabElement.tab = tab;
 
-      if (changeInfo.pinned !== undefined) {
-        // If the tab is being pinned or unpinned, we need to move it to its new
-        // location
-        this.insertTabOrMoveTo_(tabElement, tab.index);
-        if (tab.active) {
-          this.scrollToTab_(tabElement);
-        }
+    if (previousTab.pinned !== tab.pinned) {
+      // If the tab is being pinned or unpinned, we need to move it to its new
+      // location
+      this.insertTabOrMoveTo_(tabElement, tab.index);
+      if (tab.active) {
+        this.scrollToTab_(tabElement);
       }
     }
   }

chrome/browser/resources/tab_strip/tabs_api_proxy.js

@@ -2,22 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import {addSingletonGetter} from 'chrome://resources/js/cr.m.js';
+import {addSingletonGetter, sendWithPromise} from 'chrome://resources/js/cr.m.js';
 
 export class TabsApiProxy {
-  constructor() {
-    /** @type {!Object<string, !ChromeEvent>} */
-    this.callbackRouter = {
-      onActivated: chrome.tabs.onActivated,
-      onAttached: chrome.tabs.onAttached,
-      onCreated: chrome.tabs.onCreated,
-      onDetached: chrome.tabs.onDetached,
-      onMoved: chrome.tabs.onMoved,
-      onRemoved: chrome.tabs.onRemoved,
-      onUpdated: chrome.tabs.onUpdated,
-    };
-  }
-
   /**
    * @param {number} tabId
    * @return {!Promise<!Tab>}
@@ -29,26 +16,10 @@ export class TabsApiProxy {
   }
 
   /**
-   * @return {!Promise<!ChromeWindow>}
-   */
-  getCurrentWindow() {
-    const options = {
-      populate: true,           // populate window data with tabs data
-      windowTypes: ['normal'],  // prevent devtools from being returned
-    };
-    return new Promise(resolve => {
-      chrome.windows.getCurrent(options, currentWindow => {
-        resolve(currentWindow);
-      });
-    });
-  }
-
-  /**
-   * @param {number} tabId
-   * @return {!Promise<!Tab>}
+   * @return {!Promise<!Array<!Tab>>}
    */
-  getTab(tabId) {
-    return new Promise(resolve => chrome.tabs.get(tabId, resolve));
+  getTabs() {
+    return sendWithPromise('getTabs');
   }
 
   /**

chrome/browser/ui/webui/tab_strip/tab_strip_ui.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/base64.h"
+#include "base/bind.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -17,6 +18,8 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_network_state.h"
+#include "chrome/browser/ui/tabs/tab_renderer_data.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
@@ -25,6 +28,8 @@
 #include "chrome/grit/tab_strip_resources.h"
 #include "chrome/grit/tab_strip_resources_map.h"
 #include "components/favicon_base/favicon_url_parser.h"
+#include "content/public/browser/favicon_status.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/url_data_source.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "content/public/browser/web_ui_message_handler.h"
@@ -66,22 +71,82 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
   explicit TabStripUIHandler(Browser* browser)
       : browser_(browser),
         thumbnail_tracker_(base::Bind(&TabStripUIHandler::HandleThumbnailUpdate,
-                                      base::Unretained(this))) {
+                                      base::Unretained(this))) {}
+  ~TabStripUIHandler() override = default;
+
+  void OnJavascriptAllowed() override {
     browser_->tab_strip_model()->AddObserver(this);
   }
-  ~TabStripUIHandler() override = default;
 
   // TabStripModelObserver:
+  void OnTabStripModelChanged(
+      TabStripModel* tab_strip_model,
+      const TabStripModelChange& change,
+      const TabStripSelectionChange& selection) override {
+    if (tab_strip_model->empty())
+      return;
+
+    switch (change.type()) {
+      case TabStripModelChange::kInserted: {
+        for (const auto& contents : change.GetInsert()->contents) {
+          FireWebUIListener("tab-created",
+                            GetTabData(contents.contents, contents.index));
+        }
+        break;
+      }
+      case TabStripModelChange::kRemoved: {
+        for (const auto& contents : change.GetRemove()->contents) {
+          FireWebUIListener("tab-removed",
+                            base::Value(extensions::ExtensionTabUtil::GetTabId(
+                                contents.contents)));
+        }
+        break;
+      }
+      case TabStripModelChange::kMoved: {
+        auto* move = change.GetMove();
+        FireWebUIListener(
+            "tab-moved",
+            base::Value(extensions::ExtensionTabUtil::GetTabId(move->contents)),
+            base::Value(move->to_index));
+        break;
+      }
+
+      case TabStripModelChange::kReplaced:
+      case TabStripModelChange::kGroupChanged:
+      case TabStripModelChange::kSelectionOnly:
+        // Not yet implemented.
+        break;
+    }
+
+    if (selection.active_tab_changed()) {
+      content::WebContents* new_contents = selection.new_contents;
+      int index = selection.new_model.active();
+      if (new_contents && index != TabStripModel::kNoTab) {
+        FireWebUIListener(
+            "tab-active-changed",
+            base::Value(extensions::ExtensionTabUtil::GetTabId(new_contents)));
+      }
+    }
+  }
+
   void TabChangedAt(content::WebContents* contents,
                     int index,
                     TabChangeType change_type) override {
-    // TODO(crbug.com/1006946): re-fetch the TabRendererData using
-    // |TabRendererData::FromTabInModel()|.
+    FireWebUIListener("tab-updated", GetTabData(contents, index));
+  }
+
+  void TabPinnedStateChanged(TabStripModel* tab_strip_model,
+                             content::WebContents* contents,
+                             int index) override {
+    FireWebUIListener("tab-updated", GetTabData(contents, index));
   }
 
  protected:
   // content::WebUIMessageHandler:
   void RegisterMessages() override {
+    web_ui()->RegisterMessageCallback(
+        "getTabs",
+        base::Bind(&TabStripUIHandler::HandleGetTabs, base::Unretained(this)));
     web_ui()->RegisterMessageCallback(
         "getThemeColors", base::Bind(&TabStripUIHandler::HandleGetThemeColors,
                                      base::Unretained(this)));
@@ -94,6 +159,45 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
   }
 
  private:
+  base::DictionaryValue GetTabData(content::WebContents* contents, int index) {
+    base::DictionaryValue tab_data;
+
+    tab_data.SetBoolean("active",
+                        browser_->tab_strip_model()->active_index() == index);
+    tab_data.SetInteger("id", extensions::ExtensionTabUtil::GetTabId(contents));
+    tab_data.SetInteger("index", index);
+    tab_data.SetString("status", extensions::ExtensionTabUtil::GetTabStatusText(
+                                     contents->IsLoading()));
+
+    // TODO(johntlee): Replace with favicon from TabRendererData
+    content::NavigationEntry* visible_entry =
+        contents->GetController().GetVisibleEntry();
+    if (visible_entry && visible_entry->GetFavicon().valid) {
+      tab_data.SetString("favIconUrl", visible_entry->GetFavicon().url.spec());
+    }
+
+    TabRendererData tab_renderer_data =
+        TabRendererData::FromTabInModel(browser_->tab_strip_model(), index);
+    tab_data.SetBoolean("pinned", tab_renderer_data.pinned);
+    tab_data.SetString("title", tab_renderer_data.title);
+    tab_data.SetString("url", tab_renderer_data.visible_url.GetContent());
+    // TODO(johntlee): Add the rest of TabRendererData
+
+    return tab_data;
+  }
+
+  void HandleGetTabs(const base::ListValue* args) {
+    AllowJavascript();
+    const base::Value& callback_id = args->GetList()[0];
+
+    base::ListValue tabs;
+    TabStripModel* tab_strip_model = browser_->tab_strip_model();
+    for (int i = 0; i < tab_strip_model->count(); ++i) {
+      tabs.Append(GetTabData(tab_strip_model->GetWebContentsAt(i), i));
+    }
+    ResolveJavascriptCallback(callback_id, tabs);
+  }
+
   void HandleGetThemeColors(const base::ListValue* args) {
     AllowJavascript();
     const base::Value& callback_id = args->GetList()[0];

chrome/test/data/webui/tab_strip/test_tabs_api_proxy.js

@@ -4,45 +4,17 @@
 
 import {TestBrowserProxy} from 'chrome://test/test_browser_proxy.m.js';
 
-class EventDispatcher {
-  constructor() {
-    this.eventListeners_ = [];
-  }
-
-  addListener(callback) {
-    this.eventListeners_.push(callback);
-  }
-
-  dispatchEvent() {
-    this.eventListeners_.forEach((callback) => {
-      callback(...arguments);
-    });
-  }
-}
-
 export class TestTabsApiProxy extends TestBrowserProxy {
   constructor() {
     super([
       'activateTab',
       'closeTab',
-      'getCurrentWindow',
-      'getTab',
+      'getTabs',
       'moveTab',
       'trackThumbnailForTab',
     ]);
 
-    this.callbackRouter = {
-      onActivated: new EventDispatcher(),
-      onAttached: new EventDispatcher(),
-      onCreated: new EventDispatcher(),
-      onDetached: new EventDispatcher(),
-      onMoved: new EventDispatcher(),
-      onRemoved: new EventDispatcher(),
-      onUpdated: new EventDispatcher(),
-    };
-
-    this.currentWindow_;
-    this.tab_;
+    this.tabs_;
   }
 
   activateTab(tabId) {
@@ -55,14 +27,9 @@ export class TestTabsApiProxy extends TestBrowserProxy {
     return Promise.resolve();
   }
 
-  getCurrentWindow() {
-    this.methodCalled('getCurrentWindow');
-    return Promise.resolve(this.currentWindow_);
-  }
-
-  getTab(tabId) {
-    this.methodCalled('getTab', tabId);
-    return Promise.resolve(this.tab_);
+  getTabs() {
+    this.methodCalled('getTabs');
+    return Promise.resolve(this.tabs_.slice());
   }
 
   moveTab(tabId, newIndex) {
@@ -70,12 +37,8 @@ export class TestTabsApiProxy extends TestBrowserProxy {
     return Promise.resolve();
   }
 
-  setCurrentWindow(currentWindow) {
-    this.currentWindow_ = currentWindow;
-  }
-
-  setTab(tab) {
-    this.tab_ = tab;
+  setTabs(tabs) {
+    this.tabs_ = tabs;
   }
 
   trackThumbnailForTab(tabId) {