Update browser history on portal activation

When a portal contents is activated, we swap it with its predecessor,
so to the user it appears that the tab has navigated. However, we did
not notify the history service about this, so the page the user is now
viewing does not appear in chrome://history/.

Now upon portal activation, we have the history service add a page visit
for the portal's last committed entry.

Bug: 944198
Change-Id: Iebb0c66a2afddefc5172c84158ea2d01511c8309
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2080832
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#761141}

chrome/android/javatests/src/org/chromium/chrome/browser/portals/PortalsTest.java

@@ -23,6 +23,9 @@ import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
+import org.chromium.chrome.browser.history.BrowsingHistoryBridge;
+import org.chromium.chrome.browser.history.HistoryItem;
+import org.chromium.chrome.browser.history.TestBrowsingHistoryObserver;
 import org.chromium.chrome.browser.login.ChromeHttpAuthHandler;
 import org.chromium.chrome.browser.tab.EmptyTabObserver;
 import org.chromium.chrome.browser.tab.Tab;
@@ -38,6 +41,7 @@ import org.chromium.content_public.browser.test.util.TouchCommon;
 import org.chromium.net.test.EmbeddedTestServer;
 
 import java.util.List;
+import java.util.concurrent.TimeoutException;
 
 /**
  * Tests for the chrome/ layer support of the HTML portal element.
@@ -118,6 +122,17 @@ public class PortalsTest {
         swapWaiter.waitForCallback(currSwapCount, 1);
     }
 
+    private List<HistoryItem> getBrowsingHistory() throws TimeoutException {
+        TestBrowsingHistoryObserver observer = new TestBrowsingHistoryObserver();
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            BrowsingHistoryBridge provider = new BrowsingHistoryBridge(/* isIncognito */ false);
+            provider.setObserver(observer);
+            provider.queryHistory(/* query */ "");
+        });
+        observer.getQueryCallback().waitForCallback(0);
+        return observer.getHistoryQueryResults();
+    }
+
     /**
      * Tests that a portal can be activated and have its contents swapped in to its embedder's tab.
      */
@@ -417,4 +432,40 @@ public class PortalsTest {
         ThreadUtils.runOnUiThread(() -> authHandler.proceed("basicuser", "secret"));
         CriteriaHelper.pollUiThread(Criteria.equals("basicuser/secret", portalContents::getTitle));
     }
+
+    /**
+     * Tests that content in a portal is considered a page visit in browser history upon activation.
+     */
+    @Test
+    @MediumTest
+    @Feature({"Portals"})
+    public void testBrowserHistoryUpdatesOnActivation() throws Exception {
+        String mainUrl = mTestServer.getURL(
+                "/chrome/test/data/android/portals/portal-to-basic-content.html");
+        String mainTitle = "Test Portal";
+        String portalUrl =
+                mTestServer.getURL("/chrome/test/data/android/portals/basic-content.html");
+        String portalTitle = "Test Portal Content";
+
+        mActivityTestRule.startMainActivityWithURL(mainUrl);
+        Tab tab = mActivityTestRule.getActivity().getActivityTab();
+
+        // Content loaded in a portal should not be considered a page visit by the
+        // user.
+        List<HistoryItem> history = getBrowsingHistory();
+        Assert.assertEquals(1, history.size());
+        Assert.assertEquals(mainUrl, history.get(0).getUrl());
+        Assert.assertEquals(mainTitle, history.get(0).getTitle());
+
+        executeScriptAndAwaitSwap(tab, "activatePortal();");
+
+        // Now that the portal has activated, its contents are presented to the user
+        // as a navigation in the tab, so this should be considered a page visit.
+        history = getBrowsingHistory();
+        Assert.assertEquals(2, history.size());
+        Assert.assertEquals(portalUrl, history.get(0).getUrl());
+        Assert.assertEquals(portalTitle, history.get(0).getTitle());
+        Assert.assertEquals(mainUrl, history.get(1).getUrl());
+        Assert.assertEquals(mainTitle, history.get(1).getTitle());
+    }
 }

chrome/browser/android/tab_web_contents_delegate_android.cc

@@ -26,6 +26,7 @@
 #include "chrome/browser/file_select_helper.h"
 #include "chrome/browser/flags/android/cached_feature_flags.h"
 #include "chrome/browser/flags/android/chrome_feature_list.h"
+#include "chrome/browser/history/history_tab_helper.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/media/protected_media_identifier_permission_context.h"
 #include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
@@ -157,10 +158,12 @@ void TabWebContentsDelegateAndroid::PortalWebContentsCreated(
   // TODO(jbroman): This doesn't adequately handle all of the tab helpers that
   // might be wanted here, and there are also likely to be issues with tab
   // helpers that are unprepared for portal activation to transition them.
+  // See https://crbug.com/1042323
   autofill::ChromeAutofillClient::CreateForWebContents(portal_contents);
   ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
       portal_contents,
       autofill::ChromeAutofillClient::FromWebContents(portal_contents));
+  HistoryTabHelper::CreateForWebContents(portal_contents);
   InfoBarService::CreateForWebContents(portal_contents);
 }
 

chrome/browser/history/history_tab_helper.cc

@@ -52,7 +52,7 @@ void HistoryTabHelper::UpdateHistoryForNavigation(
     const history::HistoryAddPageArgs& add_page_args) {
   history::HistoryService* hs = GetHistoryService();
   if (hs)
-    GetHistoryService()->AddPage(add_page_args);
+    hs->AddPage(add_page_args);
 }
 
 history::HistoryAddPageArgs
@@ -137,6 +137,11 @@ void HistoryTabHelper::DidFinishNavigation(
   if (!navigation_handle->ShouldUpdateHistory())
     return;
 
+  // Navigations in portals don't appear in history until the portal is
+  // activated.
+  if (navigation_handle->GetWebContents()->IsPortal())
+    return;
+
   // Most of the time, the displayURL matches the loaded URL, but for about:
   // URLs, we use a data: URL as the real value.  We actually want to save the
   // about: URL to the history db and keep the data: URL hidden. This is what
@@ -177,6 +182,36 @@ void HistoryTabHelper::DidFinishNavigation(
   UpdateHistoryForNavigation(add_page_args);
 }
 
+// We update history upon the associated WebContents becoming the top level
+// contents of a tab from portal activation.
+// TODO(mcnee): Investigate whether the early return cases in
+// DidFinishNavigation apply to portal activation. See https://crbug.com/1072762
+void HistoryTabHelper::DidActivatePortal(
+    content::WebContents* predecessor_contents) {
+  history::HistoryService* hs = GetHistoryService();
+  if (!hs)
+    return;
+
+  content::NavigationEntry* last_committed_entry =
+      web_contents()->GetController().GetLastCommittedEntry();
+
+  // TODO(1058504): Update this when portal activations can be done with
+  // replacement.
+  const bool did_replace_entry = false;
+
+  const history::HistoryAddPageArgs add_page_args(
+      last_committed_entry->GetVirtualURL(),
+      last_committed_entry->GetTimestamp(),
+      history::ContextIDForWebContents(web_contents()),
+      last_committed_entry->GetUniqueID(),
+      last_committed_entry->GetReferrer().url,
+      /* redirects */ {}, ui::PAGE_TRANSITION_LINK,
+      /* hidden */ false, history::SOURCE_BROWSED, did_replace_entry,
+      /* consider_for_ntp_most_visited */ true,
+      last_committed_entry->GetTitle());
+  hs->AddPage(add_page_args);
+}
+
 void HistoryTabHelper::DidFinishLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url) {

chrome/browser/history/history_tab_helper.h

@@ -22,7 +22,7 @@ class HistoryTabHelper : public content::WebContentsObserver,
   ~HistoryTabHelper() override;
 
   // Updates history with the specified navigation. This is called by
-  // OnMsgNavigate to update history state.
+  // DidFinishNavigation to update history state.
   void UpdateHistoryForNavigation(
       const history::HistoryAddPageArgs& add_page_args);
 
@@ -41,6 +41,7 @@ class HistoryTabHelper : public content::WebContentsObserver,
   // content::WebContentsObserver implementation.
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
+  void DidActivatePortal(content::WebContents* predecessor_contents) override;
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override;
   void TitleWasSet(content::NavigationEntry* entry) override;

chrome/browser/portal/portal_browsertest.cc

@@ -9,6 +9,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/devtools/devtools_window_testing.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/history/history_test_utils.h"
 #include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
 #include "chrome/browser/pdf/pdf_extension_test_util.h"
 #include "chrome/browser/task_manager/providers/task.h"
@@ -320,3 +322,46 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, ShowSubFrameErrorPage) {
                       portal_contents,
                       "document.documentElement.hasAttribute('subframe');"));
 }
+
+IN_PROC_BROWSER_TEST_F(PortalBrowserTest, BrowserHistoryUpdatesOnActivation) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  Profile* profile = browser()->profile();
+  ASSERT_TRUE(profile);
+  ui_test_utils::WaitForHistoryToLoad(HistoryServiceFactory::GetForProfile(
+      profile, ServiceAccessType::EXPLICIT_ACCESS));
+
+  GURL url1(embedded_test_server()->GetURL("/title1.html"));
+  ui_test_utils::NavigateToURL(browser(), url1);
+  WaitForHistoryBackendToRun(profile);
+  EXPECT_TRUE(
+      base::Contains(ui_test_utils::HistoryEnumerator(profile).urls(), url1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  GURL url2(embedded_test_server()->GetURL("/title2.html"));
+  ASSERT_EQ(true,
+            content::EvalJs(
+                contents, content::JsReplace(
+                              "new Promise((resolve) => {"
+                              "  let portal = document.createElement('portal');"
+                              "  portal.src = $1;"
+                              "  portal.onload = () => { resolve(true); };"
+                              "  document.body.appendChild(portal);"
+                              "});",
+                              url2)));
+  WaitForHistoryBackendToRun(profile);
+  // Content loaded in a portal should not be considered a page visit by the
+  // user.
+  EXPECT_FALSE(
+      base::Contains(ui_test_utils::HistoryEnumerator(profile).urls(), url2));
+
+  ASSERT_EQ(true,
+            content::EvalJs(contents,
+                            "let portal = document.querySelector('portal');"
+                            "portal.activate().then(() => { return true; });"));
+  WaitForHistoryBackendToRun(profile);
+  // Now that the portal has activated, its contents are presented to the user
+  // as a navigation in the tab, so this should be considered a page visit.
+  EXPECT_TRUE(
+      base::Contains(ui_test_utils::HistoryEnumerator(profile).urls(), url2));
+}

chrome/test/data/android/portals/portal-to-basic-content.html

@@ -11,10 +11,17 @@
   </head>
   <body>
     <h1>Test Portal</h1>
-    <portal id="myPortal" src="basic-content.html"></portal>
     <script>
-      function activatePortal() {
-        document.getElementById('myPortal').activate();
+      var portal = document.createElement('portal');
+      var portalLoaded = new Promise((resolve) => {
+        portal.onload = resolve;
+      });
+      portal.src = 'basic-content.html';
+      document.body.appendChild(portal);
+
+      async function activatePortal() {
+        await portalLoaded;
+        await portal.activate();
       }
     </script>
   </body>

content/browser/accessibility/browser_accessibility_manager.cc

@@ -1517,7 +1517,8 @@ void BrowserAccessibilityManager::CacheHitTestResult(
   last_hover_bounds_ = hit_test_result->GetClippedScreenBoundsRect();
 }
 
-void BrowserAccessibilityManager::OnPortalActivated() {
+void BrowserAccessibilityManager::DidActivatePortal(
+    WebContents* predecessor_contents) {
   if (GetTreeData().loaded) {
     FireGeneratedEvent(ui::AXEventGenerator::Event::PORTAL_ACTIVATED,
                        GetRoot());

content/browser/accessibility/browser_accessibility_manager.h

@@ -194,7 +194,10 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver,
   virtual void UserIsReloading();
   void NavigationSucceeded();
   void NavigationFailed();
+
+  // WebContentsObserver overrides
   void DidStopLoading() override;
+  void DidActivatePortal(WebContents* predecessor_contents) override;
 
   // Keep track of if this page is hidden by an interstitial, in which case
   // we need to suppress all events.
@@ -453,10 +456,6 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver,
   // call to CachingAsyncHitTest().
   void CacheHitTestResult(BrowserAccessibility* hit_test_result) const;
 
-  // Called when |this| is an accessibility manager for a portal's main frame,
-  // and when that portal is activated.
-  void OnPortalActivated();
-
  protected:
   explicit BrowserAccessibilityManager(BrowserAccessibilityDelegate* delegate);
 

content/browser/portal/portal.cc

@@ -438,24 +438,18 @@ void Portal::Activate(blink::TransferableMessage data,
 
   // These pointers are cleared so that they don't dangle in the event this
   // object isn't immediately deleted. It isn't done sooner because
-  // SwapWebContents misbehaves if the WebContents doesn't appear to be a portal
-  // at that time.
+  // ActivatePortalWebContents misbehaves if the WebContents doesn't appear to
+  // be a portal at that time.
   portal_contents_.Clear();
 
   successor_contents_raw->GetMainFrame()->OnPortalActivated(
       std::move(predecessor_web_contents), std::move(data),
       std::move(callback));
-  successor_contents_raw->NotifyInsidePortal(false);
-
-  // This happens later than SwapWebContents so that the delegate can observe it
-  // happening after predecessor_web_contents has been moved into a portal.
-  successor_contents_raw->GetDelegate()->WebContentsBecamePortal(
-      outer_contents);
-
-  BrowserAccessibilityManager* manager =
-      successor_contents_raw->GetMainFrame()->browser_accessibility_manager();
-  if (manager)
-    manager->OnPortalActivated();
+  // Notifying of activation happens later than ActivatePortalWebContents so
+  // that it is observed after predecessor_web_contents has been moved into a
+  // portal.
+  DCHECK(outer_contents->IsPortal());
+  successor_contents_raw->DidActivatePortal(outer_contents);
 }
 
 void Portal::PostMessageToGuest(

content/browser/web_contents/web_contents_impl.cc

@@ -1942,6 +1942,14 @@ void WebContentsImpl::ReattachToOuterWebContentsFrame() {
   GetMainFrame()->UpdateAXTreeData();
 }
 
+void WebContentsImpl::DidActivatePortal(WebContents* predecessor_contents) {
+  DCHECK(predecessor_contents);
+  NotifyInsidePortal(false);
+  GetDelegate()->WebContentsBecamePortal(predecessor_contents);
+  for (auto& observer : observers_)
+    observer.DidActivatePortal(predecessor_contents);
+}
+
 void WebContentsImpl::NotifyInsidePortal(bool inside_portal) {
   SendPageMessage(new PageMsg_SetInsidePortal(MSG_ROUTING_NONE, inside_portal));
 }

content/browser/web_contents/web_contents_impl.h

@@ -1164,6 +1164,11 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   void set_portal(Portal* portal) { portal_ = portal; }
   Portal* portal() const { return portal_; }
 
+  // Notifies observers that this WebContents was activated. This contents'
+  // former portal host, |predecessor_contents|, has become a portal pending
+  // adoption.
+  void DidActivatePortal(WebContents* predecessor_contents);
+
   // Sends a page message to notify every process in the frame tree if the
   // web contents is a portal web contents.
   void NotifyInsidePortal(bool inside_portal);

content/public/browser/web_contents_observer.h

@@ -607,6 +607,12 @@ class CONTENT_EXPORT WebContentsObserver : public IPC::Listener {
   virtual void AudioContextPlaybackStopped(
       const AudioContextId& audio_context_id) {}
 
+  // Called after the contents replaces the |predecessor_contents| in its
+  // container due to portal activation. The |predecessor_contents| is now a
+  // portal pending adoption. |predecessor_contents| is non-null, but may
+  // subsequently be destroyed if it is not adopted.
+  virtual void DidActivatePortal(WebContents* predecessor_contents) {}
+
   // IPC::Listener implementation.
   // DEPRECATED: Use (i.e. override) the other overload instead:
   //     virtual bool OnMessageReceived(const IPC::Message& message,