PlzNavigate: Fix form submission to OOPIF frame.

content/browser/frame_host/navigation_request.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigation_request.h"
 
 #include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
@@ skipping 181 matching lines @@
 std::unique_ptr<NavigationRequest> NavigationRequest::CreateBrowserInitiated(
     FrameTreeNode* frame_tree_node,
     const GURL& dest_url,
     const Referrer& dest_referrer,
     const FrameNavigationEntry& frame_entry,
     const NavigationEntryImpl& entry,
     FrameMsg_Navigate_Type::Value navigation_type,
     PreviewsState previews_state,
     bool is_same_document_history_load,
     bool is_history_navigation_in_new_child,
+    const scoped_refptr<ResourceRequestBodyImpl>& post_body,
     const base::TimeTicks& navigation_start,
     NavigationControllerImpl* controller) {
-  // Fill POST data in the request body.
+  // A form submission happens either because the navigation is a
+  // renderer-initiated form submission that took the OpenURL path or either a

[nasko, 2017/05/16 04:08:21]

nit: s/or either/or/

[arthursonzogni, 2017/05/16 12:45:58]

Done.

+  // back/forward/reload navigation the does a form resubmission.
   scoped_refptr<ResourceRequestBodyImpl> request_body;
-  if (frame_entry.method() == "POST")
+  if (post_body) {
+    // Standard form-submission from the renderer.

[nasko, 2017/05/16 04:08:21]

nit: no need for "-" between form and submission

[arthursonzogni, 2017/05/16 12:45:58]

Done.

+    request_body = post_body;
+  } else if (frame_entry.method() == "POST") {
+    // Form resubmission during a back/forward/reload navigation.
     request_body = frame_entry.GetPostData();
+  }
+  // TODO(arthursonzogni): Form submission with the "GET" method is possible.
+  // This is not currently handled here.
+  bool is_form_submission = bool(request_body);

[nasko, 2017/05/16 04:08:21]

nit: !!request_body?

[arthursonzogni, 2017/05/16 12:45:58]

I would prefer not to. If you don't like: "bool(...)", maybe you would prefer:
"(request_body != nullptr)"

If it is okay, feel free to run the CQ.

[nasko, 2017/05/16 14:02:09]

The main point is to keep code consistent. I haven't seen "bool()" usage around
and it surprised me. I'm fine with "(request_body != nullptr)", though we try to
avoid that too in favor of the "!!ptr" pattern.

 
   base::Optional<url::Origin> initiator =
       frame_tree_node->IsMainFrame()
           ? base::Optional<url::Origin>()
           : base::Optional<url::Origin>(
                 frame_tree_node->frame_tree()->root()->current_origin());
 
   // While the navigation was started via the LoadURL path it may have come from
   // the renderer in the first place as part of OpenURL.
   bool browser_initiated = !entry.is_renderer_initiated();
 
   CommonNavigationParams common_params = entry.ConstructCommonNavigationParams(
       frame_entry, request_body, dest_url, dest_referrer, navigation_type,
       previews_state, navigation_start);
 
   std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(
       frame_tree_node, common_params,
       BeginNavigationParams(entry.extra_headers(), net::LOAD_NORMAL,
                             false,  // has_user_gestures
                             false,  // skip_service_worker
                             REQUEST_CONTEXT_TYPE_LOCATION,
                             blink::WebMixedContentContextType::kBlockable,
-                            // TODO(arthursonzogni): It can be true for form
-                            // resubmission when the user reloads the page. This
-                            // needs to be fixed.
-                            false,  // is_form_submission
-                            initiator),
+                            is_form_submission, initiator),
       entry.ConstructRequestNavigationParams(
           frame_entry, common_params.url, common_params.method,
           is_history_navigation_in_new_child,
           entry.GetSubframeUniqueNames(frame_tree_node),
           frame_tree_node->has_committed_real_load(),
           controller->GetPendingEntryIndex() == -1,
           controller->GetIndexOfEntry(&entry),
           controller->GetLastCommittedEntryIndex(),
           controller->GetEntryCount()),
       browser_initiated,
@@ skipping 617 matching lines @@
   DCHECK_EQ(request_params_.has_user_gesture, begin_params_.has_user_gesture);
 
   render_frame_host->CommitNavigation(response_.get(), std::move(body_),
                                       std::move(handle_), common_params_,
                                       request_params_, is_view_source_);
 
   frame_tree_node_->ResetNavigationRequest(true, true);
 }
 
 }  // namespace content