[webkit-reviews] review denied: [Bug 75756] Implement navigator.startActivity; add IntentsController for managing web intents callbacks. : [Attachment 121729] 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 121729: Patch
https://bugs.webkit.org/attachment.cgi?id=121729&action=review

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


This is starting to look good.	There's just a couple memory management issues
to polish up.

> Source/WebCore/ChangeLog:8
> +	   Implement navigator.startActivity; add IntentsController for
managing web intents callbacks.
> +	   https://bugs.webkit.org/show_bug.cgi?id=75756
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Test: webintents/web-intents-api.html

It would be nice to have more information in the ChangeLog.  Folks like to read
these to understand what's happening in the project.

> Source/WebCore/Modules/intents/IntentRequest.cpp:63
> +    ContextDestructionObserver::contextDestroyed();
> +    m_successCallback.clear();
> +    m_errorCallback.clear();

Rather than overriding these, you should just check whether
scriptExecutionContext() is null when the callbacks are called.

> Source/WebCore/Modules/intents/IntentRequest.cpp:85
> +    unsetPendingActivity(this);

Depending on the ownership model for IntentRequest, you might want to store a
RefPtr to |this| on the stack so that we know that the last reference to this
object doesn't get destroyed by the JavaScript that runs during handleEvent.

> Source/WebCore/Modules/intents/IntentRequest.h:53
> +    virtual void contextDestroyed();

Please add the OVERRIDE keyword.

> Source/WebCore/Modules/intents/IntentRequest.h:55
> +    // TODO(gbillock at google.com): support suspend/resume

I'd just remove this comment.  (If we were to keep it, we'd need it to be
"FIXME:" rather than "TODO(gbillock at google.com)" )

> Source/WebCore/Modules/intents/IntentRequest.h:60
> +    Intent* m_intent;

Doesn't this need to be a RefPtr?  What's keeping the Intent alive?

> Source/WebCore/Modules/intents/IntentResultCallback.h:33
> +#include <wtf/RefCounted.h>
> +#include <wtf/RefPtr.h>

These should be inside the #if

> Source/WebCore/Modules/intents/NavigatorIntents.h:44
> +    static void startActivity(Navigator*,
> +				 Intent*, PassRefPtr<IntentResultCallback>
successCallback, PassRefPtr<IntentResultCallback> errorCallback,
ExceptionCode&);

One line pls.