Add PermissionDescriptor to the permissions Mojo interface.

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 08781a8b9649b49d8dfccc72a4298592df79b773..87e59957d2c0d27e9f6d44de54feeda31af22e1d 100644
--- a/third_party/WebKit/Source/modules/permissions/Permissions.cpp
+++ b/third_party/WebKit/Source/modules/permissions/Permissions.cpp
@@ -17,8 +17,8 @@
 #include "core/frame/LocalFrame.h"
 #include "modules/permissions/PermissionDescriptor.h"
 #include "modules/permissions/PermissionStatus.h"
+#include "modules/permissions/PermissionUtils.h"
 #include "platform/UserGestureIndicator.h"
-#include "public/platform/InterfaceProvider.h"
 #include "public/platform/Platform.h"
 #include "wtf/Functional.h"
 #include "wtf/NotFound.h"
@@ -28,95 +28,68 @@
 
 namespace blink {
 
+using mojom::blink::PermissionDescriptorPtr;
 using mojom::blink::PermissionName;
 using mojom::blink::PermissionService;
 
 namespace {
 
+// Parses the raw permission dictionary and returns the Mojo
+// PermissionDescriptor if parsing was successful. If an exception occurs, it
+// will be stored in |exceptionState| and null will be returned. Therefore, the
+// |exceptionState| should be checked before attempting to use the returned
+// permission as the non-null assert will be fired otherwise.
+//
 // Websites will be able to run code when `name()` is called, changing the
 // current context. The caller should make sure that no assumption is made
 // after this has been called.
-PermissionName getPermissionName(ScriptState* scriptState, const Dictionary& rawPermission, const PermissionDescriptor& permission, ExceptionState& exceptionState)
-{
-    const String& name = permission.name();
-    if (name == "geolocation")
-        return PermissionName::GEOLOCATION;
-    if (name == "notifications")
-        return PermissionName::NOTIFICATIONS;
-    if (name == "push")
-        return PermissionName::PUSH_NOTIFICATIONS;
-    if (name == "midi") {
-        MidiPermissionDescriptor midiPermission = NativeValueTraits<MidiPermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
-        return midiPermission.sysex() ? PermissionName::MIDI_SYSEX : PermissionName::MIDI;
-    }
-    if (name == "background-sync")
-        return PermissionName::BACKGROUND_SYNC;
-
-    ASSERT_NOT_REACHED();
-    return PermissionName::GEOLOCATION;
-}
-
-// Parses the raw permission dictionary and returns the PermissionType if
-// parsing was successful. If an exception occurs, it will be stored in
-// |exceptionState| and null will be returned. Therefore, the |exceptionState|
-// should be checked before attempting to use the returned permission as the
-// non-null assert will be fired otherwise.
-Nullable<PermissionName> parsePermission(ScriptState* scriptState, const Dictionary rawPermission, ExceptionState& exceptionState)
+PermissionDescriptorPtr parsePermission(ScriptState* scriptState, const Dictionary rawPermission, ExceptionState& exceptionState)
 {
     PermissionDescriptor permission = NativeValueTraits<PermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
 
     if (exceptionState.hadException()) {
         exceptionState.throwTypeError(exceptionState.message());
-        return Nullable<PermissionName>();
-    }
-
-    PermissionName name = getPermissionName(scriptState, rawPermission, permission, exceptionState);
-    if (exceptionState.hadException()) {
-        exceptionState.throwTypeError(exceptionState.message());
-        return Nullable<PermissionName>();
+        return nullptr;
     }
 
-    // Here we reject any permissions which are not yet supported by Blink.
-    if (name == PermissionName::PUSH_NOTIFICATIONS) {
+    const String& name = permission.name();
+    PermissionName permissionName;
+    if (name == "geolocation") {
+        permissionName = PermissionName::GEOLOCATION;
+    } else if (name == "notifications") {
+        permissionName = PermissionName::NOTIFICATIONS;
+    } else if (name == "push") {
         PushPermissionDescriptor pushPermission = NativeValueTraits<PushPermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
         if (exceptionState.hadException()) {
             exceptionState.throwTypeError(exceptionState.message());
-            return Nullable<PermissionName>();
+            return nullptr;
         }
 
         // Only "userVisibleOnly" push is supported for now.
         if (!pushPermission.userVisibleOnly()) {
             exceptionState.throwDOMException(NotSupportedError, "Push Permission without userVisibleOnly:true isn't supported yet.");
-            return Nullable<PermissionName>();
+            return nullptr;
         }
-    }
-
-    return Nullable<PermissionName>(name);
-}
 
-} // anonymous namespace
-
-// static
-bool Permissions::connectToService(ExecutionContext* executionContext, mojom::blink::PermissionServiceRequest request)
-{
-    InterfaceProvider* interfaceProvider = nullptr;
-    if (executionContext->isDocument()) {
-        Document* document = toDocument(executionContext);
-        if (document->frame())
-            interfaceProvider = document->frame()->interfaceProvider();
+        permissionName = PermissionName::PUSH_NOTIFICATIONS;
+    } else if (name == "midi") {
+        MidiPermissionDescriptor midiPermission = NativeValueTraits<MidiPermissionDescriptor>::nativeValue(scriptState->isolate(), rawPermission.v8Value(), exceptionState);
+        return createMidiPermissionDescriptor(midiPermission.sysex());
+    } else if (name == "background-sync") {
+        permissionName = PermissionName::BACKGROUND_SYNC;
     } else {
-        interfaceProvider = Platform::current()->interfaceProvider();
+        return nullptr;
     }
 
-    if (interfaceProvider)
-        interfaceProvider->getInterface(std::move(request));
-    return interfaceProvider;
+    return createPermissionDescriptor(permissionName);

[mlamouri (slow - plz ping), 2016/09/13 09:57:50]

I find it a bit confusing that some if statements end up returning and some will
set `permissionName` which is used here for the final return. Given that this
helper is a one-liner, can we make this a bit easier to read with a flow like:

```
if (name == "geolocation")
  return createPermissionDescriptor(PermissionName::GEOLOCATION)

if (name == "notifications")
  return createPermissionDescriptor(PermissionName::NOTIFICATIONS)

[...]
```

[Reilly Grant (use Gerrit), 2016/09/27 10:34:28]

Done. You're right, that's cleaner.

 }
 
+} // anonymous namespace
+
 ScriptPromise Permissions::query(ScriptState* scriptState, const Dictionary& rawPermission)
 {
     ExceptionState exceptionState(ExceptionState::GetterContext,  "query", "Permissions", scriptState->context()->Global(), scriptState->isolate());
-    Nullable<PermissionName> name = parsePermission(scriptState, rawPermission, exceptionState);
+    auto descriptor = parsePermission(scriptState, rawPermission, exceptionState);

[mlamouri (slow - plz ping), 2016/09/13 09:57:50]

can you explicitly put `PermissionDescriptor` instead of `auto`?

[Reilly Grant (use Gerrit), 2016/09/27 10:34:28]

Done.

     if (exceptionState.hadException())
         return exceptionState.reject(scriptState);
 
@@ -133,14 +106,15 @@ ScriptPromise Permissions::query(ScriptState* scriptState, const Dictionary& raw
     // 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".
-    service->HasPermission(name.get(), scriptState->getExecutionContext()->getSecurityOrigin(), convertToBaseCallback(WTF::bind(&Permissions::taskComplete, wrapPersistent(this), wrapPersistent(resolver), name.get())));
+    auto descriptorCopy = descriptor->Clone();

[mlamouri (slow - plz ping), 2016/09/13 09:57:50]

ditto and all the instances below.

[Reilly Grant (use Gerrit), 2016/09/27 10:34:28]

Done.

+    service->HasPermission(std::move(descriptor), scriptState->getExecutionContext()->getSecurityOrigin(), convertToBaseCallback(WTF::bind(&Permissions::taskComplete, wrapPersistent(this), wrapPersistent(resolver), passed(std::move(descriptorCopy)))));
     return promise;
 }
 
 ScriptPromise Permissions::request(ScriptState* scriptState, const Dictionary& rawPermission)
 {
     ExceptionState exceptionState(ExceptionState::GetterContext,  "request", "Permissions", scriptState->context()->Global(), scriptState->isolate());
-    Nullable<PermissionName> name = parsePermission(scriptState, rawPermission, exceptionState);
+    auto descriptor = parsePermission(scriptState, rawPermission, exceptionState);
     if (exceptionState.hadException())
         return exceptionState.reject(scriptState);
 
@@ -153,14 +127,15 @@ ScriptPromise Permissions::request(ScriptState* scriptState, const Dictionary& r
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    service->RequestPermission(name.get(), scriptState->getExecutionContext()->getSecurityOrigin(), UserGestureIndicator::processingUserGesture(), convertToBaseCallback(WTF::bind(&Permissions::taskComplete, wrapPersistent(this), wrapPersistent(resolver), name.get())));
+    auto descriptorCopy = descriptor->Clone();
+    service->RequestPermission(std::move(descriptor), scriptState->getExecutionContext()->getSecurityOrigin(), UserGestureIndicator::processingUserGesture(), convertToBaseCallback(WTF::bind(&Permissions::taskComplete, wrapPersistent(this), wrapPersistent(resolver), passed(std::move(descriptorCopy)))));
     return promise;
 }
 
 ScriptPromise Permissions::revoke(ScriptState* scriptState, const Dictionary& rawPermission)
 {
     ExceptionState exceptionState(ExceptionState::GetterContext,  "revoke", "Permissions", scriptState->context()->Global(), scriptState->isolate());
-    Nullable<PermissionName> name = parsePermission(scriptState, rawPermission, exceptionState);
+    auto descriptor = parsePermission(scriptState, rawPermission, exceptionState);
     if (exceptionState.hadException())
         return exceptionState.reject(scriptState);
 
@@ -173,32 +148,35 @@ ScriptPromise Permissions::revoke(ScriptState* scriptState, const Dictionary& ra
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    service->RevokePermission(name.get(), scriptState->getExecutionContext()->getSecurityOrigin(), convertToBaseCallback(WTF::bind(&Permissions::taskComplete, wrapPersistent(this), wrapPersistent(resolver), name.get())));
+    auto descriptorCopy = descriptor->Clone();
+    service->RevokePermission(std::move(descriptor), scriptState->getExecutionContext()->getSecurityOrigin(), convertToBaseCallback(WTF::bind(&Permissions::taskComplete, wrapPersistent(this), wrapPersistent(resolver), passed(std::move(descriptorCopy)))));
     return promise;
 }
 
 ScriptPromise Permissions::requestAll(ScriptState* scriptState, const Vector<Dictionary>& rawPermissions)
 {
     ExceptionState exceptionState(ExceptionState::GetterContext,  "request", "Permissions", scriptState->context()->Global(), scriptState->isolate());
-    Vector<PermissionName> internalPermissions;
+    Vector<PermissionDescriptorPtr> internalPermissions;
     Vector<int> callerIndexToInternalIndex;
     callerIndexToInternalIndex.resize(rawPermissions.size());
     for (size_t i = 0; i < rawPermissions.size(); ++i) {
         const Dictionary& rawPermission = rawPermissions[i];
 
-        Nullable<PermissionName> name = parsePermission(scriptState, rawPermission, exceptionState);
+        auto descriptor = parsePermission(scriptState, rawPermission, exceptionState);
         if (exceptionState.hadException())
             return exceptionState.reject(scriptState);
 
-        // Only append permissions to the vector that is passed to the client if it is not already
-        // in the vector (i.e. do not duplicate permisison types).
-        int internalIndex;
-        auto it = internalPermissions.find(name.get());
-        if (it == kNotFound) {
+        // Only append permissions types that are not already present in the vector.
+        int internalIndex = -1;

[mlamouri (slow - plz ping), 2016/09/13 09:57:50]

What about using `kNotFound`?

[Reilly Grant (use Gerrit), 2016/09/27 10:34:28]

I didn't know kNotFound was a common type. Neat.

+        for (size_t j = 0; j < internalPermissions.size(); ++j) {
+            if (internalPermissions[j]->name == descriptor->name) {
+                internalIndex = j;
+                break;
+            }
+        }
+        if (internalIndex == -1) {

[mlamouri (slow - plz ping), 2016/09/13 09:57:50]

... and here you can do `if (internalIndex == kNotFound)`

[Reilly Grant (use Gerrit), 2016/09/27 10:34:28]

Done.

             internalIndex = internalPermissions.size();
-            internalPermissions.append(name.get());
-        } else {
-            internalIndex = it;
+            internalPermissions.append(std::move(descriptor));
         }
         callerIndexToInternalIndex[i] = internalIndex;
     }
@@ -212,14 +190,19 @@ ScriptPromise Permissions::requestAll(ScriptState* scriptState, const Vector<Dic
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    service->RequestPermissions(internalPermissions, scriptState->getExecutionContext()->getSecurityOrigin(), UserGestureIndicator::processingUserGesture(),
-        convertToBaseCallback(WTF::bind(&Permissions::batchTaskComplete, wrapPersistent(this), wrapPersistent(resolver), internalPermissions, callerIndexToInternalIndex)));
+    Vector<PermissionDescriptorPtr> internalPermissionsCopy;
+    internalPermissionsCopy.reserveCapacity(internalPermissions.size());
+    for (const auto& descriptor : internalPermissions)
+        internalPermissionsCopy.append(descriptor->Clone());
+
+    service->RequestPermissions(std::move(internalPermissions), scriptState->getExecutionContext()->getSecurityOrigin(), UserGestureIndicator::processingUserGesture(),
+        convertToBaseCallback(WTF::bind(&Permissions::batchTaskComplete, wrapPersistent(this), wrapPersistent(resolver), passed(std::move(internalPermissionsCopy)), passed(std::move(callerIndexToInternalIndex)))));
     return promise;
 }
 
 PermissionService* Permissions::getService(ExecutionContext* executionContext)
 {
-    if (!m_service && connectToService(executionContext, mojo::GetProxy(&m_service)))
+    if (!m_service && connectToPermissionService(executionContext, mojo::GetProxy(&m_service)))
         m_service.set_connection_error_handler(convertToBaseCallback(WTF::bind(&Permissions::serviceConnectionError, wrapWeakPersistent(this))));
     return m_service.get();
 }
@@ -229,14 +212,14 @@ void Permissions::serviceConnectionError()
     m_service.reset();
 }
 
-void Permissions::taskComplete(ScriptPromiseResolver* resolver, PermissionName name, mojom::blink::PermissionStatus result)
+void Permissions::taskComplete(ScriptPromiseResolver* resolver, mojom::blink::PermissionDescriptorPtr descriptor, mojom::blink::PermissionStatus result)
 {
     if (!resolver->getExecutionContext() || resolver->getExecutionContext()->activeDOMObjectsAreStopped())
         return;
-    resolver->resolve(PermissionStatus::take(resolver, result, name));
+    resolver->resolve(PermissionStatus::take(resolver, result, std::move(descriptor)));
 }
 
-void Permissions::batchTaskComplete(ScriptPromiseResolver* resolver, Vector<PermissionName> names, Vector<int> callerIndexToInternalIndex, const Vector<mojom::blink::PermissionStatus>& results)
+void Permissions::batchTaskComplete(ScriptPromiseResolver* resolver, Vector<mojom::blink::PermissionDescriptorPtr> descriptors, Vector<int> callerIndexToInternalIndex, const Vector<mojom::blink::PermissionStatus>& results)
 {
     if (!resolver->getExecutionContext() || resolver->getExecutionContext()->activeDOMObjectsAreStopped())
         return;
@@ -247,7 +230,7 @@ void Permissions::batchTaskComplete(ScriptPromiseResolver* resolver, Vector<Perm
     HeapVector<Member<PermissionStatus>> result;
     result.reserveInitialCapacity(callerIndexToInternalIndex.size());
     for (int internalIndex : callerIndexToInternalIndex)
-        result.append(PermissionStatus::createAndListen(resolver->getExecutionContext(), results[internalIndex], names[internalIndex]));
+        result.append(PermissionStatus::createAndListen(resolver->getExecutionContext(), results[internalIndex], descriptors[internalIndex]->Clone()));
     resolver->resolve(result);
 }