Plumb reserve method of the BudgetAPI

third_party/WebKit/Source/modules/budget/BudgetService.cpp

Index: third_party/WebKit/Source/modules/budget/BudgetService.cpp
diff --git a/third_party/WebKit/Source/modules/budget/BudgetService.cpp b/third_party/WebKit/Source/modules/budget/BudgetService.cpp
index 6cbd0baa01c27c20cef728afeabbf835aee84bbe..edfea530d5d78b1fc9a22a89e36453bd37cf0a2b 100644
--- a/third_party/WebKit/Source/modules/budget/BudgetService.cpp
+++ b/third_party/WebKit/Source/modules/budget/BudgetService.cpp
@@ -15,6 +15,28 @@
 #include "public/platform/modules/budget_service/budget_service.mojom-blink.h"
 
 namespace blink {
+namespace {
+    mojom::blink::BudgetOperationType stringToOperationType(const AtomicString& operation)

[Peter Beverloo, 2016/09/05 17:06:16]

nit: please don't indent this code by an additional four characters.

[harkness, 2016/09/06 15:45:01]

Done.

[Peter Beverloo, 2016/09/06 15:54:10]

This isn't done either.

[harkness, 2016/09/06 16:17:25]

git cl format strikes again, but if I override it, presubmit complains. I'm
going to leave the indentation broken, and let you follow up with the tools
author.

+    {
+        if (operation == "silent-push")
+            return mojom::blink::BudgetOperationType::SILENT_PUSH;
+
+        return mojom::blink::BudgetOperationType::INVALID_OPERATION;
+    }
+
+    DOMException* errorTypeToException(mojom::blink::BudgetServiceErrorType error)
+    {
+        switch (error) {
+        case mojom::blink::BudgetServiceErrorType::NO_ERROR:
+            return nullptr;
+        case mojom::blink::BudgetServiceErrorType::DATABASE_ERROR:
+            return DOMException::create(DataError, "Error reading the budget database.");
+        case mojom::blink::BudgetServiceErrorType::NOT_SUPPORTED:
+            return DOMException::create(NotSupportedError, "Requested opration was not supported");
+        }

[Peter Beverloo, 2016/09/05 17:06:16]

Please add a NOTREACHED() at the end of this method.

[harkness, 2016/09/06 15:45:01]

Done.

+    }
+
+} // namespace
 
 BudgetService::BudgetService()
 {
@@ -30,16 +52,20 @@ BudgetService::~BudgetService()
 {
 }
 
-ScriptPromise BudgetService::getCost(ScriptState* scriptState, const AtomicString& /* actionType */)
+ScriptPromise BudgetService::getCost(ScriptState* scriptState, const AtomicString& operation)
 {
     DCHECK(m_service);
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
-    // TODO(harkness): Map the actionType to BudgetOperationType.
+    mojom::blink::BudgetOperationType type = stringToOperationType(operation);
+    if (type == mojom::blink::BudgetOperationType::INVALID_OPERATION) {
+        resolver->reject(DOMException::create(NotSupportedError, "Invalid operation type specified"));

[Peter Beverloo, 2016/09/05 17:06:16]

nit: the clean way of doing this (here and on lines 98 and 123) would be to
*not* create the promise and the resolver yourself, but rather bail out early
through:

return ScriptPromise::rejectWithDOMException(..);

[harkness, 2016/09/06 15:45:01]

I updated here and 123, but 98 is a different situation, because it's on the
return path so the resolver/promise have already been created. Is that still a
valid place for the rejectWithDOMException syntax?

[Peter Beverloo, 2016/09/06 15:54:10]

98's fine, thanks :)

[harkness, 2016/09/06 16:17:25]

Acknowledged.

+        return promise;
+    }
+
     // Get the cost for the action from the browser BudgetService.
-    mojom::blink::BudgetOperationType type = mojom::blink::BudgetOperationType::SILENT_PUSH;
     m_service->GetCost(type, convertToBaseCallback(WTF::bind(&BudgetService::gotCost, wrapPersistent(this), wrapPersistent(resolver))));
     return promise;
 }
@@ -66,8 +92,13 @@ ScriptPromise BudgetService::getBudget(ScriptState* scriptState)
     return promise;
 }
 
-void BudgetService::gotBudget(ScriptPromiseResolver* resolver, const mojo::WTFArray<mojom::blink::BudgetStatePtr> expectations) const
+void BudgetService::gotBudget(ScriptPromiseResolver* resolver, mojom::blink::BudgetServiceErrorType error, const mojo::WTFArray<mojom::blink::BudgetStatePtr> expectations) const
 {
+    if (error != mojom::blink::BudgetServiceErrorType::NO_ERROR) {
+        resolver->reject(errorTypeToException(error));
+        return;
+    }
+
     // Copy the chunks into the budget array.
     HeapVector<Member<BudgetState>> budget(expectations.size());
     for (size_t i = 0; i < expectations.size(); i++)
@@ -76,6 +107,39 @@ void BudgetService::gotBudget(ScriptPromiseResolver* resolver, const mojo::WTFAr
     resolver->resolve(budget);
 }
 
+ScriptPromise BudgetService::reserve(ScriptState* scriptState, const AtomicString& operation)
+{
+    DCHECK(m_service);
+
+    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+
+    ExecutionContext* context = scriptState->getExecutionContext();
+    if (!context)

[haraken, 2016/09/06 01:46:44]

Nit: I don't think this should happen.

[harkness, 2016/09/06 15:45:01]

In a previous review, you mentioned that it could be a problem if the script was
being executed on a detached frame. Peter explained a bit of the world of
execution contexts and how some of the underlying code is changing to deal with
the possibility of null contexts. Is this something that has changed so that
it's now safe?

[haraken, 2016/09/07 01:55:47]

Maybe are you talking about other getExecutionContext()?

scriptState->getExecutionContext() returns an ExecutionContext even after the
associated frame is detached. If you want to prevent the following code from
being run after the frame detach, you need to add the following check:

  if (!m_scriptState->contextIsValid())
    return;

Either way, the real problem is that APIs around getting contexts are confusing.
The binding team will take care of it.

[harkness, 2016/09/07 09:24:52]

Thanks very much for the explanation, I've removed the null check.

+        return promise;
+

[Peter Beverloo, 2016/09/05 17:06:16]

nit: mind adding a TODO that we need to do a secure origin check here?

[harkness, 2016/09/06 15:45:00]

Done.

+    mojom::blink::BudgetOperationType type = stringToOperationType(operation);
+    if (type == mojom::blink::BudgetOperationType::INVALID_OPERATION) {
+        resolver->reject(DOMException::create(NotSupportedError, "Invalid operation type specified"));
+        return promise;
+    }
+
+    // Call to the BudgetService to place the reservation.
+    RefPtr<SecurityOrigin> origin(context->getSecurityOrigin());
+    m_service->Reserve(origin, type, convertToBaseCallback(WTF::bind(&BudgetService::gotReservation, wrapPersistent(this), wrapPersistent(resolver))));
+    return promise;
+}
+
+void BudgetService::gotReservation(ScriptPromiseResolver* resolver, mojom::blink::BudgetServiceErrorType error, bool success) const
+{
+    if (error != mojom::blink::BudgetServiceErrorType::NO_ERROR) {
+        resolver->reject(errorTypeToException(error));
+        return;
+    }
+
+    resolver->resolve(success);
+}
+
 void BudgetService::onConnectionError()
 {
     LOG(ERROR) << "Unable to connect to the Mojo BudgetService.";