DevTools: deprecate success field in responses

This deprecates two of the three "boolean success" fields in the protocol.
The errors are indicated through standard protocol means, in case
the method returns a non-error result, success is always set to true.

Change-Id: Iada79f5c50d08112a996dad192e1c3cfc86fe6ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425305
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810464}

content/browser/devtools/protocol/network_handler.cc

@@ -102,6 +102,8 @@ using DeleteCookiesCallback = Network::Backend::DeleteCookiesCallback;
 using ClearBrowserCookiesCallback =
     Network::Backend::ClearBrowserCookiesCallback;
 
+const char kInvalidCookieFields[] = "Invalid cookie fields";
+
 Network::CertificateTransparencyCompliance SerializeCTPolicyCompliance(
     net::ct::CTPolicyCompliance ct_compliance) {
   switch (ct_compliance) {
@@ -1186,9 +1188,7 @@ void NetworkHandler::SetCookie(const std::string& name,
       same_site.fromMaybe(""), expires.fromMaybe(-1), priority.fromMaybe(""));
 
   if (!cookie) {
-    // TODO(caseq): Current logic is for compatability only.
-    // Consider returning protocol error here.
-    callback->sendSuccess(false);
+    callback->sendFailure(Response::InvalidParams(kInvalidCookieFields));
     return;
   }
 
@@ -1260,7 +1260,7 @@ void NetworkHandler::SetCookies(
               callback->sendSuccess();
             } else {
               callback->sendFailure(
-                  Response::InvalidParams("Invalid cookie fields"));
+                  Response::InvalidParams(kInvalidCookieFields));
             }
           },
           std::move(callback)));

content/browser/devtools/protocol/target_handler.cc

@@ -866,7 +866,9 @@ Response TargetHandler::CloseTarget(const std::string& target_id,
       DevToolsAgentHost::GetForId(target_id);
   if (!agent_host)
     return Response::InvalidParams("No target with given id found");
-  *out_success = agent_host->Close();
+  if (!agent_host->Close())
+    return Response::InvalidParams("Specified target doesn't support closing");
+  *out_success = true;
   return Response::Success();
 }
 

third_party/blink/public/devtools_protocol/browser_protocol.pdl

@@ -5051,8 +5051,8 @@ domain Network
       # Cookie Priority type.
       experimental optional CookiePriority priority
     returns
-      # True if successfully set cookie.
-      boolean success
+      # Always set to true. If an error occurs, the response indicates protocol error.
+      deprecated boolean success
 
   # Sets given cookies.
   command setCookies
@@ -7434,7 +7434,8 @@ domain Target
     parameters
       TargetID targetId
     returns
-      boolean success
+      # Always set to true. If an error occurs, the response indicates protocol error.
+      deprecated boolean success
 
   # Inject object to the target's main frame that provides a communication
   # channel with browser target.

third_party/blink/web_tests/http/tests/inspector-protocol/cookies-protocol-test-expected.txt

@@ -4,19 +4,16 @@ Enabling network
 
 Running test: simpleCookieAdd
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar1, domain: 127.0.0.1, path: /, session
 
 Running test: simpleCookieChange
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: second bar2, domain: 127.0.0.1, path: /, session
 
 Running test: anotherSimpleCookieAdd
 Setting Cookie
-Success: true
 Num of cookies 2
 name: foo, value: second bar2, domain: 127.0.0.1, path: /, session
 name: foo2, value: bar1, domain: 127.0.0.1, path: /, session
@@ -30,7 +27,6 @@ Running test: deleteAllCookies
 
 Running test: sessionCookieAdd
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar4, domain: 127.0.0.1, path: /, session
 
@@ -38,14 +34,12 @@ Running test: deleteAllCookies
 
 Running test: nonSessionCookieZeroAdd
 Setting Cookie
-Success: true
 Num of cookies 0
 
 Running test: deleteAllCookies
 
 Running test: nonSessionCookieAdd
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar6, domain: 127.0.0.1, path: /, expires
 
@@ -53,7 +47,6 @@ Running test: deleteAllCookies
 
 Running test: differentOriginCookieAdd
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar7, domain: example.com, path: /, session
 
@@ -61,21 +54,20 @@ Running test: deleteAllCookies
 
 Running test: invalidCookieAddDomain
 Setting Cookie
-Success: false
+setCookie failed: Invalid cookie fields
 Num of cookies 0
 
 Running test: deleteAllCookies
 
 Running test: invalidCookieAddName
 Setting Cookie
-Success: false
+setCookie failed: Invalid cookie fields
 Num of cookies 0
 
 Running test: deleteAllCookies
 
 Running test: secureCookieAdd
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar, domain: 127.0.0.1, path: /, secure, session
 
@@ -83,7 +75,6 @@ Running test: deleteAllCookies
 
 Running test: cookieAddHttpOnly
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar, domain: 127.0.0.1, path: /, httpOnly, session
 
@@ -91,7 +82,6 @@ Running test: deleteAllCookies
 
 Running test: cookieAddSameSiteLax
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar, domain: 127.0.0.1, path: /, session, Lax
 
@@ -99,7 +89,6 @@ Running test: deleteAllCookies
 
 Running test: cookieAddSameSiteLax
 Setting Cookie
-Success: true
 Num of cookies 1
 name: foo, value: bar, domain: 127.0.0.1, path: /, session, Strict
 

third_party/blink/web_tests/http/tests/inspector-protocol/cookies-protocol-test.js

@@ -2,9 +2,7 @@
   var {page, session, dp} = await testRunner.startBlank(
       `Tests that cookies are set, updated and removed.`);
 
-  async function logCookies(success) {
-    if (success !== undefined)
-      testRunner.log('Success: ' + success);
+  async function logCookies() {
     var data = (await dp.Network.getAllCookies()).result;
     testRunner.log('Num of cookies ' + data.cookies.length);
     data.cookies.sort((a, b) => a.name.localeCompare(b.name));
@@ -27,7 +25,9 @@
   async function setCookie(cookie) {
     testRunner.log('Setting Cookie');
     var response = await dp.Network.setCookie(cookie);
-    await logCookies(response.result.success);
+    if (response.error)
+      testRunner.log(`setCookie failed: ${response.error.message}`);
+    await logCookies();
   }
 
   async function deleteCookie(cookie) {