PlzNavigate: Make ServiceWorker work with PlzNavigate.

content/browser/frame_host/navigation_handle_impl.h

Index: content/browser/frame_host/navigation_handle_impl.h
diff --git a/content/browser/frame_host/navigation_handle_impl.h b/content/browser/frame_host/navigation_handle_impl.h
index edd7b5552c95227a2980da067c8b71d17ea760ca..0fa75570fac86205d3b7a83321c42057272533f7 100644
--- a/content/browser/frame_host/navigation_handle_impl.h
+++ b/content/browser/frame_host/navigation_handle_impl.h
@@ -8,6 +8,7 @@
 #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"
@@ -19,6 +20,8 @@ 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
@@ -55,9 +58,11 @@ struct NavigationRequestInfo;
 // 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:
@@ -142,8 +147,9 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
   };
 
   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_;
@@ -171,6 +177,12 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
   // 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);
 };