[webkit-reviews] review denied: [Bug 28058] SharedWorkers should be shared : [Attachment 34238] proposed patch

Fri Aug 7 01:49:16 PDT 2009

David Levin <levin at chromium.org> has denied Andrew Wilson
<atwilson at google.com>'s request for review:
Bug 28058: SharedWorkers should be shared
https://bugs.webkit.org/show_bug.cgi?id=28058

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

------- Additional Comments from David Levin <levin at chromium.org>
Thanks this is looking good.  Just a few things to consider/address.

> diff --git a/LayoutTests/fast/workers/shared-worker-shared.html-disabled
b/LayoutTests/fast/workers/shared-worker-shared.html-disabled
> +<body>
> +<p>Test simple shared worker sharing cases.</p>

It is nice to tell people what to expect here.	When this runs you should see a
bunch of PASSes followed by a DONE.

> +
> +function testNewWorker()
> +{
> +    // New name, so should be a distinct worker from the previous one

Please add ".".

> +function testAlreadyLoaded()
> +{
> +    // Make sure that referencing a worker that is already loaded yields the
same instance

Please add ".".

> +function done()
> +{

Please print done here.

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

>  namespace WebCore {
>  
> -class SharedWorkerProxy : public WorkerLoaderProxy {
> +class SharedWorkerProxy : public ThreadSafeShared<SharedWorkerProxy>, public
WorkerLoaderProxy {
>  public:
> +    static PassRefPtr<SharedWorkerProxy> create(const String& name, const
KURL& url) { return adoptRef(new SharedWorkerProxy(name.copy(), url)); }

No need to do name.copy() here. It is done in the constructor as it should be.

> +    bool closing() { return m_closing; }
Make it const.

> +    KURL url() { return m_url; }
Since SharedWorkerProxy can be used on different threads, I think this should
return m_url.copy() (and make the method const).

> +    String name() { return m_name; }
Since SharedWorkerProxy can be used on different threads, I think this should
return m_name.copy() (and make the method const).

> +    SharedWorkerProxy(const String&, const KURL&);

Nice to add param name for the const String&.

> +void SharedWorkerProxy::addToDocumentSet(ScriptExecutionContext* context)

I didn't know what should be in a DocumentSet.	I do know what a referring
document is.

So why not make the name addReferringDocument()?

> +    // FIXME: track referring documents so we can shutdown the thread when
the last one exits.

Also nice to mention that this needs to remove the SharedWorkerProxy from the
SharedWorkerProxyCache.

> +// Loads the script on behalf of a worker.
> +class SharedWorkerScriptLoader : public
RefCounted<SharedWorkerScriptLoader>, public ActiveDOMObject, private
WorkerScriptLoaderClient {

Why is the callback
   virtual void notifyFinished();
public? (Make it private if possible.)

I guess you did this before during the big code review but I'm noticing it now
:)

> +void DefaultSharedWorkerRepository::connectToWorker(PassRefPtr<SharedWorker>
worker, PassOwnPtr<MessagePortChannel> port, const KURL& url, const String&
name, ExceptionCode& ec)
>  {
> +    MutexLocker lock(m_lock);
> +   
ASSERT(worker->scriptExecutionContext()->securityOrigin()->canAccess(SecurityOr
igin::create(url).get()));
> +    // Fetch a proxy corresponding to this SharedWorker.
> +    RefPtr<SharedWorkerProxy> proxy = getProxy(name, url);
> +    proxy->addToDocumentSet(worker->scriptExecutionContext());

This also needs to put the proxy in the worker.

> +    if (proxy->url() != url) {
> +	   // Proxy already existed under alternate URL - return an error.
> +	   ec = URL_MISMATCH_ERR;
> +	   return;
> +    }

Up to here, it feels like generic code that should go in
SharedWorkerRepository.  It would be nice to push as much as possible into
common code.  Of course, doing that, means that getProxy() would be a method on
SharedWorkerRepository that is implemented in here.  It would just call
DefaultSharedWorkerRepository::getProxy which would do the lock.

> +PassRefPtr<SharedWorkerProxy> DefaultSharedWorkerRepository::getProxy(const
String& name, const KURL& url)
> +{
> +    // Look for an existing worker, and create one if it doesn't exist.
> +    RefPtr<SecurityOrigin> origin = SecurityOrigin::create(url);

If SecurityOrigin actually stored the url, this would be broken because origin
may be used on multiple threads, so it feels fragile.  At the same time it
feels wasteful to do url.copy() when it isn't needed, but I favor less fragile
so consider:

   RefPtr<SecurityOrigin> origin = SecurityOrigin::create(url.copy());

or you could do the copy only when doing the set below.

> +    SharedWorkerNameMap* nameMap = m_cache.get(origin);
> +    if (!nameMap) {
> +	   nameMap = new SharedWorkerNameMap();
> +	   m_cache.set(origin, nameMap);
> +    }

> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.h
b/WebCore/workers/DefaultSharedWorkerRepository.h
>  #include "SharedWorkerRepository.h"
>  
Why have a blank line here?

> +#include "StringHash.h"

> +	   PassRefPtr<SharedWorkerProxy> getProxy(const String&, const KURL&);

Nice to have param name for the string.

> +	   typedef HashMap<String, RefPtr<SharedWorkerProxy> >
SharedWorkerNameMap;
> +	   typedef HashMap<RefPtr<SecurityOrigin>, SharedWorkerNameMap*,
SecurityOriginHash> SharedWorkerProxyCache;

This was pretty tricky to review because there are several objects in these
HashMaps that don't like to be ref'ed on multiple threads at once (StringImpl,
SecurityOrigin).
However, it is safe because you create things from scratch to put in them and
they are only accessed/used within a mutex.

It would be easy to mess this up by putting something in that wasn't created
fresh or by passing one of these items out without copying it.

I think it would be good to be a nice warning comment about the tight
restrictions that one must follow when using this structure.

> +	   SharedWorkerProxyCache m_cache;
>      };