[webkit-reviews] review denied: [Bug 75756] Implement navigator.startActivity; add IntentsController for managing web intents callbacks. : [Attachment 121680] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Greg Billock <gbillock at google.com>'s
request for review:
Bug 75756: Implement navigator.startActivity; add IntentsController for
managing web intents callbacks.
https://bugs.webkit.org/show_bug.cgi?id=75756

Attachment 121680: Patch
https://bugs.webkit.org/attachment.cgi?id=121680&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121680&action=review


This generally looks fine, but you haven't addressed my comments from before.

> Source/WebCore/page/Navigator.cpp:310
> +#if ENABLE(WEB_INTENTS)
> +void Navigator::startActivity(ScriptExecutionContext* context,
> +				 Intent* intent,
PassRefPtr<IntentResultCallback> successCallback,
PassRefPtr<IntentResultCallback> errorCallback, ExceptionCode& ec)

This should be done with [Supplemental] so we don't need to dump more unrelated
code into Navigator.cpp.

> Source/WebCore/page/Navigator.cpp:312
> +    Frame* activeFrame = static_cast<Document*>(context)->frame();

We should probably use the frame from Navigator rather than from the currently
executing script.  That's the normal thing when you call a method of an object.


> Source/WebCore/page/Navigator.cpp:326
> +    if (!activeFrame || !intent) {
> +	   ec = INVALID_STATE_ERR;
> +	   return;
> +    }
> +
> +    if (intent->action().isEmpty() || intent->type().isEmpty()) {
> +	   ec = VALIDATION_ERR;
> +	   return;
> +    }
> +
> +    if (!ScriptController::processingUserGesture()) {
> +	   ec = INVALID_ACCESS_ERR;
> +	   return;
> +    }

Is the order of these error checks spelled out in the spec?  Because you're
throwing different exceptions, the order is detectable.

> Source/WebCore/page/Navigator.idl:66
> +	   [CallWith=ScriptExecutionContext] void startActivity(in Intent
intent,

We don't need CallWith=ScriptExecutionContext.	Navigator is already associated
with a frame.