bluetooth: show better error messages for services, characteristics and descriptors

third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp

Index: third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
diff --git a/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp b/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
index 383649d5fa405a3c9da61f73c70abbcee2f0525a..a3a582380bfbea6fe730b1331fc2762107547419 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
@@ -9,15 +9,42 @@
 
 namespace blink {
 
-DOMException* BluetoothError::take(ScriptPromiseResolver*,
-                                   mojom::blink::WebBluetoothResult error) {
+// static
+DOMException* BluetoothError::createDOMException(
+    mojom::blink::WebBluetoothResult error,
+    const String& detailedMessage) {
   switch (error) {
     case mojom::blink::WebBluetoothResult::SUCCESS:
-      ASSERT_NOT_REACHED();
+      NOTREACHED();
+      return DOMException::create(UnknownError);
+    case mojom::blink::WebBluetoothResult::SERVICE_NOT_FOUND:
+      return DOMException::create(NotFoundError, detailedMessage);
+    case mojom::blink::WebBluetoothResult::CHARACTERISTIC_NOT_FOUND:
+      return DOMException::create(NotFoundError, detailedMessage);
+    case mojom::blink::WebBluetoothResult::DESCRIPTOR_NOT_FOUND:
+      return DOMException::create(NotFoundError, detailedMessage);
+    case mojom::blink::WebBluetoothResult::INVALID_SERVICE:
+      return DOMException::create(InvalidStateError, detailedMessage);
+    case mojom::blink::WebBluetoothResult::INVALID_CHARACTERISTIC:
+      return DOMException::create(InvalidStateError, detailedMessage);
+    case mojom::blink::WebBluetoothResult::INVALID_DESCRIPTOR:
+      return DOMException::create(InvalidStateError, detailedMessage);
+    default:
+      NOTREACHED();
+      return DOMException::create(UnknownError);
+  }
+}
+
+// static
+DOMException* BluetoothError::createDOMException(
+    mojom::blink::WebBluetoothResult error) {
+  switch (error) {
+    case mojom::blink::WebBluetoothResult::SUCCESS:
+      NOTREACHED();
       return DOMException::create(UnknownError);
 #define MAP_ERROR(enumeration, name, message)         \
   case mojom::blink::WebBluetoothResult::enumeration: \
-    return DOMException::create(name, message)
+    return DOMException::create(name, message);
 
       // InvalidModificationErrors:
       MAP_ERROR(GATT_INVALID_ATTRIBUTE_LENGTH, InvalidModificationError,
@@ -30,6 +57,15 @@ DOMException* BluetoothError::take(ScriptPromiseResolver*,
                 "GATT Characteristic no longer exists.");
       MAP_ERROR(DESCRIPTOR_NO_LONGER_EXISTS, InvalidStateError,
                 "GATT Descriptor no longer exists.");
+      MAP_ERROR(INVALID_CHARACTERISTIC, InvalidStateError,
+                "Characteristic is no longer valid. Remember to retrieve the "
+                "characteristic again after reconnecting.");
+      MAP_ERROR(INVALID_DESCRIPTOR, InvalidStateError,
+                "Descriptor is no longer valid. Remember to retrieve the "
+                "Descriptor again after reconnecting.");
+      MAP_ERROR(INVALID_SERVICE, InvalidStateError,

[scheib, 2017/02/10 18:25:34]

This new pattern does not LGTM. (Sorry for noticing this late.) It creates case
handlers that won't be executed, and in this file need to either be
speculatively filled in or runtime checked with NOTREACHED(). We end up
duplicating handlers e.g. for INVALID_SERVICE.

I suggest we split the mojom enum WebBluetoothResult into two:
enum WebBluetoothResult
enum WebBluetoothResultWithMessage
and each handler will know precisely how to handle that enum. I think all enums
will fall into only one or the other.

We should avoid switch statements having a "default:" handler, so that the
compiler can help us verify that all cases are handled.

+                "Service is no longer valid. Remember to retrieve the service "
+                "again after reconnecting.");
 
       // NetworkErrors:
       MAP_ERROR(CONNECT_ALREADY_IN_PROGRESS, NetworkError,
@@ -68,6 +104,17 @@ DOMException* BluetoothError::take(ScriptPromiseResolver*,
                 "GATT operation already in progress.");
       MAP_ERROR(UNTRANSLATED_CONNECT_ERROR_CODE, NetworkError,
                 "Unknown ConnectErrorCode.");
+      MAP_ERROR(GATT_SERVER_NOT_CONNECTED, NetworkError,
+                "GATT Server is disconnected. Cannot perform GATT operations.");
+      MAP_ERROR(GATT_SERVER_DISCONNECTED, NetworkError,
+                "GATT Server disconnected while performing a GATT operation.");
+      MAP_ERROR(GATT_SERVER_DISCONNECTED_WHILE_RETRIEVING_CHARACTERISTICS,
+                NetworkError,
+                "GATT Server disconnected while retrieving characteristics.");
+      MAP_ERROR(
+          GATT_SERVER_NOT_CONNECTED_CANNOT_RETRIEVE_CHARACTERISTICS,
+          NetworkError,
+          "GATT Server is disconnected. Cannot retrieve characteristics.");
 
       // NotFoundErrors:
       MAP_ERROR(WEB_BLUETOOTH_NOT_SUPPORTED, NotFoundError,
@@ -146,7 +193,7 @@ DOMException* BluetoothError::take(ScriptPromiseResolver*,
 #undef MAP_ERROR
   }
 
-  ASSERT_NOT_REACHED();
+  NOTREACHED();
   return DOMException::create(UnknownError);
 }