PlzNavigate: Make ServiceWorker work with PlzNavigate.

content/browser/frame_host/navigation_handle_impl.h

 // Copyright 2015 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.
 
 #ifndef CONTENT_BROWSER_FRAME_HOST_NAVIGATION_HANDLE_IMPL_H_
 #define CONTENT_BROWSER_FRAME_HOST_NAVIGATION_HANDLE_IMPL_H_
 
 #include "content/public/browser/navigation_handle.h"
 
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/navigation_throttle.h"
 #include "url/gurl.h"
 
 namespace content {
 
 class NavigatorDelegate;
 struct NavigationRequestInfo;
+class ServiceWorkerContext;

[michaeln, 2015/10/01 01:42:15]

i don't think this forward is needed

[Fabrice (no longer in Chrome), 2015/10/01 18:29:55]

Done.

+class ServiceWorkerContextWrapper;
 
 // This class keeps track of a single navigation. It is created upon receipt of
 // a DidStartProvisionalLoad IPC in a RenderFrameHost. The RenderFrameHost owns
 // the newly created NavigationHandleImpl as long as the navigation is ongoing.
 // The NavigationHandleImpl in the RenderFrameHost will be reset when the
 // navigation stops, that is if one of the following events happen:
 //   - The RenderFrameHost receives a DidStartProvisionalLoad IPC for a new
 //   navigation (see below for special cases where the DidStartProvisionalLoad
 //   message does not indicate the start of a new navigation).
 //   - The RenderFrameHost stops loading.
@@ skipping 16 matching lines @@
 // NavigationRequest. It is then owned by the NavigationRequest until the
 // navigation is ready to commit. The NavigationHandleImpl ownership is then
 // transferred to the RenderFrameHost in which the navigation will commit.
 //
 // When PlzNavigate is enabled, the NavigationHandleImpl will never be reset
 // following the receipt of a DidStartProvisionalLoad IPC. There are also no
 // transferring navigations. The other causes of NavigationHandleImpl reset in
 // the RenderFrameHost still apply.
 class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
  public:
-  static scoped_ptr<NavigationHandleImpl> Create(const GURL& url,
-                                                 bool is_main_frame,
-                                                 NavigatorDelegate* delegate);
+  static scoped_ptr<NavigationHandleImpl> Create(
+      const GURL& url,
+      bool is_main_frame,
+      NavigatorDelegate* delegate,
+      int service_worker_provider_id);

[clamy, 2015/10/01 12:41:26]

I do not like having the service_worker_provider_id be a parameter to this
function, that is used both by PlzNavigate and non-PlzNavigate code. I think we
could consider the following options:
- having the id be generated in the constructor of NavigationHandleImpl, and
then put it in the RequestNavigationParams.
- add a boolean to the request params to check whether they are constructed in
the browser or the renderer. If they are constructed in the browser, then
generate a SWid, otherwise the SWid is kInvalid... In that case, when the handle
is created, we can set its swid to the one in the request params.

[Fabrice (no longer in Chrome), 2015/10/01 18:29:55]

That was my first idea too, but the NavigationRequestInfo is created before the
NavigationHandleImpl, meaning it is too late to add the information to the
RequestNavigationParams after NavigationHandleImpl ctor is called.
Same problem as above, it is too late to create the ID in this ctor and pass it
back to NavigationRequestInfo.

[michaeln, 2015/10/01 23:11:11]

I think your talking about the construction sequences below and how the handle
is created after NavigitaionRequest (whose constructor creates the
NavigationRequestInfo struct). Is that right? Also am I right in thinking that
these are the only two callsites where we need to precreate a swprovider host?

void NavigatorImpl::OnBeginNavigation
  ...
  frame_tree_node->CreatedNavigationRequest(
      NavigationRequest::CreateRendererInitiated(
          frame_tree_node, common_params, begin_params, body,
          controller_->GetLastCommittedEntryIndex(),
          controller_->GetEntryCount()));
  NavigationRequest* navigation_request = frame_tree_node->navigation_request();
  navigation_request->CreateNavigationHandle(delegate_);


void NavigatorImpl::RequestNavigation
  ....
  frame_tree_node->CreatedNavigationRequest(
      NavigationRequest::CreateBrowserInitiated(
          frame_tree_node, dest_url, dest_referrer, frame_entry, entry,
          navigation_type, is_same_document_history_load, navigation_start,
          controller_));
  NavigationRequest* navigation_request = frame_tree_node->navigation_request();
  navigation_request->CreateNavigationHandle(delegate_);


It looks pretty straightforward to mildy alter this construction sequence. You
could pass the delegate_ into the factory methods and have the handle be created
during NavigationRequest construction. You could probably get rid of the
CreateNavigationHandle method and retain the const getter and
TakeNavigationHandle method. Then the NavigationRequest could internally
construct the NavigationHandle which could internally construct the
ServiceWorkerNavigationHandle. Or the NavigationRequest::CreateXxxxInitiated
methods could take a scoped_ptr<ServiceWorkerNavigationHandle> as input.

I think something like that would be nice cleanup.

void NavigatorImpl::OnBeginNavigation
  ...
  frame_tree_node->CreatedNavigationRequest(
      NavigationRequest::CreateRendererInitiated(
          frame_tree_node, common_params, begin_params, body,
          controller_->GetLastCommittedEntryIndex(),
          controller_->GetEntryCount(), delegate_,
          make_scoped_ptr(new ServiceWorkerNavigationHandle(swcontext_wrapper));


void NavigatorImpl::RequestNavigation
  ....
  frame_tree_node->CreatedNavigationRequest(
      NavigationRequest::CreateBrowserInitiated(
          frame_tree_node, dest_url, dest_referrer, frame_entry, entry,
          navigation_type, is_same_document_history_load, navigation_start,
          controller_, delegate_,
          make_scoped_ptr(new
ServiceWorkerNavigationHandle(swcontext_wrapper))));

[clamy, 2015/10/02 10:34:44]

While it does look straightforward to alter the construction sequence, there is
a reason the NavigationHandle is not constructed in the NavigationRequest in the
first place. The construction and destruction of the NavigationHandle trigger
calls to WCO::DidStartNavigation and WCO::DidFinishNavigation. In order not to
confuse observers, we want the call to DidFinishNavigation to arrive first for
the old navigation and then the call to DidStartNavigation for the second
navigation. This means destroying the old NavigationHandle before creating the
new one. Since the NavigationRequest that owns the old handle is destroyed in
frame_tree_node->CreatedNavigationRequest, if the new request already contains
the new handle, we break this ordering.

We considered creating the NavigationRequest inside the FrameTreeNode (after the
old request has been destroyed), but ruled that out since it's bringing too much
navigation knowledge that FrameTreeNode is not meant to handle. I'm also
thinking that we will want to merge NavigationRequest and NavigationHandle into
a single class once PlzNavigate is enabled by default, hence why it was left
this way.

Thinking on the problem some more, I think we should initialize the SWProviderID
neither in the constructor of RequestNavigationParams nor NavigationHandle. The
SWProviderID should only be initialized when the NavigationRequest is sent to
the network stack, when we are sure we will create a SWProviderHost. Otherwise,
I'm worried we may introduce some bugs with navigations that commit without
making a network request.

In order to do so, the id will be set in the RequestNavigationParams and the
NavigationHandle when NavigationRequest::BeginNavigation determined that a
network request needed to be made.

[Fabrice (no longer in Chrome), 2015/10/02 16:37:34]

That works thanks!

   ~NavigationHandleImpl() override;
 
   // NavigationHandle implementation:
   const GURL& GetURL() override;
   bool IsInMainFrame() override;
   bool IsPost() override;
   const Referrer& GetReferrer() override;
   bool HasUserGesture() override;
   ui::PageTransition GetPageTransition() override;
   bool IsExternalProtocol() override;
@@ skipping 64 matching lines @@
   // Used to track the state the navigation is currently in.
   enum State {
     INITIAL = 0,
     WILL_SEND_REQUEST,
     READY_TO_COMMIT,
     DID_COMMIT,
     DID_COMMIT_ERROR_PAGE,
   };
 
   NavigationHandleImpl(const GURL& url,
-                       const bool is_main_frame,
-                       NavigatorDelegate* delegate);
+                       bool is_main_frame,
+                       NavigatorDelegate* delegate,
+                       int service_worker_provider_id);
 
   // See NavigationHandle for a description of those member variables.
   GURL url_;
   const bool is_main_frame_;
   bool is_post_;
   Referrer sanitized_referrer_;
   bool has_user_gesture_;
   ui::PageTransition transition_;
   bool is_external_protocol_;
   net::Error net_error_code_;
   RenderFrameHostImpl* render_frame_host_;
   bool is_same_page_;
 
   // The state the navigation is in.
   State state_;
 
   // Whether the navigation is in the middle of a transfer. Set to false when
   // the DidStartProvisionalLoad is received from the new renderer.
   bool is_transferring_;
 
   // The delegate that should be notified about events related to this
   // navigation.
   NavigatorDelegate* delegate_;
 
   // A list of Throttles registered for this navigation.
   ScopedVector<NavigationThrottle> throttles_;
 
+  // PlzNavigate
+  // The SWProvider ID used for navigations.

[michaeln, 2015/10/01 01:42:15]

for non-plznav navigations, this should probably be a kInvalid number

[clamy, 2015/10/01 12:41:26]

nit: use the full class name for SWProvider. Also, as michaeln@ suggested,
mention its value for the non-PlzNavigate case.

[Fabrice (no longer in Chrome), 2015/10/01 18:29:54]

Done.

+  int service_worker_provider_id_;
+
+  scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_;
+
   DISALLOW_COPY_AND_ASSIGN(NavigationHandleImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_NAVIGATION_HANDLE_IMPL_H_