[webkit-reviews] review denied: [Bug 83634] IDL and implementation for Web Intents delivery : [Attachment 136565] 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 83634: IDL and implementation for Web Intents delivery
https://bugs.webkit.org/show_bug.cgi?id=83634

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

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


The main thing we need here is tests.  Perhaps adding a function to
Internals.idl that sends an intent to a given window?

> Source/WebCore/ChangeLog:3
> +	   IDL and implementation for Web Intents delivery

Can you add a link to the spec?

>> Source/WebCore/ChangeLog:8
>> +	    No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or
explain why no new tests were possible.  [changelog/nonewtests] [5]

Can we add some tests, even just basic ones that check that the property
exists?

> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:71
> +    return true;

This always returns true?

> Source/WebCore/Modules/intents/DOMWindowIntents.idl:36
> +	   // !!! does this mean JS won't be able to call intent.postResult? or

> +	   // just that the setter is masked so they'll be able to do "var
intent = x"
> +	   // which I want?

The latter.  readonly just refers to the slot in the DOMWindow, not the object
that's in the slot.

> Source/WebCore/Modules/intents/DeliveredIntent.h:56
> +    virtual MessagePortArray* ports() const;
> +    virtual String getExtra(const String& key);
> +    virtual void postResult(PassRefPtr<SerializedScriptValue> data);
> +    virtual void postFailure(PassRefPtr<SerializedScriptValue> data);

Why do these need to be virtual?

> Source/WebCore/bindings/v8/custom/V8DeliveredIntentCustom.cpp:39
> +v8::Handle<v8::Value>
V8DeliveredIntent::portsAccessorGetter(v8::Local<v8::String> name, const
v8::AccessorInfo& info)

Does this need to be custom?  Do we need to teach the CodeGenerator something
about translating MessagePortArrays into JS arrays?

> Source/WebKit/chromium/public/WebFrame.h:-575
> -

This change seems spurious.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1850
> +    WebSerializedScriptValue ssv =
WebSerializedScriptValue::fromString(intent.data());

Generally, we prefer using complete words in variable names.  Perhaps
intentData ?