[webkit-reviews] review denied: [Bug 27927] SharedWorkers should instantiate a thread and a SharedWorkerContext : [Attachment 34118] Patch addressing levin's comments

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Andrew Wilson
<atwilson at google.com>'s request for review:
Bug 27927: SharedWorkers should instantiate a thread and a SharedWorkerContext
https://bugs.webkit.org/show_bug.cgi?id=27927

Attachment 34118: Patch addressing levin's comments
https://bugs.webkit.org/attachment.cgi?id=34118&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few last things to fix up....

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Added disabled tests for SharedWorker minimal functionality.
> +
> +	   Began refactoring of dedicated worker tests to allow sharing test
cases between shared and dedicated worker (will continue in another patch).
> +
> +	   SharedWorkers should instantiate a thread and a SharedWorkerContext
> +	   https://bugs.webkit.org/show_bug.cgi?id=27927

The format du jour is to put these two lines after the Reviewed By line and
then the description.


> diff --git a/LayoutTests/fast/workers/shared-worker-gc.html-disabled
b/LayoutTests/fast/workers/shared-worker-gc.html-disabled
> +</html>

I see an extra </html> here but I think that you removed it in the next patch.

> +++
b/LayoutTests/fast/workers/shared-worker-replace-global-constructor.html-disabl
ed
> +</html>
I see an extra </html> here but I think that you removed it in the next patch.


> diff --git a/LayoutTests/fast/workers/shared-worker-simple.html-disabled
b/LayoutTests/fast/workers/shared-worker-simple.html-disabled
> +</html>
Extra </html> here but I think that you removed it in the next patch.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-08-01  Drew Wilson  <atwilson at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Created first working implementation of SharedWorkers (execution
only, no sharing).
> +	   Added initial implementations of SharedWorkerThread and
SharedWorkerContext.
> +	   No v8 bindings yet.
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=27927


The current preferred format is to put the bug title and then bg link right
after the review by line.

prepare-ChangeLog --bug #

will do this for you.

> diff --git a/WebCore/bindings/js/JSEventTarget.cpp
b/WebCore/bindings/js/JSEventTarget.cpp
>  #if ENABLE(SHARED_WORKERS)
>  #include "JSSharedWorker.h"
> +#include "JSSharedWorkerContext.h"
>  #include "SharedWorker.h"
> +#include "SharedWorkerContext.h"
>  #endif

Since you've ifdef'ed the headers, can you remove the ifdef's here (and then
sort them appropriately)?  (Less ifdef cruft is nice.)

> diff --git a/WebCore/bindings/js/WorkerScriptController.cpp
b/WebCore/bindings/js/WorkerScriptController.cpp
> +#if ENABLE(SHARED_WORKERS)
> +#include "JSSharedWorkerContext.h"
> +#endif

Since you've ifdef'ed the headers, can you remove the ifdef's here?

> diff --git a/WebCore/bindings/v8/DOMObjectsInclude.h
b/WebCore/bindings/v8/DOMObjectsInclude.h
> index 32354b3..ddfd629 100644
> --- a/WebCore/bindings/v8/DOMObjectsInclude.h
> +++ b/WebCore/bindings/v8/DOMObjectsInclude.h
> @@ -209,6 +209,7 @@
>  #if ENABLE(SHARED_WORKERS)
>  #include "AbstractWorker.h"
>  #include "SharedWorker.h"
> +#include "SharedWorkerContext.h"
>  #endif // SHARED_WORKERS

Since you've ifdef'ed the headers, can you remove the ifdef's here?

> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.cpp
b/WebCore/workers/DefaultSharedWorkerRepository.cpp

> +void DefaultSharedWorkerRepository::workerScriptLoaded(const String& name,
const KURL& url, const String& userAgent, const String& workerScript,
PassOwnPtr<MessagePortChannel> port)
> +{
> +    // FIXME: Cache the proxy to allow sharing and to allow cleanup after
the thread exits.
> +    SharedWorkerProxy *proxy = new SharedWorkerProxy();

SharedWorkerProxy*  (* on the wrong side...You may be glad to know that there
is a patch in a bug to check-webkit-style to catch but it is s\
till undergoing some fixes.)

Would you add a comment about what owns the SharedWorkerProxy (because it looks
like it leaks in this code)?


> +void SharedWorkerRepository::connect(PassRefPtr<SharedWorker> worker,
PassOwnPtr<MessagePortChannel> port, const KURL& url, const String& name,
ExceptionCode&)
> +{
> +    // Create a loader to load/initialize the requested worker
Add "."

> +    RefPtr<SharedWorkerLoader> loader = adoptRef(new
SharedWorkerLoader(worker, port.release(), name));

I think "port" alone should suffice here.


> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.h
b/WebCore/workers/DefaultSharedWorkerRepository.h
> +	   // Invoked once the worker script has been loaded to fire up the
worker thread.
> +	   void workerScriptLoaded(const String& name, const KURL& url, const
String& userAgent, const String& workerScript, PassOwnPtr<MessagePortChannel>);


No need to param name "url" here.

> diff --git a/WebCore/workers/SharedWorkerContext.idl
b/WebCore/workers/SharedWorkerContext.idl
> +    ] SharedWorkerContext : WorkerContext {
> +
> +	   readonly attribute DOMString name;
> +	   attribute EventListener onconnect;

The typical format is to indent attribute to leave spaces for "readonly" when
it isn't there.