History Web UI: Remove setForeignSessions global function

- Remove setForeignSessions global function
- Replace chrome.send call in history.js with starting foreign session
fetch in C++ when the UI is constructed
- Convert getForeignSessions to use sendWithPromise
- Send sessions to JS on receiving getForeignSessions Web UI message

Bug: 1022212
Change-Id: Ia4613335a4cd3d0c8090f030cc0206661f81886d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1949773Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722702}

chrome/browser/resources/history/app.js

@@ -163,6 +163,9 @@ Polymer({
   /** @private {?function(!Event)} */
   boundOnKeyDown_: null,
 
+  /** @private {?history.BrowserService} */
+  browserService_: null,
+
   /** @override */
   created: function() {
     history.listenForPrivilegedLinkClicks();
@@ -178,7 +181,13 @@ Polymer({
     this.addWebUIListener(
         'has-other-forms-changed',
         hasOtherForms => this.onHasOtherFormsChanged_(hasOtherForms));
-    history.BrowserService.getInstance().historyLoaded();
+    this.addWebUIListener(
+        'foreign-sessions-changed',
+        sessionList => this.setForeignSessions_(sessionList));
+    this.browserService_ = history.BrowserService.getInstance();
+    this.browserService_.historyLoaded();
+    this.browserService_.getForeignSessions().then(
+        sessionList => this.setForeignSessions_(sessionList));
   },
 
   /** @override */
@@ -188,8 +197,8 @@ Polymer({
   },
 
   onFirstRender: function() {
-    setTimeout(function() {
-      history.BrowserService.getInstance().recordTime(
+    setTimeout(() => {
+      this.browserService_.recordTime(
           'History.ResultsRenderedTime', window.performance.now());
     });
 
@@ -223,7 +232,7 @@ Polymer({
 
   /** @private */
   onCrToolbarMenuPromoShown_: function() {
-    history.BrowserService.getInstance().menuPromoShown();
+    this.browserService_.menuPromoShown();
   },
 
   /** @private */
@@ -322,8 +331,9 @@ Polymer({
   /**
    * @param {!Array<!ForeignSession>} sessionList Array of objects describing
    *     the sessions from other devices.
+   * @private
    */
-  setForeignSessions: function(sessionList) {
+  setForeignSessions_: function(sessionList) {
     this.set('queryResult_.sessionList', sessionList);
   },
 
@@ -427,7 +437,7 @@ Polymer({
         break;
     }
 
-    history.BrowserService.getInstance().recordHistogram(
+    this.browserService_.recordHistogram(
         'History.HistoryPageView', histogramValue,
         HistoryPageViewHistogram.END);
   },

chrome/browser/resources/history/browser_service.js

@@ -13,6 +13,11 @@ cr.define('history', function() {
       chrome.send('historyLoaded');
     }
 
+    /** @return {!Promise<!Array<!ForeignSession>>} */
+    getForeignSessions() {
+      return cr.sendWithPromise('getForeignSessions');
+    }
+
     /**
      * @param {!string} url
      */

chrome/browser/resources/history/history.js

@@ -5,7 +5,6 @@
 // Send the history query immediately. This allows the query to process during
 // the initial page startup.
 chrome.send('queryHistory', ['', RESULTS_PER_PAGE]);
-chrome.send('getForeignSessions');
 
 /** @type {Promise} */
 let upgradePromise = null;
@@ -47,17 +46,3 @@ function historyResult(info, results) {
     }
   });
 }
-
-/**
- * Receives the synced history data. An empty list means that either there are
- * no foreign sessions, or tab sync is disabled for this profile.
- *
- * @param {!Array<!ForeignSession>} sessionList Array of objects describing the
- *     sessions from other devices.
- */
-function setForeignSessions(sessionList) {
-  waitForAppUpgrade().then(function() {
-    /** @type {HistoryAppElement} */ ($('history-app'))
-        .setForeignSessions(sessionList);
-  });
-}

chrome/browser/ui/webui/history/foreign_session_handler.cc

@@ -223,21 +223,6 @@ sync_sessions::OpenTabsUIDelegate* ForeignSessionHandler::GetOpenTabsUIDelegate(
 }
 
 void ForeignSessionHandler::RegisterMessages() {
-  Profile* profile = Profile::FromWebUI(web_ui());
-
-  sync_sessions::SessionSyncService* service =
-      SessionSyncServiceFactory::GetInstance()->GetForProfile(profile);
-
-  // NOTE: The SessionSyncService can be null in tests.
-  if (service) {
-    // base::Unretained() is safe below because the subscription itself is a
-    // class member field and handles destruction well.
-    foreign_session_updated_subscription_ =
-        service->SubscribeToForeignSessionsChanged(
-            base::BindRepeating(&ForeignSessionHandler::OnForeignSessionUpdated,
-                                base::Unretained(this)));
-  }
-
   web_ui()->RegisterMessageCallback(
       "deleteForeignSession",
       base::BindRepeating(&ForeignSessionHandler::HandleDeleteForeignSession,
@@ -257,8 +242,34 @@ void ForeignSessionHandler::RegisterMessages() {
           base::Unretained(this)));
 }
 
+void ForeignSessionHandler::OnJavascriptAllowed() {
+  // This can happen if the page is refreshed.
+  if (initial_session_list_.is_none())
+    initial_session_list_ = GetForeignSessions();
+
+  Profile* profile = Profile::FromWebUI(web_ui());
+
+  sync_sessions::SessionSyncService* service =
+      SessionSyncServiceFactory::GetInstance()->GetForProfile(profile);
+
+  // NOTE: The SessionSyncService can be null in tests.
+  if (service) {
+    // base::Unretained() is safe below because the subscription itself is a
+    // class member field and handles destruction well.
+    foreign_session_updated_subscription_ =
+        service->SubscribeToForeignSessionsChanged(
+            base::BindRepeating(&ForeignSessionHandler::OnForeignSessionUpdated,
+                                base::Unretained(this)));
+  }
+}
+
 void ForeignSessionHandler::OnForeignSessionUpdated() {
-  HandleGetForeignSessions(nullptr);
+  FireWebUIListener("foreign-sessions-changed",
+                    std::move(GetForeignSessions()));
+}
+
+void ForeignSessionHandler::InitializeForeignSessions() {
+  initial_session_list_ = GetForeignSessions();
 }
 
 base::string16 ForeignSessionHandler::FormatSessionTime(
@@ -272,7 +283,18 @@ base::string16 ForeignSessionHandler::FormatSessionTime(
 }
 
 void ForeignSessionHandler::HandleGetForeignSessions(
-    const base::ListValue* /*args*/) {
+    const base::ListValue* args) {
+  AllowJavascript();
+  CHECK(!initial_session_list_.is_none());
+  const base::Value& callback_id = args->GetList()[0];
+  ResolveJavascriptCallback(callback_id, std::move(initial_session_list_));
+
+  // Clear the initial list so that it will be reset in AllowJavascript if the
+  // page is refreshed.
+  initial_session_list_ = base::Value(base::Value::Type::NONE);
+}
+
+base::Value ForeignSessionHandler::GetForeignSessions() {
   sync_sessions::OpenTabsUIDelegate* open_tabs =
       GetOpenTabsUIDelegate(web_ui());
   std::vector<const sync_sessions::SyncedSession*> sessions;
@@ -332,8 +354,7 @@ void ForeignSessionHandler::HandleGetForeignSessions(
       session_list.GetList().push_back(std::move(session_data));
     }
   }
-  web_ui()->CallJavascriptFunctionUnsafe("setForeignSessions",
-                                         std::move(session_list));
+  return session_list;
 }
 
 void ForeignSessionHandler::HandleOpenForeignSession(

chrome/browser/ui/webui/history/foreign_session_handler.h

@@ -12,6 +12,7 @@
 #include "base/callback_list.h"
 #include "base/macros.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "content/public/browser/web_ui.h"
@@ -46,10 +47,13 @@ class ForeignSessionHandler : public content::WebUIMessageHandler {
  public:
   // WebUIMessageHandler implementation.
   void RegisterMessages() override;
+  void OnJavascriptAllowed() override;
 
   ForeignSessionHandler();
   ~ForeignSessionHandler() override;
 
+  void InitializeForeignSessions();
+
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
   static void OpenForeignSessionTab(content::WebUI* web_ui,
@@ -69,6 +73,8 @@ class ForeignSessionHandler : public content::WebUIMessageHandler {
  private:
   void OnForeignSessionUpdated();
 
+  base::Value GetForeignSessions();
+
   // Returns a string used to show the user when a session was last modified.
   base::string16 FormatSessionTime(const base::Time& time);
 
@@ -94,6 +100,8 @@ class ForeignSessionHandler : public content::WebUIMessageHandler {
   // the WebUI was present before the sessions data was visible.
   base::TimeTicks load_attempt_time_;
 
+  base::Value initial_session_list_;
+
   std::unique_ptr<base::CallbackList<void()>::Subscription>
       foreign_session_updated_subscription_;
 

chrome/browser/ui/webui/history/history_ui.cc

@@ -156,8 +156,12 @@ HistoryUI::HistoryUI(content::WebUI* web_ui) : WebUIController(web_ui) {
   web_ui->AddMessageHandler(std::make_unique<BrowsingHistoryHandler>());
   web_ui->AddMessageHandler(std::make_unique<MetricsHandler>());
 
-  web_ui->AddMessageHandler(
-      std::make_unique<browser_sync::ForeignSessionHandler>());
+  auto foreign_session_handler =
+      std::make_unique<browser_sync::ForeignSessionHandler>();
+  browser_sync::ForeignSessionHandler* foreign_session_handler_ptr =
+      foreign_session_handler.get();
+  web_ui->AddMessageHandler(std::move(foreign_session_handler));
+  foreign_session_handler_ptr->InitializeForeignSessions();
   web_ui->AddMessageHandler(std::make_unique<HistoryLoginHandler>(
       base::Bind(&HistoryUI::UpdateDataSource, base::Unretained(this))));
 

chrome/test/data/webui/history/history_browsertest.js

@@ -60,6 +60,8 @@ HistoryItemTest.prototype = {
   __proto__: HistoryBrowserTest.prototype,
 
   extraLibraries: HistoryBrowserTest.prototype.extraLibraries.concat([
+    '../test_browser_proxy.js',
+    'test_browser_service.js',
     'history_item_test.js',
   ]),
 };
@@ -74,6 +76,8 @@ HistoryLinkClickTest.prototype = {
   __proto__: HistoryBrowserTest.prototype,
 
   extraLibraries: HistoryBrowserTest.prototype.extraLibraries.concat([
+    '../test_browser_proxy.js',
+    'test_browser_service.js',
     'link_click_test.js',
   ]),
 };

chrome/test/data/webui/history/history_focus_test.js

@@ -43,6 +43,8 @@ var HistoryToolbarFocusTest = class extends HistoryFocusTest {
   /** @override */
   get extraLibraries() {
     return super.extraLibraries.concat([
+      '../test_browser_proxy.js',
+      'test_browser_service.js',
       'history_toolbar_focus_test.js',
     ]);
   }
@@ -57,6 +59,8 @@ var HistoryListFocusTest = class extends HistoryFocusTest {
   /** @override */
   get extraLibraries() {
     return super.extraLibraries.concat([
+      '../test_browser_proxy.js',
+      'test_browser_service.js',
       'history_list_focus_test.js',
     ]);
   }

chrome/test/data/webui/history/history_item_test.js

@@ -21,9 +21,12 @@ suite('<history-item> unit test', function() {
   let item;
 
   setup(function() {
+    PolymerTest.clearBody();
+    history.BrowserService.instance_ = new TestBrowserService();
+
     item = document.createElement('history-item');
     item.item = TEST_HISTORY_RESULTS[0];
-    replaceBody(item);
+    document.body.appendChild(item);
   });
 
   test('click targets for selection', function() {
@@ -71,7 +74,12 @@ suite('<history-item> integration test', function() {
   let element;
 
   setup(function() {
-    element = replaceApp().$.history;
+    PolymerTest.clearBody();
+    history.BrowserService.instance_ = new TestBrowserService();
+
+    const app = document.createElement('history-app');
+    document.body.appendChild(app);
+    element = app.$.history;
   });
 
   test('basic separator insertion', function() {

chrome/test/data/webui/history/history_list_focus_test.js

@@ -14,7 +14,13 @@ suite('<history-list>', function() {
   TEST_HISTORY_RESULTS[2].starred = true;
 
   setup(function() {
-    app = replaceApp();
+    window.history.replaceState({}, '', '/');
+    PolymerTest.clearBody();
+    testService = new TestBrowserService();
+    history.BrowserService.instance_ = testService;
+
+    app = document.createElement('history-app');
+    document.body.appendChild(app);
     element = app.$.history;
     return test_util.flushTasks();
   });

chrome/test/data/webui/history/history_metrics_test.js

@@ -13,7 +13,6 @@ suite('Metrics', function() {
   });
 
   setup(async () => {
-    window.setForeignSessions = function() {};
     PolymerTest.clearBody();
 
     history.BrowserService.instance_ = new TestBrowserService();
@@ -23,14 +22,19 @@ suite('Metrics', function() {
     histogramMap = testService.histogramMap;
 
     app = document.createElement('history-app');
-    document.body.appendChild(app);
-    await testService.whenCalled('historyLoaded');
-
-    cr.webUIListenerCallback('sign-in-state-changed', false);
-    await test_util.flushTasks();
   });
 
+  /** @return {!Promise} Promise that resolves when setup is complete. */
+  function finishSetup() {
+    document.body.appendChild(app);
+    return testService.whenCalled('historyLoaded').then(() => {
+      cr.webUIListenerCallback('sign-in-state-changed', false);
+      return test_util.flushTasks();
+    });
+  }
+
   test('History.HistoryPageView', async () => {
+    await finishSetup();
     app.grouped_ = true;
 
     const histogram = histogramMap['History.HistoryPageView'];
@@ -50,6 +54,8 @@ suite('Metrics', function() {
   });
 
   test('history-list', async () => {
+    await finishSetup();
+
     // Create a history entry that is between 7 and 8 days in the past. For the
     // purposes of the tested functionality, we consider a day to be a 24 hour
     // period, with no regard to DST shifts.
@@ -128,12 +134,6 @@ suite('Metrics', function() {
   });
 
   test('synced-device-manager', async () => {
-    app.selectedPage_ = 'syncedTabs';
-    await test_util.flushTasks();
-
-    const histogram = histogramMap[SYNCED_TABS_HISTOGRAM_NAME];
-    assertEquals(1, histogram[SyncedTabsHistogram.INITIALIZED]);
-
     const sessionList = [
       createSession(
           'Nexus 5',
@@ -145,7 +145,16 @@ suite('Metrics', function() {
             createWindow(['http://www.gmail.com', 'http://badssl.com'])
           ]),
     ];
-    app.setForeignSessions(sessionList);
+    testService.setForeignSessions(sessionList);
+    await finishSetup();
+
+    app.selectedPage_ = 'syncedTabs';
+    await test_util.flushTasks();
+
+    const histogram = histogramMap[SYNCED_TABS_HISTOGRAM_NAME];
+    assertEquals(1, histogram[SyncedTabsHistogram.INITIALIZED]);
+
+    await testService.whenCalled('getForeignSessions');
     await test_util.flushTasks();
 
     assertEquals(1, histogram[SyncedTabsHistogram.HAS_FOREIGN_DATA]);

chrome/test/data/webui/history/history_routing_test.js

@@ -17,7 +17,12 @@ cr.define('history.history_routing_test', function() {
       }
 
       setup(function() {
-        app = replaceApp();
+        window.history.replaceState({}, '', '/');
+        PolymerTest.clearBody();
+        history.BrowserService.instance_ = new TestBrowserService();
+        app = document.createElement('history-app');
+        document.body.appendChild(app);
+
         assertEquals('chrome://history/', window.location.href);
         sidebar = app.$['content-side-bar'];
         toolbar = app.$['toolbar'];

chrome/test/data/webui/history/history_toolbar_focus_test.js

@@ -7,8 +7,13 @@ suite('<history-toolbar>', function() {
   let toolbar;
 
   setup(function() {
-    window.resultsRendered = false;
-    app = replaceApp();
+    PolymerTest.clearBody();
+    window.history.replaceState({}, '', '/');
+    testService = new TestBrowserService();
+    history.BrowserService.instance_ = testService;
+
+    app = document.createElement('history-app');
+    document.body.appendChild(app);
 
     toolbar = app.$['toolbar'];
   });
@@ -16,7 +21,7 @@ suite('<history-toolbar>', function() {
   test('search bar is focused on load in wide mode', async () => {
     toolbar.$['main-toolbar'].narrow = false;
 
-    historyResult(createHistoryInfo(), []);
+    app.historyResult(createHistoryInfo(), []);
     await test_util.flushTasks();
 
     // Ensure the search bar is focused on load.
@@ -27,13 +32,10 @@ suite('<history-toolbar>', function() {
   test('search bar is not focused on load in narrow mode', async () => {
     toolbar.$['main-toolbar'].narrow = true;
 
-    historyResult(createHistoryInfo(), []);
+    app.historyResult(createHistoryInfo(), []);
     await test_util.flushTasks();
     // Ensure the search bar is focused on load.
-    assertFalse($('history-app')
-                    .$.toolbar.$['main-toolbar']
-                    .getSearchField()
-                    .isSearchFocused());
+    assertFalse(toolbar.$['main-toolbar'].getSearchField().isSearchFocused());
   });
 
   test('shortcuts to open search field', function() {

chrome/test/data/webui/history/link_click_test.js

@@ -3,7 +3,11 @@
 // found in the LICENSE file.
 
 suite('listenForPrivilegedLinkClicks unit test', function() {
-  test('click handler', function() {
+  test('click handler', async () => {
+    PolymerTest.clearBody();
+    const testService = new TestBrowserService();
+    history.BrowserService.instance_ = testService;
+
     history.listenForPrivilegedLinkClicks();
     document.body.innerHTML = `
       <a id="file" href="file:///path/to/file">File</a>
@@ -11,18 +15,18 @@ suite('listenForPrivilegedLinkClicks unit test', function() {
       <a href="about:blank"><b id="blank">Click me</b></a>
     `;
 
-    let clickArgs = null;
-    const oldSend = chrome.send;
-    chrome.send = function(message, args) {
-      assertEquals('navigateToUrl', message);
-      clickArgs = args;
-    };
     $('file').click();
-    assertEquals('file:///path/to/file', clickArgs[0]);
+    let clickUrl = await testService.whenCalled('navigateToUrl');
+    assertEquals('file:///path/to/file', clickUrl);
+    testService.resetResolver('navigateToUrl');
+
     $('chrome').click();
-    assertEquals('about:chrome', clickArgs[0]);
+    clickUrl = await testService.whenCalled('navigateToUrl');
+    assertEquals('about:chrome', clickUrl);
+    testService.resetResolver('navigateToUrl');
+
     $('blank').click();
-    assertEquals('about:blank', clickArgs[0]);
-    chrome.send = oldSend;
+    clickUrl = await testService.whenCalled('navigateToUrl');
+    assertEquals('about:blank', clickUrl);
   });
 });

chrome/test/data/webui/history/test_browser_service.js

@@ -6,6 +6,7 @@ class TestBrowserService extends TestBrowserProxy {
   constructor() {
     super([
       'deleteForeignSession',
+      'getForeignSessions',
       'historyLoaded',
       'navigateToUrl',
       'openForeignSessionTab',
@@ -18,6 +19,7 @@ class TestBrowserService extends TestBrowserProxy {
     this.actionMap = {};
     /** @private {?PromiseResolver} */
     this.delayedRemove_ = null;
+    this.foreignSessions_ = [];
   }
 
   delayDelete() {
@@ -29,6 +31,17 @@ class TestBrowserService extends TestBrowserProxy {
     this.methodCalled('deleteForeignSession', sessionTag);
   }
 
+  /** @override */
+  getForeignSessions() {
+    this.methodCalled('getForeignSessions');
+    return Promise.resolve(this.foreignSessions_);
+  }
+
+  /** @param {!Array<!ForeignSession>} sessions */
+  setForeignSessions(sessions) {
+    this.foreignSessions_ = sessions;
+  }
+
   /** @override */
   removeVisits(visits) {
     this.methodCalled('removeVisits', visits);