[webkit-reviews] review denied: [Bug 28839] Need to update v8 bindings to support passing multiple ports to postMessage() : [Attachment 38801] proposed patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Andrew Wilson
<atwilson at chromium.org>'s request for review:
Bug 28839: Need to update v8 bindings to support passing multiple ports to
postMessage()
https://bugs.webkit.org/show_bug.cgi?id=28839

Attachment 38801: proposed patch
https://bugs.webkit.org/attachment.cgi?id=38801&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
These are all style nits:

> +	   if (!fillWebCoreMessagePortArray(args[1], portArray))
> +	       return v8::Undefined();

if (!getMessagePortArray(.. is the common convention in WebKit. No need to use
WebCore in prefix. It's in String conv methods only because there are two
ambiguous string types.

> +	   if (!fillWebCoreMessagePortArray(args[1], portArray))

ditto.

> +    if (ec)
> +	   V8Proxy::setDOMException(ec);

return throwError(ec);

> +
> +CALLBACK_FUNC_DECL(MessageEventInitMessageEvent)
> +{
> +    INC_STATS("DOM.MessageEvent.initMessageEvent");
> +    MessageEvent* event =
V8DOMWrapper::convertToNativeObject<MessageEvent>(V8ClassIndex::MESSAGEEVENT,
args.Holder());
> +    String typeArg = v8ValueToWebCoreString(args[0]);
> +    bool canBubbleArg = args[1]->BooleanValue();
> +    bool cancelableArg = args[2]->BooleanValue();
> +    String dataArg = v8ValueToWebCoreString(args[3]);
> +    String originArg = v8ValueToWebCoreString(args[4]);
> +    String lastEventIdArg = v8ValueToWebCoreString(args[5]);
> +    DOMWindow* sourceArg = V8DOMWindow::HasInstance(args[6]) ?
V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW,
v8::Handle<v8::Object>::Cast(args[6])) : 0;

I am surprised there's no arg checking here. Is this correct?

> +    if (ec)
> +	   V8Proxy::setDOMException(ec);

return throwError(ec);

> +bool fillWebCoreMessagePortArray(v8::Local<v8::Value> value,
MessagePortArray& portArray)

bool getMessagePortArray(v8::Local<v8::Value> value, MessagePortArray&
portArray)

> +    if (!value->IsObject()) {
> +	   V8Proxy::throwError(V8Proxy::TypeError, "MessagePortArray argument
must be an object");
> +	   return false;

throwError("MessagePortArray argument must be an object");
return false;

> +	   v8::Local<v8::Value> sequenceLength =
ports->Get(v8::String::New("length"));
> +	   if (!sequenceLength->IsNumber()) {
> +	       V8Proxy::throwError(V8Proxy::TypeError, "MessagePortArray
argument has no length attribute");
> +	       return false;

throwError("MessagePortArray argument has no length attribute");
return false;

> +	       V8Proxy::setDOMException(INVALID_STATE_ERR);
> +	       return false;


throwError(ec);
return false;

> +	       V8Proxy::throwError(V8Proxy::TypeError, "MessagePortArray
argument must contain only MessagePorts");
> +	       return false;

ditto.

> +    if (ec)
> +	   V8Proxy::setDOMException(ec);

return throwError(ec);