[webkit-reviews] review denied: [Bug 73051] [Web Intents] Flagged-off WebCore implementation of navigator.startActivity : [Attachment 120372] 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 73051: [Web Intents] Flagged-off WebCore implementation of
navigator.startActivity
https://bugs.webkit.org/show_bug.cgi?id=73051

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

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


This patch is too large and contains many problems.  I'd recommend breaking it
down into smaller pieces so we can make sure everything gets a good review. 
The most problematic piece of this patch is the IntentsController.  Can you
help me understand what scope that object should have?

> Source/WebCore/Modules/intents/Intent.cpp:38
> +

Extra blank line here.

> Source/WebCore/Modules/intents/Intent.cpp:47
> +String Intent::action() const

const String&

> Source/WebCore/Modules/intents/Intent.cpp:52
> +String Intent::type() const

const String&

> Source/WebCore/Modules/intents/Intent.h:49
> +    static PassRefPtr<Intent> create(
> +	   const String& action,
> +	   const String& type,
> +	   PassRefPtr<SerializedScriptValue> data)

All on one line pls

> Source/WebCore/Modules/intents/Intent.h:62
> +protected:
> +    Intent(const String& action, const String& type,
PassRefPtr<SerializedScriptValue> data);

Why protected?	Does this class have subclasses?

> Source/WebCore/Modules/intents/IntentResultCallback.idl:29
> +	 LegacyDefaultOptionalArguments,

This attribute should never be added to any IDL files.	It's only for legacy
IDL files (and I think we've eradicated it for all of them by now).

> Source/WebCore/Modules/intents/IntentsController.cpp:60
> +    if (m_successCallbacks[intentId].get())

You should take() the callback out of the map and then handle the event.  The
way you've written this code, you've got a potential re-entrancy problem.

> Source/WebCore/Modules/intents/IntentsController.cpp:71
> +    if (m_errorCallbacks[intentId].get())
> +	   m_errorCallbacks[intentId]->handleEvent(value);

Same problem.  handleEvent can do arbitrary things.  There's no guantee this
object even exists afterwards.

> Source/WebCore/Modules/intents/IntentsController.h:40
> +class IntentsController {

Should this be non-copyable?

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:45
> +v8::Handle<v8::Value> V8Intent::constructorCallback(const v8::Arguments&
args)

We shouldn't need a custom constructor for this object.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String>
name, const v8::AccessorInfo& info)

We shouldn't need custom code here either.

> Source/WebCore/page/DOMWindow.idl:623
> +	   attribute IntentConstructor Intent; // Usable with the new operator

You can just used Conditional=WEB_INTENTS

> Source/WebCore/page/Navigator.cpp:328
> +    if (!frame() || !frame()->loader() || !frame()->loader()->client() ||
!intent.get()) {

frame()->loader() is always non-null.  frame()->loader()->client() is always
non-null.

"!intent.get()" can just be "!intent"

> Source/WebCore/page/Navigator.cpp:343
> +    int id =
m_frame->page()->intentsController()->assignIntentId(successCallback,
errorCallback);

You did a bunch of extra null checking, but you forgot to null check
m_frame->page(), which actually can be null.

It's very unlikely that intentsController should be a page-scoped object. 
Shouldn't we have one per ScriptExecutionContext?

> Source/WebCore/page/Navigator.h:85
> +    void startActivity(PassRefPtr<Intent>, PassRefPtr<IntentResultCallback>
successCallback, PassRefPtr<IntentResultCallback> errorCallback,
ExceptionCode&);

PassRefPtr<Intent> should just be const Intent&.  This function doesn't take
ownership of the intent.  Please see http://www.webkit.org/coding/RefPtr.html

> Source/WebCore/page/Page.cpp:158
> +#if ENABLE(WEB_INTENTS)
> +    , m_intentsController(IntentsController::create())
> +#endif

Why Page-lifetime?  This seems unlikely.  Can you show me where in the spec it
says how long we need to retain this state?

> Source/WebKit/chromium/public/WebIntent.h:65
> +    WebIntent(const WebCore::Intent&);

explicit

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1820
> +    WTF::String replyString = reply.operator String();

Do we really have to use the word "operator" here?  Shouldn't the compiler be
able to figure that out for us?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1821
> +    m_frame->page()->intentsController()->postResult(replyString,
intentIdentifier);

How do you know this result is going back to the right security context?  Page
can contain many different documents over its lifetime.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1831
> +    WTF::String replyString = reply.operator String();

Are we really not "using WTF::String" in this file?

> Source/WebKit/chromium/src/WebIntent.cpp:46
> +    : m_action(intent.action())
> +    , m_type(intent.type())
> +    , m_data(intent.data()->toWireString())
> +    , m_identifier(intent.identifier())

Why do we copy out all these fields instead of just take a reference to the
WebCore::Intent object?  That's the usual pattern for API objects that wrap
WebCore concepts.

> LayoutTests/platform/wincairo/Skipped:2005
> +web-intents/

This should probably be one word, without the -