PlzNavigate: Make ServiceWorker work with PlzNavigate.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index b2e454f4cdece95127e38118fd56c6f05e9fb887..12b35719453a55871ba75163b6cbe65c80d83c7b 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -2576,23 +2576,44 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
     }
   }
 
-  // Create the serviceworker's per-document network observing object if it
-  // does not exist (When navigation happens within a page, the provider already
-  // exists).
-  if (!ServiceWorkerNetworkProvider::FromDocumentState(
-          DocumentState::FromDataSource(datasource))) {
-    ServiceWorkerProviderType provider_type =
-        SERVICE_WORKER_PROVIDER_FOR_WINDOW;
-    if ((frame->effectiveSandboxFlags() & blink::WebSandboxFlags::Origin) ==
-        blink::WebSandboxFlags::Origin) {
+  bool browser_side_navigation =

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

this block of code is still tricky to follow and easily error prone?

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

Refactored a bit. WDYT?

+      base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation);
+  if ((browser_side_navigation && content_initiated) ||

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

We end up w/o having created a ServiceWorkerNetworkProvider in this case which i
think may be a bug, i think is current sources we always return from this method
with a non-null provider. We should retain that behavior.

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

In PlzNavigate, this methods ends up being called twice. The first time, we
haven't received the RequestNavigationParams yet so we cannot initialize
anything even if we wanted to.
That's why I'm testing for content_initiated.

+      (!browser_side_navigation &&
+       ServiceWorkerNetworkProvider::FromDocumentState(

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

i think u could put this up top (at line) as an early return condition
independent of plznav mode. if a provider exists we don't want to create a new
one <period>.

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

I am not 100% sure about that since we end up creating a new SWPH for
renderer-initiated navigations. Do we need to take care of cleaning that up
somewhere when a renderer intializes a new navigation?
Either way, I changed that in the next patch.

+           DocumentState::FromDataSource(datasource))))
+    return;
+
+  int service_worker_provider_id = kInvalidServiceWorkerProviderId;
+  ServiceWorkerProviderType provider_type =
+      SERVICE_WORKER_PROVIDER_FOR_WINDOW;
+
+  // PlzNavigate
+  // Retrieve the service_worker_provider_id from the RequestNavigationParams.
+  if (browser_side_navigation) {
+    NavigationStateImpl* navigation_state =
+        static_cast<NavigationStateImpl*>(
+            DocumentState::FromDataSource(datasource)->navigation_state());
+    service_worker_provider_id =
+        navigation_state->request_params().service_worker_provider_id;
+    if (service_worker_provider_id == kInvalidServiceWorkerProviderId)
       provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME;

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

The provider_type is only used by the ServiceWorkerNetworkProvider class to
possibly compute a kInvalid id. Since this class now has so much logic dedicated
to dealing with that id value, for the plznav case, lets hoist all of the logic
into this class and keep the ServiceWorkerNetworkProvider ctor simple.

ServiceWorkerNetworkProvider(route_id, provider_id) {
    : provider_id_(provider_id) {
  if (provider_id_ == kInvalid)
    return;
  context_ = new ServiceWorkerProviderContext(provider_id_);
  if (!ChildThreadImpl::current())
    return;
  ChildThreadImpl::current()->Send(...);l
}

And retain the existing ctor for use by non-plz nav case. I think it can be
implemented in terms of the other.

ServiceWorkerNetworkProvider(route_id, provider_type) {
      : ServiceWorkerNetworkProvider(route_id, GenerateIdForType(type)) {}

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

Thanks, I moved the logic up here.
Done.

-    }
-    scoped_ptr<ServiceWorkerNetworkProvider> network_provider(
-        new ServiceWorkerNetworkProvider(routing_id_, provider_type));
-    ServiceWorkerNetworkProvider::AttachToDocumentState(
-        DocumentState::FromDataSource(datasource),
-        network_provider.Pass());
   }
+
+  if (!browser_side_navigation) {

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

this could be an else clause

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

... Yes. I feel silly.

+      if ((frame->effectiveSandboxFlags() & blink::WebSandboxFlags::Origin) ==
+          blink::WebSandboxFlags::Origin) {
+        provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME;
+      }
+  }
+
+  scoped_ptr<ServiceWorkerNetworkProvider> network_provider(
+      new ServiceWorkerNetworkProvider(routing_id_, provider_type,
+                                       service_worker_provider_id));
+  ServiceWorkerNetworkProvider::AttachToDocumentState(
+      DocumentState::FromDataSource(datasource),
+      network_provider.Pass());
 }
 
 void RenderFrameImpl::didStartProvisionalLoad(blink::WebLocalFrame* frame,
@@ -3332,17 +3353,20 @@ void RenderFrameImpl::willSendRequest(
       ServiceWorkerNetworkProvider* provider =
           ServiceWorkerNetworkProvider::FromDocumentState(
               DocumentState::FromDataSource(frame->provisionalDataSource()));
-      provider_id = provider->provider_id();
+      if (provider)
+        provider_id = provider->provider_id();
     }
   } else if (frame->dataSource()) {
     ServiceWorkerNetworkProvider* provider =
         ServiceWorkerNetworkProvider::FromDocumentState(
             DocumentState::FromDataSource(frame->dataSource()));
-    provider_id = provider->provider_id();
-    // Explicitly set the SkipServiceWorker flag here if the renderer process
-    // hasn't received SetControllerServiceWorker message.
-    if (!provider->IsControlledByServiceWorker())
-      request.setSkipServiceWorker(true);
+    if (provider) {
+      provider_id = provider->provider_id();
+      // Explicitly set the SkipServiceWorker flag here if the renderer process
+      // hasn't received SetControllerServiceWorker message.
+      if (!provider->IsControlledByServiceWorker())
+        request.setSkipServiceWorker(true);
+    }
   }
 
   WebFrame* parent = frame->parent();
@@ -4903,12 +4927,30 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request) {
   if (data_source && render_view_->history_list_length_ > 0) {
     should_replace_current_entry = data_source->replacesCurrentHistoryItem();
   }
+
+  DCHECK_EQ(FETCH_REQUEST_MODE_SAME_ORIGIN,
+            GetFetchRequestModeForWebURLRequest(*request));
+  DCHECK_EQ(FETCH_CREDENTIALS_MODE_INCLUDE,
+            GetFetchCredentialsModeForWebURLRequest(*request));
+  DCHECK(GetFetchRedirectModeForWebURLRequest(*request) ==
+         FetchRedirectMode::MANUAL_MODE);
+  DCHECK_IMPLIES(!frame_->parent(),
+                 GetRequestContextFrameTypeForWebURLRequest(*request) ==
+                 REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL);
+  DCHECK_IMPLIES(frame_->parent(),
+                 GetRequestContextFrameTypeForWebURLRequest(*request) ==
+                 REQUEST_CONTEXT_FRAME_TYPE_NESTED);
+
   Send(new FrameHostMsg_BeginNavigation(
       routing_id_,
       MakeCommonNavigationParams(request, should_replace_current_entry),
       BeginNavigationParams(
-          request->httpMethod().latin1(), GetWebURLRequestHeaders(*request),
-          GetLoadFlagsForWebURLRequest(*request), request->hasUserGesture()),
+          request->httpMethod().latin1(),
+          GetWebURLRequestHeaders(*request),
+          GetLoadFlagsForWebURLRequest(*request),
+          request->hasUserGesture(),
+          request->skipServiceWorker(),
+          GetRequestContextTypeForWebURLRequest(*request)),
       GetRequestBodyForWebURLRequest(*request)));
 }