Remove the use of page id from building the commit params.

BUG=369661
TEST=tests stay green

Committed: https://crrev.com/66f3ec8e5ee6c3b528b5a6054cdca85ac0b29699
Reverted: https://crrev.com/96545af0f94ec1ac19a57c86806c95834de5d33c

Review URL: https://codereview.chromium.org/839413004

Cr-Commit-Position: refs/heads/master@{#313589}

content/browser/frame_host/navigation_controller_impl_browsertest.cc

@@ -4,17 +4,29 @@
 
 #include "base/bind.h"
 #include "base/strings/stringprintf.h"
+#include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "content/test/content_browser_test_utils_internal.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 
 namespace content {
 
 class NavigationControllerBrowserTest : public ContentBrowserTest {
+ protected:
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, LoadDataWithBaseURL) {
@@ -76,5 +88,98 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
             controller.GetLastCommittedEntry()->GetURL());
 }
 
-}  // namespace content
+struct FrameNavigateParamsCapturer : public WebContentsObserver {
+ public:
+  explicit FrameNavigateParamsCapturer(FrameTreeNode* node)
+      : WebContentsObserver(
+            node->current_frame_host()->delegate()->GetAsWebContents()),
+        frame_tree_node_id_(node->frame_tree_node_id()),
+        message_loop_runner_(new MessageLoopRunner) {}
+
+  void Wait() {
+    message_loop_runner_->Run();
+  }
+
+  const FrameNavigateParams& params() const {
+    return params_;
+  }
+
+ private:
+  void DidNavigateAnyFrame(RenderFrameHost* render_frame_host,
+                           const LoadCommittedDetails& details,
+                           const FrameNavigateParams& params) override {
+    RenderFrameHostImpl* rfh =
+        static_cast<RenderFrameHostImpl*>(render_frame_host);
+    if (rfh->frame_tree_node()->frame_tree_node_id() != frame_tree_node_id_)
+      return;
+
+    params_ = params;
+    message_loop_runner_->Quit();
+  }
+
+  // The id of the FrameTreeNode in which navigations are peformed.
+  int frame_tree_node_id_;
+
+  // The params of the last navigation.
+  FrameNavigateParams params_;
+
+  // The MessageLoopRunner used to spin the message loop.
+  scoped_refptr<MessageLoopRunner> message_loop_runner_;
+};
 
+// Verify that PAGE_TRANSITION_AUTO_SUBFRAME and PAGE_TRANSITION_MANUAL_SUBFRAME
+// are properly set for subframe navigations.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       ManualAndAutoSubframeNavigationTransitions) {
+  GURL main_url(
+      embedded_test_server()->GetURL("/frame_tree/page_with_one_frame.html"));
+  NavigateToURL(shell(), main_url);
+
+  // It is safe to obtain the root frame tree node here, as it doesn't change.
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->
+          GetFrameTree()->root();
+
+  ASSERT_EQ(1U, root->child_count());
+  ASSERT_NE(nullptr, root->child_at(0));
+
+  {
+    // Navigate the iframe to a new URL; expect a manual subframe transition.
+    FrameNavigateParamsCapturer capturer(root->child_at(0));
+    GURL frame_url(
+          embedded_test_server()->GetURL("/frame_tree/2-1.html"));
+    NavigateFrameToURL(root->child_at(0), frame_url);
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_MANUAL_SUBFRAME,
+              capturer.params().transition);
+  }
+
+  {
+    // History navigations should result in an auto subframe transition.
+    FrameNavigateParamsCapturer capturer(root->child_at(0));
+    shell()->web_contents()->GetController().GoBack();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.params().transition);
+  }
+
+  {
+    // History navigations should result in an auto subframe transition.
+    FrameNavigateParamsCapturer capturer(root->child_at(0));
+    shell()->web_contents()->GetController().GoForward();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.params().transition);
+  }
+
+  {
+    // Navigate the iframe to a new URL; expect a manual subframe transition.
+    FrameNavigateParamsCapturer capturer(root->child_at(0));
+    GURL frame_url(
+          embedded_test_server()->GetURL("/frame_tree/2-3.html"));
+    NavigateFrameToURL(root->child_at(0), frame_url);
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_MANUAL_SUBFRAME,
+              capturer.params().transition);
+  }
+}
+
+}  // namespace content

content/renderer/render_frame_impl.cc

@@ -2571,7 +2571,7 @@ void RenderFrameImpl::didCommitProvisionalLoad(
   // new navigation.
   navigation_state->set_request_committed(true);
 
-  SendDidCommitProvisionalLoad(frame);
+  SendDidCommitProvisionalLoad(frame, commit_type);
 
   // Check whether we have new encoding name.
   UpdateEncoding(frame, frame->view()->pageEncoding().utf8());
@@ -3639,7 +3639,9 @@ bool RenderFrameImpl::IsHidden() {
 }
 
 // Tell the embedding application that the URL of the active page has changed.
-void RenderFrameImpl::SendDidCommitProvisionalLoad(blink::WebFrame* frame) {
+void RenderFrameImpl::SendDidCommitProvisionalLoad(
+      blink::WebFrame* frame,
+      blink::WebHistoryCommitType commit_type) {
   DCHECK(!frame_ || frame_ == frame);
   WebDataSource* ds = frame->dataSource();
   DCHECK(ds);
@@ -3702,7 +3704,7 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(blink::WebFrame* frame) {
   render_view_->navigation_gesture_ = NavigationGestureUnknown;
 
   // Make navigation state a part of the DidCommitProvisionalLoad message so
-  // that commited entry has it at all times.
+  // that committed entry has it at all times.
   HistoryEntry* entry = render_view_->history_controller()->GetCurrentEntry();
   if (entry)
     params.page_state = HistoryEntryToPageState(entry);
@@ -3807,13 +3809,12 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(blink::WebFrame* frame) {
     // Subframe navigation: the type depends on whether this navigation
     // generated a new session history entry. When they do generate a session
     // history entry, it means the user initiated the navigation and we should
-    // mark it as such. This test checks if this is the first time
-    // SendDidCommitProvisionalLoad has been called since WillNavigateToURL was
-    // called to initiate the load.
-    if (render_view_->page_id_ > render_view_->last_page_id_sent_to_browser_)
-      params.transition = ui::PAGE_TRANSITION_MANUAL_SUBFRAME;
-    else
+    // mark it as such.
+    bool is_history_navigation = commit_type == blink::WebBackForwardCommit;
+    if (is_history_navigation || ds->replacesCurrentHistoryItem())
       params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
+    else
+      params.transition = ui::PAGE_TRANSITION_MANUAL_SUBFRAME;
 
     DCHECK(!navigation_state->history_list_was_cleared());
     params.history_list_was_cleared = false;
@@ -3824,10 +3825,6 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(blink::WebFrame* frame) {
       Send(new FrameHostMsg_DidCommitProvisionalLoad(routing_id_, params));
   }
 
-  render_view_->last_page_id_sent_to_browser_ =
-      std::max(render_view_->last_page_id_sent_to_browser_,
-               render_view_->page_id_);
-
   // If we end up reusing this WebRequest (for example, due to a #ref click),
   // we don't want the transition type to persist.  Just clear it.
   navigation_state->set_transition_type(ui::PAGE_TRANSITION_LINK);

content/renderer/render_frame_impl.h

@@ -566,7 +566,8 @@ class CONTENT_EXPORT RenderFrameImpl
   void RemoveObserver(RenderFrameObserver* observer);
 
   // Builds and sends DidCommitProvisionalLoad to the host.
-  void SendDidCommitProvisionalLoad(blink::WebFrame* frame);
+  void SendDidCommitProvisionalLoad(blink::WebFrame* frame,
+                                    blink::WebHistoryCommitType commit_type);
 
   // Gets the focused element. If no such element exists then the element will
   // be NULL.

content/renderer/render_view_impl.cc

@@ -642,7 +642,6 @@ RenderViewImpl::RenderViewImpl(const ViewMsg_New_Params& params)
       opener_suppressed_(false),
       suppress_dialogs_until_swap_out_(false),
       page_id_(-1),
-      last_page_id_sent_to_browser_(-1),
       next_page_id_(params.next_page_id),
       history_list_offset_(-1),
       history_list_length_(0),

content/renderer/render_view_impl.h

@@ -857,13 +857,6 @@ class CONTENT_EXPORT RenderViewImpl
   // See documentation in RenderView.
   int32 page_id_;
 
-  // Indicates the ID of the last page that we sent a FrameNavigate to the
-  // browser for. This is used to determine if the most recent transition
-  // generated a history entry (less than page_id_), or not (equal to or
-  // greater than). Note that this will be greater than page_id_ if the user
-  // goes back.
-  int32 last_page_id_sent_to_browser_;
-
   // The next available page ID to use for this RenderView.  These IDs are
   // specific to a given RenderView and the frames within it.
   int32 next_page_id_;