permissions: refactor Permissions API in blink to vastly reduce duplication

third_party/WebKit/Source/modules/permissions/Permissions.cpp

Index: third_party/WebKit/Source/modules/permissions/Permissions.cpp
diff --git a/third_party/WebKit/Source/modules/permissions/Permissions.cpp b/third_party/WebKit/Source/modules/permissions/Permissions.cpp
index 16a05dcc3699c1541718241bad9e05e0606e116d..d777ce32b5bb6528ae6c2c42ba6645893e2bca9f 100644
--- a/third_party/WebKit/Source/modules/permissions/Permissions.cpp
+++ b/third_party/WebKit/Source/modules/permissions/Permissions.cpp
@@ -6,6 +6,7 @@
 #include "modules/permissions/Permissions.h"
 
 #include "bindings/core/v8/Dictionary.h"
+#include "bindings/core/v8/Nullable.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/modules/v8/V8MidiPermissionDescriptor.h"
@@ -27,24 +28,7 @@ namespace blink {
 
 namespace {
 
-// Here we eagerly reject any permissions that we do not support.
-// If the permission is handled here, it will not be propogated up to the content layer.
-// The return value indicates if the permissions has been handled by the funciton.
-bool handleNotSupportedPermission(
-    ScriptState* scriptState, const Dictionary& rawPermission, ScriptPromiseResolver* resolver, WebPermissionType type, TrackExceptionState& exceptionState)
-{
-    if (type == WebPermissionTypePushNotifications) {
-        PushPermissionDescriptor pushPermission = NativeValueTraits<PushPermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
-        // Only "userVisibleOnly" push is supported for now.
-        if (!pushPermission.userVisibleOnly()) {
-            resolver->reject(DOMException::create(NotSupportedError, "Push Permission without userVisibleOnly:true isn't supported yet."));
-            return true;
-        }
-    }
-    return false;
-}
-
-WebPermissionType getPermissionType(ScriptState* scriptState, const Dictionary& rawPermission, const PermissionDescriptor& permission, TrackExceptionState& exceptionState)
+WebPermissionType getPermissionType(ScriptState* scriptState, const Dictionary& rawPermission, const PermissionDescriptor& permission, ExceptionState& exceptionState)
 {
     const String& name = permission.name();
     if (name == "geolocation")
@@ -62,6 +46,39 @@ WebPermissionType getPermissionType(ScriptState* scriptState, const Dictionary&
     return WebPermissionTypeGeolocation;
 }
 
+Nullable<WebPermissionType> parsePermission(ScriptState* scriptState, const Dictionary rawPermission, ExceptionState& exceptionState)

[mlamouri (slow - plz ping), 2015/09/28 10:48:43]

This need some documentation :)

[Lalit Maganti, 2015/09/28 12:23:09]

Done.

+{
+    PermissionDescriptor permission = NativeValueTraits<PermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
+
+    if (exceptionState.hadException()) {
+        exceptionState.throwTypeError(exceptionState.message());
+        return Nullable<WebPermissionType>();
+    }
+
+    WebPermissionType type = getPermissionType(scriptState, rawPermission, permission, exceptionState);
+    if (exceptionState.hadException()) {
+        exceptionState.throwTypeError(exceptionState.message());
+        return Nullable<WebPermissionType>();
+    }
+
+    // Here we reject any permissions which are not yet supported by Blink.
+    if (type == WebPermissionTypePushNotifications) {
+        PushPermissionDescriptor pushPermission = NativeValueTraits<PushPermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
+        if (exceptionState.hadException()) {
+            exceptionState.throwTypeError(exceptionState.message());
+            return Nullable<WebPermissionType>();
+        }
+
+        // Only "userVisibleOnly" push is supported for now.
+        if (!pushPermission.userVisibleOnly()) {
+            exceptionState.throwDOMException(NotSupportedError, "Push Permission without userVisibleOnly:true isn't supported yet.");
+            return Nullable<WebPermissionType>();
+        }
+    }
+
+    return Nullable<WebPermissionType>(type);
+}
+
 } // anonymous namespace
 
 // static
@@ -83,24 +100,19 @@ ScriptPromise Permissions::query(ScriptState* scriptState, const Dictionary& raw
     if (!client)
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "In its current state, the global scope can't query permissions."));
 
-    TrackExceptionState exceptionState;
-    PermissionDescriptor permission = NativeValueTraits<PermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
-
+    ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr, scriptState->context()->Global(), scriptState->isolate());

[mlamouri (slow - plz ping), 2015/09/28 10:48:43]

Why not passing: |ExceptionState::GetterContext, "query", "Permissions"| ?

[Lalit Maganti, 2015/09/28 12:23:09]

Makes a lot of sense. I scoured code search to figure out what I should do here
but no one seemed to do this "properly".

+    Nullable<WebPermissionType> type = parsePermission(scriptState, rawPermission, exceptionState);
     if (exceptionState.hadException())
-        return ScriptPromise::reject(scriptState, v8::Exception::TypeError(v8String(scriptState->isolate(), exceptionState.message())));
+        return exceptionState.reject(scriptState);
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    WebPermissionType type = getPermissionType(scriptState, rawPermission, permission, exceptionState);
-    if (handleNotSupportedPermission(scriptState, rawPermission, resolver, type, exceptionState))
-        return promise;
-
     // If the current origin is a file scheme, it will unlikely return a
     // meaningful value because most APIs are broken on file scheme and no
     // permission prompt will be shown even if the returned permission will most
     // likely be "prompt".
-    client->queryPermission(type, KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new PermissionCallback(resolver, type));
+    client->queryPermission(type.get(), KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new PermissionCallback(resolver, type.get()));
     return promise;
 }
 
@@ -110,20 +122,15 @@ ScriptPromise Permissions::request(ScriptState* scriptState, const Dictionary& r
     if (!client)
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "In its current state, the global scope can't request permissions."));
 
-    TrackExceptionState exceptionState;
-    PermissionDescriptor permission = NativeValueTraits<PermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
-
+    ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr, scriptState->context()->Global(), scriptState->isolate());

[mlamouri (slow - plz ping), 2015/09/28 10:48:43]

ditto with "request"

[Lalit Maganti, 2015/09/28 12:23:09]

Done.

+    Nullable<WebPermissionType> type = parsePermission(scriptState, rawPermission, exceptionState);

[mlamouri (slow - plz ping), 2015/09/28 10:48:43]

So here you use |Nullable<>| but you don't actually use the fact that |type|
could be null. You only return null if there is an exception. It's a bit odd.
Could you add least add a comment explaining that |Nullable<>| will be null iif
an exception was thrown and an ASSERT() will be fired if the caller do not check
for the permission and uses a null type? (You can add that in the
|parsePermission()| method comment.

[Lalit Maganti, 2015/09/28 12:23:09]

Done.

     if (exceptionState.hadException())
-        return ScriptPromise::reject(scriptState, v8::Exception::TypeError(v8String(scriptState->isolate(), exceptionState.message())));
+        return exceptionState.reject(scriptState);
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    WebPermissionType type = getPermissionType(scriptState, rawPermission, permission, exceptionState);
-    if (handleNotSupportedPermission(scriptState, rawPermission, resolver, type, exceptionState))
-        return promise;
-
-    client->requestPermission(type, KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new PermissionCallback(resolver, type));
+    client->requestPermission(type.get(), KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new PermissionCallback(resolver, type.get()));
     return promise;
 }
 
@@ -133,28 +140,21 @@ ScriptPromise Permissions::request(ScriptState* scriptState, const Vector<Dictio
     if (!client)
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "In its current state, the global scope can't request permissions."));
 
-    TrackExceptionState exceptionState;
+    ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr, scriptState->context()->Global(), scriptState->isolate());

[mlamouri (slow - plz ping), 2015/09/28 10:48:43]

ditto with "request"

[Lalit Maganti, 2015/09/28 12:23:09]

Done.

     OwnPtr<WebVector<WebPermissionType>> permissions = adoptPtr(new WebVector<WebPermissionType>(rawPermissions.size()));
-
     for (size_t i = 0; i < rawPermissions.size(); ++i) {
         const Dictionary& rawPermission = rawPermissions[i];
-        PermissionDescriptor permission = NativeValueTraits<PermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
 
+        Nullable<WebPermissionType> type = parsePermission(scriptState, rawPermission, exceptionState);
         if (exceptionState.hadException())
-            return ScriptPromise::reject(scriptState, v8::Exception::TypeError(v8String(scriptState->isolate(), exceptionState.message())));
+            return exceptionState.reject(scriptState);
 
-        permissions->operator[](i) = getPermissionType(scriptState, rawPermission, permission, exceptionState);
+        permissions->operator[](i) = type.get();
     }
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    // We need to do this is a separate loop because we can't create the Resolver and Promise untile we are clear of all parsing/type errors.
-    for (size_t i = 0; i < rawPermissions.size(); ++i) {
-        if (handleNotSupportedPermission(scriptState, rawPermissions[i], resolver, (*permissions)[i], exceptionState))
-            return promise;
-    }
-
     client->requestPermissions(*permissions, KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new PermissionsCallback(resolver, permissions.release()));
     return promise;
 }
@@ -165,20 +165,15 @@ ScriptPromise Permissions::revoke(ScriptState* scriptState, const Dictionary& ra
     if (!client)
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "In its current state, the global scope can't revoke permissions."));
 
-    TrackExceptionState exceptionState;
-    PermissionDescriptor permission = NativeValueTraits<PermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
-
+    ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr, scriptState->context()->Global(), scriptState->isolate());

[mlamouri (slow - plz ping), 2015/09/28 10:48:43]

ditto with "revoke"

[Lalit Maganti, 2015/09/28 12:23:09]

Done.

+    Nullable<WebPermissionType> type = parsePermission(scriptState, rawPermission, exceptionState);
     if (exceptionState.hadException())
-        return ScriptPromise::reject(scriptState, v8::Exception::TypeError(v8String(scriptState->isolate(), exceptionState.message())));
+        return exceptionState.reject(scriptState);
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    WebPermissionType type = getPermissionType(scriptState, rawPermission, permission, exceptionState);
-    if (handleNotSupportedPermission(scriptState, rawPermission, resolver, type, exceptionState))
-        return promise;
-
-    client->revokePermission(type, KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new PermissionCallback(resolver, type));
+    client->revokePermission(type.get(), KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new PermissionCallback(resolver, type.get()));
     return promise;
 }