[webkit-reviews] review denied: [Bug 25435] Dedicated workers do not support sending MessagePorts in messages : [Attachment 31895] revised patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Andrew Wilson
<atwilson at google.com>'s request for review:
Bug 25435: Dedicated workers do not support sending MessagePorts in messages
https://bugs.webkit.org/show_bug.cgi?id=25435

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

------- Additional Comments from David Levin <levin at chromium.org>
Just a few issues to address.



> diff --git a/LayoutTests/fast/workers/worker-cloneport.html-disabled
b/LayoutTests/fast/workers/worker-cloneport.html-disabled

> +worker.postMessage("postBack " + numMessages, channel.port1);
> +
> +// Test posting back 50000 messages, make sure ordering is fine

Nice to add "."

> +worker.onmessage = function(evt) {
> +    if (evt.data == "postBackDone") {
> +	  stopCloning = true;

indent off.

> +// Keep cloning the passed port until we're told to stop

Nice to add "."


> +	   if (numClones < 1000) {
> +	       // If we didn't clone at least 1000 times, then there's
something amiss

Nice to add "."

This seems timing sensitive.  I guess you're relying on a 1:50 ratio so it
shouldn't be too timing sensitive but I don't really see why an implementation
that did 50,000 messages of the other messages before 1000 of these would be
wrong.


> +	   // Make sure the messages arrived in order

Nice to add "."

> +	   var timer = setTimeout(function() {
> +	       log("FAILURE: Received: " + itemNum + " events - expected: " +
numMessages);
> +	   }, 1000);

How fast does the test run? (How close do you come to this time out and will
slower machines hit this time out?)
 

> +	       if (done) {
> +		  gc();
> +		  setTimeout(reportDone, 100);	// Make sure no unexpected
events come in

Nice to add "."

Also these are indented 3 spaces instead of 4.	However, it has been argued
that inconsistency in formatting js code helps ensure that the js parser can
handle these different formats, so I leave it up to you.




> diff --git a/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp
b/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp
>  
>  #include "Document.h"
>  #include "Frame.h"
> +#include "WorkerContext.h"
>  
>  #include "V8Binding.h"
>  #include "V8Proxy.h"
> +#include "WorkerContextExecutionProxy.h"

These headers aren't ordered correctly.  Sort alphabetically.
  



> diff --git a/WebCore/platform/CrossThreadCopier.h
b/WebCore/platform/CrossThreadCopier.h
> +    template<typename T> struct CrossThreadCopierBase<false, PassOwnPtr<T> >
{
> +	   typedef PassOwnPtr<T> Type;
> +	   static Type copy(const PassOwnPtr<T>& refPtr)
> +	   {
> +	       return PassOwnPtr<T>(static_cast<T*>(refPtr.release()));
> +	   }

s/refPtr/ownPtr/



> diff --git a/WebCore/workers/Worker.idl b/WebCore/workers/Worker.idl
> +	   void postMessage(in DOMString message, in [Optional] MessagePort
messagePort)
> +	       raises(DOMException);

Same question as WorkerContext.idl about this extra parameter (see below).


> diff --git a/WebCore/workers/WorkerContext.idl
b/WebCore/workers/WorkerContext.idl

> +	   void postMessage(in DOMString message, in [Optional] MessagePort
messagePort)
> +	       raises(DOMException);

Should MessagePort be allowed to be passed in if !ENABLE(CHANNEL_MESSAGING) ?

>  
> +#if ENABLE_CHANNEL_MESSAGING

Can this be
  #if ENABLE(CHANNEL_MESSAGING)
?

> +	   attribute [JSCCustomGetter] MessageChannelConstructor
MessageChannel;
> +#endif


> diff --git a/WebCore/workers/WorkerObjectProxy.h
b/WebCore/workers/WorkerObjectProxy.h
> +#include <wtf/PassOwnPtr.h>


The <> come after the "" headers.

>  #include "Console.h"