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

content/browser/service_worker/service_worker_client_utils.cc

Index: content/browser/service_worker/service_worker_client_utils.cc
diff --git a/content/browser/service_worker/service_worker_client_utils.cc b/content/browser/service_worker/service_worker_client_utils.cc
index 487e85b24fd1b417b7d4542d3490c913cdfc52bc..3a65a39b03f7292fad4a14d04f0daf97a9edc045 100644
--- a/content/browser/service_worker/service_worker_client_utils.cc
+++ b/content/browser/service_worker/service_worker_client_utils.cc
@@ -59,11 +59,15 @@ class OpenURLObserver : public WebContentsObserver {
 
   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();