Tab Strip WebUI: Update elements on tab attached/detached

Bug: 989131
Change-Id: Ia551767693a7ebcda664bb6bff5cf6755379a02c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807655
Commit-Queue: John Lee <johntlee@chromium.org>
Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698584}

chrome/browser/resources/tab_strip/tab_list.js

@@ -45,6 +45,17 @@ 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}
@@ -114,12 +125,21 @@ class TabListElement extends CustomElement {
         }
       }
 
+      this.tabsApiHandler_.onAttached.addListener(
+          (tabId, attachedInfo) => this.onTabAttached_(tabId, attachedInfo));
       this.tabsApiHandler_.onActivated.addListener(
-          this.onTabActivated_.bind(this));
-      this.tabsApiHandler_.onCreated.addListener(this.onTabCreated_.bind(this));
-      this.tabsApiHandler_.onMoved.addListener(this.onTabMoved_.bind(this));
-      this.tabsApiHandler_.onRemoved.addListener(this.onTabRemoved_.bind(this));
-      this.tabsApiHandler_.onUpdated.addListener(this.onTabUpdated_.bind(this));
+          (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));
     });
   }
 
@@ -266,6 +286,24 @@ 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
@@ -280,6 +318,23 @@ class TabListElement extends CustomElement {
     this.addAnimationPromise_(tabElement.slideIn());
   }
 
+  /**
+   * @param {number} tabId
+   * @param {!TabDetachedInfo} detachInfo
+   * @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

chrome/browser/resources/tab_strip/tabs_api_proxy.js

@@ -9,7 +9,9 @@ export class TabsApiProxy {
     /** @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,
@@ -41,6 +43,14 @@ export class TabsApiProxy {
     });
   }
 
+  /**
+   * @param {number} tabId
+   * @return {!Promise<!Tab>}
+   */
+  getTab(tabId) {
+    return new Promise(resolve => chrome.tabs.get(tabId, resolve));
+  }
+
   /**
    * @param {number} tabId
    * @return {!Promise}

chrome/browser/resources/tab_strip/types.js

@@ -14,6 +14,22 @@
  */
 let TabActivatedInfo;
 
+/**
+ * @typedef {{
+ *    newPosition: number,
+ *    newWindowId: number,
+ * }}
+ */
+let TabAttachedInfo;
+
+/**
+ * @typedef {{
+ *    oldPosition: number,
+ *    oldWindowId: number,
+ * }}
+ */
+let TabDetachedInfo;
+
 /**
  * @typedef {{
  *    fromIndex: number,

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

@@ -332,6 +332,65 @@ suite('TabList', () => {
     assertEquals(fakeScroller.scrollLeft, activeTab.offsetLeft - scrollPadding);
   });
 
+  test('attaching a tab creates a new tab element', async () => {
+    const attachedTab = {
+      index: 2,
+      id: 9001,
+      title: 'My new tab',
+      windowId: currentWindow.id,
+    };
+    testTabsApiProxy.setTab(attachedTab);
+    callbackRouter.onAttached.dispatchEvent(attachedTab.id, {
+      newPosition: attachedTab.index,
+      newWindowId: attachedTab.windowId,
+    });
+
+    const tabId = await testTabsApiProxy.whenCalled('getTab');
+    assertEquals(tabId, attachedTab.id);
+    assertEquals(getUnpinnedTabs().length, currentWindow.tabs.length + 1);
+    assertEquals(getUnpinnedTabs()[attachedTab.index].tab, attachedTab);
+  });
+
+  test('detaching a tab removes the tab element', () => {
+    const detachedTab = currentWindow.tabs[1];
+    callbackRouter.onDetached.dispatchEvent(detachedTab.id, {
+      oldPosition: 1,
+      oldWindowId: currentWindow.id,
+    });
+    assertEquals(getUnpinnedTabs().length, currentWindow.tabs.length - 1);
+  });
+
+  test(
+      'respects the last attached/detached event when multiple events are ' +
+          'dispatched for the same tab',
+      async () => {
+        const attachedTab = {
+          index: 2,
+          id: 9001,
+          title: 'My new tab',
+          windowId: currentWindow.id,
+        };
+        testTabsApiProxy.setTab(attachedTab);
+        callbackRouter.onAttached.dispatchEvent(attachedTab.id, {
+          newPosition: attachedTab.index,
+          newWindowId: attachedTab.windowId,
+        });
+        callbackRouter.onDetached.dispatchEvent(attachedTab.id, {
+          oldPosition: attachedTab.index,
+          oldWindowId: attachedTab.windowId,
+        });
+        callbackRouter.onAttached.dispatchEvent(attachedTab.id, {
+          newPosition: attachedTab.index,
+          newWindowId: attachedTab.windowId,
+        });
+        callbackRouter.onDetached.dispatchEvent(attachedTab.id, {
+          oldPosition: attachedTab.index,
+          oldWindowId: attachedTab.windowId,
+        });
+        await testTabsApiProxy.whenCalled('getTab');
+        assertEquals(getUnpinnedTabs().length, currentWindow.tabs.length);
+      });
+
   test('dragstart sets a drag image offset by the event coordinates', () => {
     const draggedTab = getUnpinnedTabs()[0];
     const mockDataTransfer = new MockDataTransfer();

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

@@ -26,19 +26,23 @@ export class TestTabsApiProxy extends TestBrowserProxy {
       'activateTab',
       'closeTab',
       'getCurrentWindow',
+      'getTab',
       '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_;
   }
 
   activateTab(tabId) {
@@ -56,6 +60,11 @@ export class TestTabsApiProxy extends TestBrowserProxy {
     return Promise.resolve(this.currentWindow_);
   }
 
+  getTab(tabId) {
+    this.methodCalled('getTab', tabId);
+    return Promise.resolve(this.tab_);
+  }
+
   moveTab(tabId, newIndex) {
     this.methodCalled('moveTab', [tabId, newIndex]);
     return Promise.resolve();
@@ -65,6 +74,10 @@ export class TestTabsApiProxy extends TestBrowserProxy {
     this.currentWindow_ = currentWindow;
   }
 
+  setTab(tab) {
+    this.tab_ = tab;
+  }
+
   trackThumbnailForTab(tabId) {
     this.methodCalled('trackThumbnailForTab', tabId);
   }