[ServiceWorker] Fix bugs of wpt tests for Client.navigate

content/browser/service_worker/service_worker_client_utils.cc

 // 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.
 
 #include "content/browser/service_worker/service_worker_client_utils.h"
 
 #include <algorithm>
 #include <tuple>
 
 #include "base/location.h"
@@ skipping 41 matching lines @@
  public:
   OpenURLObserver(WebContents* web_contents,
                   int frame_tree_node_id,
                   const OpenURLCallback& callback)
       : WebContentsObserver(web_contents),
         frame_tree_node_id_(frame_tree_node_id),
         callback_(callback) {}
 
   void DidFinishNavigation(NavigationHandle* navigation_handle) override {
     DCHECK(web_contents());
-    if (!navigation_handle->HasCommitted())
+    if (!navigation_handle->HasCommitted()) {

[falken, 2017/06/12 01:50:14]

Add a comment like:
// Return error.

[leonhsl(Using Gerrit), 2017/06/12 07:18:13]

Done.

+      RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE);

[leonhsl(Using Gerrit), 2017/06/10 02:57:44]

DidFinishNavigation() should definitely fire the callback even in error case,
otherwise the callback will be pending.

Based on comments at line48,49 of this file, pass (kInvalidUniqueID,
MSG_ROUTING_NONE) back to indicate navigation error, this way will finally
trigger Blink side to throw type error at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

[falken, 2017/06/12 01:50:14]

According to HasCommitted() documentation, this also returns false if the
navigation ended up being a download or 204/205 response. It also  returns true
if the navigation resulted in an error page.

Do you know if we have test coverage for these kinds of navigations?

I'm guessing our error handling for OpenWindow will need more rigor and testing
but this looks OK as a step in the right direction.

[leonhsl(Using Gerrit), 2017/06/12 07:18:12]

I checked through but did not find any wpt tests against Client.navigate() for
'download or 204/205 response or displayed an error page', and seems there are't
any wpts tests against Clients.openWindow().

For both Client.navigate() and Clients.openWindow(), I suppose the expected
behavior should be as following?
  - download:          throw type error, because no new page display, no Client
will be returned back
  - 204/205 response:  throw type error, because no new page display, no Client
will be returned back
  - error page:        success
WDYT? Thanks.

[falken, 2017/06/12 07:27:26]

Not sure if error pages should be clients.

But we could follow up on the spec and follow-up bugs.

       return;
+    }
 
-    if (navigation_handle->GetFrameTreeNodeId() != frame_tree_node_id_)
+    if (navigation_handle->GetFrameTreeNodeId() != frame_tree_node_id_) {

[falken, 2017/06/12 01:50:14]

// Return error.

[leonhsl(Using Gerrit), 2017/06/12 07:18:12]

Done.

+      RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE);
       return;
+    }
 
     RenderFrameHost* render_frame_host =
         navigation_handle->GetRenderFrameHost();
     RunCallback(render_frame_host->GetProcess()->GetID(),
                 render_frame_host->GetRoutingID());
   }
 
   void RenderProcessGone(base::TerminationStatus status) override {
     RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE);
   }
@@ skipping 459 matching lines @@
       options.client_type == blink::kWebServiceWorkerClientTypeAll) {
     GetWindowClients(controller, options, callback, std::move(clients));
     return;
   }
 
   GetNonWindowClients(controller, options, callback, std::move(clients));
 }
 
 }  // namespace service_worker_client_utils
 }  // namespace content