[webkit-reviews] review denied: [Bug 28170] SharedWorkers do not exit when the last parent document exits : [Attachment 34539] proposed 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 28170: SharedWorkers do not exit when the last parent document exits
https://bugs.webkit.org/show_bug.cgi?id=28170

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

------- Additional Comments from David Levin <levin at chromium.org>
Final comments for this version of the patch.


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

How is this testing that previous incarnations of the shared worker are closed?
 Maybe you can add a comment in here?


> diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp
> +#if ENABLE(SHARED_WORKERS)
> +	   && !SharedWorkerRepository::hasSharedWorkers(m_frame->document())

Why can't the page be cache when it has shared workers? Oh nice, you put this
in the changelog!
Nevermind.


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

> +    bool isDocumentInWorkerDocuments(Document* document) { return
m_documentSet.contains(document); }

Suggest: isInWorkerDocuments or inWorkerDocuments


About "m_documentSet", it seems like you're using "workerDocuments"
consistently for this, so why not name the variable that as well?


> +void SharedWorkerProxy::close()
> +{

ASSERT(!m_closing);

> +    m_closing = true;
> +    // Stop the worker thread - the proxy will stay around until we get
workerThreadExited() notification.
> +    if (m_thread)
> +	   m_thread->stop();
>  }


> @@ -203,7 +237,7 @@ DefaultSharedWorkerRepository&
DefaultSharedWorkerRepository::instance()
>  void DefaultSharedWorkerRepository::workerScriptLoaded(SharedWorkerProxy&
proxy, const String& userAgent, const String& workerScript,
PassOwnPtr<MessagePortChannel> port)
>  {
>      MutexLocker lock(m_lock);
> -    if (proxy.closing())
> +    if (proxy.isClosing())
>	   return;
>  
>      // Another loader may have already started up a thread for this proxy -
if so, just send a connect to the pre-existing thread.
> @@ -220,6 +254,44 @@ void
SharedWorkerRepository::connect(PassRefPtr<SharedWorker> worker, PassOwnPtr
>      DefaultSharedWorkerRepository::instance().connectToWorker(worker, port,
url, name, ec);
>  }
>  
> +void SharedWorkerRepository::documentDetached(Document* document)
> +{
> +    DefaultSharedWorkerRepository::instance().documentDetached(document);
> +}
> +
> +bool SharedWorkerRepository::hasSharedWorkers(Document* document)
> +{
> +    return
DefaultSharedWorkerRepository::instance().hasSharedWorkers(document);
> +}
> +
> +bool DefaultSharedWorkerRepository::hasSharedWorkers(Document* document)
> +{
> +    MutexLocker lock(m_lock);
> +    for (SharedWorkerProxyRepository::iterator iter = m_cache.begin() ; iter
!= m_cache.end() ; ++iter) {

Prefer indexes over iterators for vector



> @@ -247,17 +319,13 @@ PassRefPtr<SharedWorkerProxy>
DefaultSharedWorkerRepository::getProxy(const Stri

> +    for (SharedWorkerProxyRepository::iterator iter = m_cache.begin() ; iter
< m_cache.end() ; ++iter) {

Prefer indexes over iterators for vectors.


> +	   if (!(*iter)->isClosing() && (*iter)->matches(name, origin))
> +	       return *iter;
>      }
> +    // Proxy is not in the repository currently - create a new one.
> +    RefPtr<SharedWorkerProxy> proxy = SharedWorkerProxy::create(name, url,
origin.release());
> +    m_cache.append(proxy);
>      return proxy;

Slightly better to use release() here. (Avoids unnecessary ref count fuzzing.)



> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.h
b/WebCore/workers/DefaultSharedWorkerRepository.h
> @@ -59,6 +61,14 @@ namespace WebCore {
> +	   // Notification that a document has been detached, so we can
shutdown any associated SharedWorkers.

Suggest removing "so..." see explanation for similar suggestions (near the end
of this review).

> +	   void documentDetached(Document*);

> +	   // Invoked when a thread has exited, so we can remove the
SharedWorkerProxy from the repository.
Suggest removing "so..." see explanation for similar suggestions (near the end
of this review).
> +	   void removeProxy(SharedWorkerProxy*);


> +	   SharedWorkerProxyRepository m_cache;

Consider fixing the name "cache" since this isn't a cache.


> diff --git a/WebCore/workers/SharedWorkerRepository.h
b/WebCore/workers/SharedWorkerRepository.h
> +	   // Invoked when a document has been detached, so we can shut down
any associated workers.

"so we can shut down any associated workers."
This part of the comment can be easily become obsolete in lots of ways. I'd
suggest removing it.


> +	   static void documentDetached(Document*);
> +
> +	   // Returns true if the passed document is associated with any
SharedWorkers, to allow FrameLoader to determine if the Document can be cached
in the page cache.

This part of the comment can be easily become obsolete in lots of ways.
  " to allow FrameLoader to determine if the Document can be cached in the page
cache."
I'd suggest removing it.