Bug 38742 – [chromium] Implement canEstablishDatabase call for workers.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

This wires up the canEstablishDatabase call for workers. It gets routed to WebWorkerCommonClient::allowDatabase

Created attachment 55364 [details]
Patch

[WebKit Review Bot]

Attachment 55364 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 55365 [details]
Patch

[WebKit Review Bot]

Attachment 55364 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2196035

Created attachment 55367 [details]
Patch

[WebKit Review Bot]

Attachment 55365 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2215040

[Dmitry Titov]

Comment on attachment 55367 [details]
Patch

Looks reasonable, couple of notes:

WebKit/chromium/src/WebWorkerBase.cpp:157
 +          return false;
It is enough to have simple ASSERT(controller) here, since this method is not supposed to be called for non-worker contexts and there is no meaningful scenario when we actually want to return false from here.

WebKit/chromium/src/WebWorkerBase.h:150
 +      class AllowDatabaseMainThreadBridge;
This class is a detail of implementation and should be declared/defined in cpp file.

WebKit/chromium/src/WebWorkerBase.h:152
 +      static void allowDatabaseTask(
This can be just a static method in cpp file, it does not need to be in .h

WebKit/chromium/src/WebWorkerBase.cpp:198
 +  void WebWorkerBase::AllowDatabaseMainThreadBridge::cancel()
Where is this method used?

WebKit/chromium/src/WebWorkerBase.cpp:206
 +      MutexLocker lock(m_synchronousMutex);
Why this mutex is necessary? It is usually not needed to protect bool values like that.

[Michael Nordman]

This looks pretty good to me.

Consider setting the result data member and done data member in the AllowDatabaseMainThreadBridge::didComplete() method body which is run on the context thread. That could help get rid of the mutex.

 91     // Controls whether access to Web Databases is allowed for this worker.
 92     virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize);

Does WebWorkerBase::allowDatabase really need to be virtual? I don't think this is an overload of the method in the public api. Also I think the method arguments can be in terms of WebCore types instead of WebKit types.

Is WebWorkerClientImpl::allowDatabase really needed? When/where is that method called?

[Michael Nordman]

> Is WebWorkerClientImpl::allowDatabase really needed? When/where is that method
> called?

Duh... please ignore that last comment (above) about WebWorkerClientImpl :)

(In reply to comment #8)
> This looks pretty good to me.
> 
> Consider setting the result data member and done data member in the
> AllowDatabaseMainThreadBridge::didComplete() method body which is run on the
> context thread. That could help get rid of the mutex.

good point.

> 
>  91     // Controls whether access to Web Databases is allowed for this worker.
>  92     virtual bool allowDatabase(WebFrame*, const WebString& name, const
> WebString& displayName, unsigned long estimatedSize);
> 
> Does WebWorkerBase::allowDatabase really need to be virtual? I don't think this
> is an overload of the method in the public api. Also I think the method
> arguments can be in terms of WebCore types instead of WebKit types.
> 
> Is WebWorkerClientImpl::allowDatabase really needed? When/where is that method
> called?

(In reply to comment #7)
> (From update of attachment 55367 [details])
> Looks reasonable, couple of notes:
> 
> WebKit/chromium/src/WebWorkerBase.cpp:157
>  +          return false;
> It is enough to have simple ASSERT(controller) here, since this method is not
> supposed to be called for non-worker contexts and there is no meaningful
> scenario when we actually want to return false from here.
> 
> WebKit/chromium/src/WebWorkerBase.h:150
>  +      class AllowDatabaseMainThreadBridge;
> This class is a detail of implementation and should be declared/defined in cpp
> file.
> 
> WebKit/chromium/src/WebWorkerBase.h:152
>  +      static void allowDatabaseTask(
> This can be just a static method in cpp file, it does not need to be in .h
> 
> WebKit/chromium/src/WebWorkerBase.cpp:198
>  +  void WebWorkerBase::AllowDatabaseMainThreadBridge::cancel()
> Where is this method used?

Hups, I've intended to invoke this when the runloop was terminated before the call was completed. In that case, the didComplete call should be get scheduled on the context thread.

> 
> WebKit/chromium/src/WebWorkerBase.cpp:206
>  +      MutexLocker lock(m_synchronousMutex);
> Why this mutex is necessary? It is usually not needed to protect bool values
> like that.

Created attachment 55524 [details]
Patch

[WebKit Review Bot]

Attachment 55524 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebWorkerBase.cpp:122:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #13)
> Attachment 55524 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebKit/chromium/src/WebWorkerBase.cpp:122:  One space before end of line comments  [whitespace/comments] [5]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

this is

}  // namespace

there's another instance in WebCore/rendering that does the same, and the coding style doesn't really say what to do with namespaces :/

Created attachment 55540 [details]
Patch

I've landed the allowDatabase stubs on chromium, so this patch is safe to apply.

[Adam Barth]

 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> this is
> 
> }  // namespace
> 
> there's another instance in WebCore/rendering that does the same, and the coding style doesn't really say what to do with namespaces :/

I believe the style checker is correct here and the examples of this in other files are errors that predate the style checker.

(In reply to comment #17)
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > this is
> > 
> > }  // namespace
> > 
> > there's another instance in WebCore/rendering that does the same, and the coding style doesn't really say what to do with namespaces :/
> 
> I believe the style checker is correct here and the examples of this in other files are errors that predate the style checker.

yes, I've updated the patch accordingly.

[Andrew Wilson]

Comment on attachment 55540 [details]
Patch

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 13e117d3f520ad993695e5793be6b66eb7f22b6a..8d0886515d3e350977640e8bec0a7132d6df23db 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,26 @@
> [phone]  Jochen Eisinger  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implement canEstablishDatabase call for workers.
> +        https://bugs.webkit.org/show_bug.cgi?id=38742
> +
> +        * public/WebCommonWorkerClient.h:
> +        * src/DatabaseObserver.cpp:
> +        (WebCore::DatabaseObserver::canEstablishDatabase):
> +        * src/WebWorkerBase.cpp:
> +        (WebKit::WebWorkerBase::allowDatabase):
> +        (WebKit::WebWorkerBase::allowDatabaseTask):
> +        (WebKit::WebWorkerBase::AllowDatabaseMainThreadBridge::AllowDatabaseMainThreadBridge):
> +        (WebKit::WebWorkerBase::AllowDatabaseMainThreadBridge::cancel):
> +        (WebKit::WebWorkerBase::AllowDatabaseMainThreadBridge::done):
> +        (WebKit::WebWorkerBase::AllowDatabaseMainThreadBridge::result):
> +        (WebKit::WebWorkerBase::AllowDatabaseMainThreadBridge::signalCompleted):
> +        (WebKit::WebWorkerBase::AllowDatabaseMainThreadBridge::didComplete):
> +        * src/WebWorkerBase.h:
> +        * src/WebWorkerClientImpl.h:
> +        (WebKit::WebWorkerClientImpl::allowDatabase):
> +
>  2010-05-08  Jens Alfke  <[e-mail]>
>  
>          Reviewed by Darin Fisher.
> diff --git a/WebKit/chromium/public/WebCommonWorkerClient.h b/WebKit/chromium/public/WebCommonWorkerClient.h
> index 13603cbc9315085430393a84c7f695e1e305d256..cea647123a52f5032850890dc84552c41644ff2d 100644
> --- a/WebKit/chromium/public/WebCommonWorkerClient.h
> +++ b/WebKit/chromium/public/WebCommonWorkerClient.h
> @@ -35,6 +35,7 @@ namespace WebKit {
>  
>  class WebApplicationCacheHost;
>  class WebApplicationCacheHostClient;
> +class WebFrame;
>  class WebNotificationPresenter;
>  class WebString;
>  class WebWorker;
> @@ -79,6 +80,9 @@ public:
>      // Called on the main webkit thread in the worker process during initialization.
>      virtual WebApplicationCacheHost* createApplicationCacheHost(WebApplicationCacheHostClient*) = 0;
>  
> +    // Called on the main webkit thread before opening a web database.
> +    virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) = 0;
> +
>  protected:
>      ~WebCommonWorkerClient() { }
>  };
> diff --git a/WebKit/chromium/src/DatabaseObserver.cpp b/WebKit/chromium/src/DatabaseObserver.cpp
> index 6a2e2a70ac08441adbb8e22e0e49355475507c6c..1c5117c0f57bdff1ca51dcc8774af6b3cd27912b 100644
> --- a/WebKit/chromium/src/DatabaseObserver.cpp
> +++ b/WebKit/chromium/src/DatabaseObserver.cpp
> @@ -50,12 +50,16 @@ namespace WebCore {
>  bool DatabaseObserver::canEstablishDatabase(ScriptExecutionContext* scriptExecutionContext, const String& name, const String& displayName, unsigned long estimatedSize)
>  {
>      ASSERT(scriptExecutionContext->isContextThread());
> -    // FIXME: add support for the case scriptExecutionContext()->isWorker() once workers implement web databases.
> -    ASSERT(scriptExecutionContext->isDocument());
> +    ASSERT(scriptExecutionContext->isDocument() || scriptExecutionContext->isWorkerContext());
>      if (scriptExecutionContext->isDocument()) {
>          Document* document = static_cast<Document*>(scriptExecutionContext);
>          WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());
>          return webFrame->client()->allowDatabase(webFrame, name, displayName, estimatedSize);
> +    } else {
> +        WorkerContext* workerContext = static_cast<WorkerContext*>(scriptExecutionContext);
> +        WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy();
> +        WebWorkerBase* webWorker = static_cast<WebWorkerBase*>(workerLoaderProxy);
> +        return webWorker->allowDatabase(0, name, displayName, estimatedSize);
>      }
>  
>      return true;
> diff --git a/WebKit/chromium/src/WebWorkerBase.cpp b/WebKit/chromium/src/WebWorkerBase.cpp
> index 2c4b4f632341a2ee8b82ecb51ac172c8a50119b8..b844345ab160a2b6ee9ee1e053692d4b4a44466a 100644
> --- a/WebKit/chromium/src/WebWorkerBase.cpp
> +++ b/WebKit/chromium/src/WebWorkerBase.cpp
> @@ -31,6 +31,7 @@
>  #include "config.h"
>  #include "WebWorkerBase.h"
>  
> +#include "DatabaseTask.h"
>  #include "GenericWorkerTask.h"
>  #include "MessagePortChannel.h"
>  #include "PlatformMessagePortChannel.h"
> @@ -44,6 +45,7 @@
>  #include "WebView.h"
>  #include "WebWorkerClient.h"
>  
> +#include "WorkerScriptController.h"
>  #include "WorkerThread.h"
>  #include <wtf/MainThread.h>
>  
> @@ -53,6 +55,72 @@ namespace WebKit {
>  
>  #if ENABLE(WORKERS)
>  
> +static const char allowDatabaseMode[] = "allowDatabaseMode";
> +
> +namespace {
> +
> +// This class is used to route the result of the WebWorkerBase::allowDatabase
> +// call back to the worker context.
> +class AllowDatabaseMainThreadBridge : public ThreadSafeShared<AllowDatabaseMainThreadBridge> {
> +public:
> +    static PassRefPtr<AllowDatabaseMainThreadBridge> create(WebWorkerBase* worker, const WebCore::String& mode, WebCommonWorkerClient* commonClient, WebFrame* frame, const WebCore::String& name, const WebCore::String& displayName, unsigned long estimatedSize) { return adoptRef(new AllowDatabaseMainThreadBridge(worker, mode, commonClient, frame, name, displayName, estimatedSize)); }
> +
> +    // These methods are invoked on the worker context.
> +    void cancel()
> +    {
> +        MutexLocker locker(m_mutex);
> +        m_worker = 0;
> +    }
> +
> +    bool done()
> +    {
> +        return m_completed;
> +    }
> +
> +    bool result()
> +    {
> +        return m_result;
> +    }
> +
> +    // This method is invoked on the main thread.
> +    void signalCompleted(bool result)
> +    {
> +        MutexLocker locker(m_mutex);
> +        if (m_worker)
> +            m_worker->postTaskForModeToWorkerContext(createCallbackTask(&didComplete, this, result), m_mode);
> +    }
> +
> +private:
> +    AllowDatabaseMainThreadBridge(WebWorkerBase* worker, const WebCore::String& mode, WebCommonWorkerClient* commonClient, WebFrame* frame, const WebCore::String& name, const WebCore::String& displayName, unsigned long estimatedSize)
> +        : m_completed(false)
> +        , m_worker(worker)
> +        , m_mode(mode)
> +    {
> +        worker->dispatchTaskToMainThread(createCallbackTask(&allowDatabaseTask, commonClient, frame, String(name), String(displayName), estimatedSize, this));
> +    }
> +
> +    static void allowDatabaseTask(WebCore::ScriptExecutionContext* context, WebCommonWorkerClient* commonClient, WebFrame* frame, const WebCore::String name, const WebCore::String displayName, unsigned long estimatedSize, PassRefPtr<AllowDatabaseMainThreadBridge> bridge)
> +    {
> +        if (!commonClient)
> +            bridge->signalCompleted(false);
> +        else
> +            bridge->signalCompleted(commonClient->allowDatabase(frame, name, displayName, estimatedSize));
> +    }
> +
> +    static void didComplete(WebCore::ScriptExecutionContext* context, PassRefPtr<AllowDatabaseMainThreadBridge> bridge, bool result)
> +    {
> +        bridge->m_result = result;
> +        bridge->m_completed = true;
> +    }
> +
> +    bool m_result;
> +    bool m_completed;
> +    Mutex m_mutex;
> +    WebWorkerBase* m_worker;
> +    WebCore::String m_mode;
> +};
> +}
> +
>  // This function is called on the main thread to force to initialize some static
>  // values used in WebKit before any worker thread is started. This is because in
>  // our worker processs, we do not run any WebKit code in main thread and thus
> @@ -147,6 +215,30 @@ WebApplicationCacheHost* WebWorkerBase::createApplicationCacheHost(WebFrame*, We
>      return 0;
>  }
>  
> +bool WebWorkerBase::allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize)
> +{
> +    WorkerRunLoop& runLoop = m_workerThread->runLoop();
> +    WorkerScriptController* controller = WorkerScriptController::controllerForContext();
> +    ASSERT(controller);
> +    WorkerContext* workerContext = controller->workerContext();
> +
> +    // Create a unique mode just for this synchronous call.
> +    String mode = allowDatabaseMode;
> +    mode.append(String::number(runLoop.createUniqueId()));
> +
> +    RefPtr<AllowDatabaseMainThreadBridge> bridge = AllowDatabaseMainThreadBridge::create(this, mode, commonClient(), m_webView->mainFrame(), String(name), String(displayName), estimatedSize);
> +    MessageQueueWaitResult result = MessageQueueMessageReceived;
> +    while (!bridge->done() && result != MessageQueueTerminated)
> +        result = runLoop.runInMode(workerContext, mode);
> +
> +    if (!bridge->done() && result == MessageQueueTerminated) {
> +        bridge->cancel();
> +        return false;
> +    }
> +
> +    return bridge->result();
> +}
> +
>  // WorkerObjectProxy -----------------------------------------------------------
>  
>  void WebWorkerBase::postMessageToWorkerObject(PassRefPtr<SerializedScriptValue> message,
> diff --git a/WebKit/chromium/src/WebWorkerBase.h b/WebKit/chromium/src/WebWorkerBase.h
> index a470ee467d2f8629879d95af4c7593249597bd54..15e8013c9e5958b2765fca51e55729570c052a6d 100644
> --- a/WebKit/chromium/src/WebWorkerBase.h
> +++ b/WebKit/chromium/src/WebWorkerBase.h
> @@ -88,6 +88,9 @@ public:
>      virtual void didCreateDataSource(WebFrame*, WebDataSource*);
>      virtual WebApplicationCacheHost* createApplicationCacheHost(WebFrame*, WebApplicationCacheHostClient*);
>  
> +    // Controls whether access to Web Databases is allowed for this worker.
> +    virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize);
> +
>      // Executes the given task on the main thread.
>      static void dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
>  
> diff --git a/WebKit/chromium/src/WebWorkerClientImpl.h b/WebKit/chromium/src/WebWorkerClientImpl.h
> index 907499a266a6762cfbf29acd93d10574a60f9eeb..afa9b9b337695270e72737dd71ce3995ff59e718 100644
> --- a/WebKit/chromium/src/WebWorkerClientImpl.h
> +++ b/WebKit/chromium/src/WebWorkerClientImpl.h
> @@ -95,6 +95,7 @@ public:
>          return 0;
>      }
>      virtual WebApplicationCacheHost* createApplicationCacheHost(WebApplicationCacheHostClient*) { return 0; }
> +    virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) { return true; }
>  
>  private:
>      virtual ~WebWorkerClientImpl();
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 4e48b364a4c12269074dafaf26b4b4d30312952f..4b57ab4260d7de0290da772e93d236f1d9724644 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,15 @@
> [phone]  Jochen Eisinger  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add allowDatabase method to TestWebWorker.
> +        https://bugs.webkit.org/show_bug.cgi?id=38742
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * DumpRenderTree/chromium/TestWebWorker.h:
> +        (TestWebWorker::allowDatabase):
> +
>  2010-05-09  Daniel Bates  <[e-mail]>
>  
>          Reviewed by Chris Jerdonek.
> diff --git a/WebKitTools/DumpRenderTree/chromium/TestWebWorker.h b/WebKitTools/DumpRenderTree/chromium/TestWebWorker.h
> index f28cc2055d25754478f0622f3b71673dffb30102..94708040d50e412390c55ab8956639be8add5e09 100644
> --- a/WebKitTools/DumpRenderTree/chromium/TestWebWorker.h
> +++ b/WebKitTools/DumpRenderTree/chromium/TestWebWorker.h
> @@ -80,6 +80,7 @@ public:
>      virtual WebKit::WebWorker* createWorker(WebKit::WebWorkerClient*) { return 0; }
>      virtual WebKit::WebNotificationPresenter* notificationPresenter() { return 0; }
>      virtual WebKit::WebApplicationCacheHost* createApplicationCacheHost(WebKit::WebApplicationCacheHostClient*) { return 0; }
> +    virtual bool allowDatabase(WebKit::WebFrame*, const WebKit::WebString&, const WebKit::WebString&, unsigned long) { return true; }
>  
>  private:
>      ~TestWebWorker() {}
WebKit/chromium/src/WebWorkerBase.cpp:221
 +      WorkerScriptController* controller = WorkerScriptController::controllerForContext();
We should be checking for controller == 0 here (allowDatabase() called while worker is shutting down) and returning false.

WebKit/chromium/src/WebWorkerClientImpl.h:98
 +      virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) { return true; }
This should never be called on the renderer side, so I think this should be ASSERT_NOT_REACHED()

WebKit/chromium/src/WebWorkerBase.cpp:66
 +      static PassRefPtr<AllowDatabaseMainThreadBridge> create(WebWorkerBase* worker, const WebCore::String& mode, WebCommonWorkerClient* commonClient, WebFrame* frame, const WebCore::String& name, const WebCore::String& displayName, unsigned long estimatedSize) { return adoptRef(new AllowDatabaseMainThreadBridge(worker, mode, commonClient, frame, name, displayName, estimatedSize)); }
This seems really hard to read as one big long line. Can we break this up? I'd suggest putting the function body on its own lines at least:

static PassRefPtr<...>create(.....)
{
  return adoptRef(...);
}


WebKit/chromium/src/WebWorkerBase.cpp:234
 +      if (!bridge->done() && result == MessageQueueTerminated) {
Just curious - do we need the check for bridge->done() here and above (and any of the m_completed logic in the Bridge class)? I'm not seeing how runInMode() can return without the appropriate task having been fired (or the queue terminated).

[Andrew Wilson]

Sorry, reviewer FAIL on that previous comment :(

Anyhow, just a quick drive-by:

WebKit/chromium/src/WebWorkerBase.cpp:221
 +      WorkerScriptController* controller = WorkerScriptController::controllerForContext();
We should be checking for controller == 0 here (allowDatabase() called while worker is shutting down) and returning false.

WebKit/chromium/src/WebWorkerClientImpl.h:98
 +      virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) { return true; }
This should never be called on the renderer side, so I think this should be ASSERT_NOT_REACHED()

WebKit/chromium/src/WebWorkerBase.cpp:66
 +      static PassRefPtr<AllowDatabaseMainThreadBridge> create(WebWorkerBase* worker, const WebCore::String& mode, WebCommonWorkerClient* commonClient, WebFrame* frame, const WebCore::String& name, const WebCore::String& displayName, unsigned long estimatedSize) { return adoptRef(new AllowDatabaseMainThreadBridge(worker, mode, commonClient, frame, name, displayName, estimatedSize)); }
This seems really hard to read as one big long line. Can we break this up? I'd suggest putting the function body on its own lines at least:

static PassRefPtr<...>create(.....)
{
  return adoptRef(...);
}


WebKit/chromium/src/WebWorkerBase.cpp:234
 +      if (!bridge->done() && result == MessageQueueTerminated) {
Just curious - do we need the check for bridge->done() here and above (and any of the m_completed logic in the Bridge class)? I'm not seeing how runInMode() can return without the appropriate task having been fired (or the queue terminated).

(In reply to comment #20)
> Sorry, reviewer FAIL on that previous comment :(
> 
> Anyhow, just a quick drive-by:
> 
> WebKit/chromium/src/WebWorkerBase.cpp:221
>  +      WorkerScriptController* controller = WorkerScriptController::controllerForContext();
> We should be checking for controller == 0 here (allowDatabase() called while worker is shutting down) and returning false.

Dmitry stated in Comment #7 that this was not possible
> 
> WebKit/chromium/src/WebWorkerClientImpl.h:98
>  +      virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) { return true; }
> This should never be called on the renderer side, so I think this should be ASSERT_NOT_REACHED()


e.g. createApplicationCacheHost should not be called either, I'd rather add ASSERT_NOT_REACHED to all methods that should not be invoked (but I cannot judge which these are), or none.

wdyt?

> 
> WebKit/chromium/src/WebWorkerBase.cpp:66
>  +      static PassRefPtr<AllowDatabaseMainThreadBridge> create(WebWorkerBase* worker, const WebCore::String& mode, WebCommonWorkerClient* commonClient, WebFrame* frame, const WebCore::String& name, const WebCore::String& displayName, unsigned long estimatedSize) { return adoptRef(new AllowDatabaseMainThreadBridge(worker, mode, commonClient, frame, name, displayName, estimatedSize)); }
> This seems really hard to read as one big long line. Can we break this up? I'd suggest putting the function body on its own lines at least:
> 
> static PassRefPtr<...>create(.....)
> {
>   return adoptRef(...);
> }

done

> 
> 
> WebKit/chromium/src/WebWorkerBase.cpp:234
>  +      if (!bridge->done() && result == MessageQueueTerminated) {
> Just curious - do we need the check for bridge->done() here and above (and any of the m_completed logic in the Bridge class)? I'm not seeing how runInMode() can return without the appropriate task having been fired (or the queue terminated).

done

Created attachment 55673 [details]
Patch

[Andrew Wilson]

(In reply to comment #21)
> Dmitry stated in Comment #7 that this was not possible
I don't think that's correct. What he was saying is that you are never going to call this code outside of worker context, which I agree with. However, you could certainly call this code while the worker is shutting down, which will also cause controllerForContext() to return 0, I believe. Dmitry is indeed the authority here, but I'd like him to confirm that not checking for 0 is OK.

> 
> e.g. createApplicationCacheHost should not be called either, I'd rather add ASSERT_NOT_REACHED to all methods that should not be invoked (but I cannot judge which these are), or none.
> 
I understand your desire for consistency. If you want to change those other lines to be ASSERT_NOT_REACHED() also (and run the appropriate tests to make sure it doesn't break anything) I'm fine with that. The worker interface is confusing enough with various interfaces that are implemented but never called on certain sides of the renderer/worker divide, that I feel pretty strongly that we should be using ASSERT_NOT_REACHED() wherever possible, even if past authors haven't been rigorous in this regard.

[Michael Nordman]

> > e.g. createApplicationCacheHost should not be called either, I'd rather add ASSERT_NOT_REACHED to all methods that should not be invoked (but I cannot judge which these are), or none.
> > 
> I understand your desire for consistency. If you want to change those other
> lines to be ASSERT_NOT_REACHED() also (and run the appropriate tests to make
> sure it doesn't break anything) I'm fine with that. The worker interface is 
> confusing enough with various interfaces that are implemented but never called 
> on certain sides of the renderer/worker divide, that I feel pretty strongly
> that we should be using ASSERT_NOT_REACHED() wherever possible, even if past
> authors haven't been rigorous in this regard.

The traversing into and out of and back into (etc) in various processes really makes it challenging to grok the worker interfaces, especially when the same interfaces are used for very different purposes in different places. Interfaces like WebSharedWorker and WebWorker are difficult to get a handle on because they are both used by chrome to call into webkit and used by webkit to call out to chrome, and some of the methods are only applicable on one side or the other.

What may be nice is if the worker interfaces where more segregated than they are. One programming interface for use in the process that creates a javascript worker object. Another for use in the process that realizes the worker thread. Could possibly help to avoid NOT_REACHED methods altogether.

Created attachment 55815 [details]
Patch

(In reply to comment #23)
> (In reply to comment #21)
> > Dmitry stated in Comment #7 that this was not possible
> I don't think that's correct. What he was saying is that you are never going to call this code outside of worker context, which I agree with. However, you could certainly call this code while the worker is shutting down, which will also cause controllerForContext() to return 0, I believe. Dmitry is indeed the authority here, but I'd like him to confirm that not checking for 0 is OK.

I've added the check for 0 back in. It surely won't hurt.

> 
> > 
> > e.g. createApplicationCacheHost should not be called either, I'd rather add ASSERT_NOT_REACHED to all methods that should not be invoked (but I cannot judge which these are), or none.
> > 
> I understand your desire for consistency. If you want to change those other lines to be ASSERT_NOT_REACHED() also (and run the appropriate tests to make sure it doesn't break anything) I'm fine with that. The worker interface is confusing enough with various interfaces that are implemented but never called on certain sides of the renderer/worker divide, that I feel pretty strongly that we should be using ASSERT_NOT_REACHED() wherever possible, even if past authors haven't been rigorous in this regard.

I've added ASSERT_NOT_REACHED() to allowDatabase then

[Dmitry Titov]

(In reply to comment #24)
> The traversing into and out of and back into (etc) in various processes really makes it challenging to grok the worker interfaces, especially when the same interfaces are used for very different purposes in different places. Interfaces like WebSharedWorker and WebWorker are difficult to get a handle on because they are both used by chrome to call into webkit and used by webkit to call out to chrome, and some of the methods are only applicable on one side or the other.
> 
> What may be nice is if the worker interfaces where more segregated than they are. One programming interface for use in the process that creates a javascript worker object. Another for use in the process that realizes the worker thread. Could possibly help to avoid NOT_REACHED methods altogether.

This is certainly a painful issue. In Chromium setup, the number of layers, objects and interfaces that simply deliver a call from Worker object to WorkerContext and back clearly exceeds is the number of things the brain can keep simultaneously :-)

I think we will have an opportunity to re-visit this once we can run workers in the same process. Dedicated workers could be simply switched back to the renderer process and we might need a setup for shared workers that can support both in-process and IPC connections w/o much code required specifically for the latter.

In any case, patches that improve readability and simplify code are very welcome!

[Dmitry Titov]

Comment on attachment 55815 [details]
Patch

Looks great, would r+ with "change on landing" but since cq is needed, we need a perfect patch.

One small thing:

WebKit/chromium/src/WebWorkerBase.cpp:217
 +      // The controller might be 0 when the worker context is in the process of shutting down.
I see the discussion in the comments about this check. It is not really needed. There is another method which returns a V8 proxy (WorkerScriptController::proxy()) which indeed returns 0 when the termination of JS in the worker is requested. The WorkerScriptController::controllerForContext() always returns a non-null controller in worker context.
See http://trac.webkit.org/changeset/56580. That change split the old function that could return 0 if either the context is not WorkerContext or the termination was requested, to be able to make these checks separately. This is why I think there is no real case when we need an "if(!controller)" here and rather could just have an assert, or even better no check at all since the very next statement is going to crash reliably anyways.

[Michael Nordman]

> I think we will have an opportunity to re-visit this once we can run workers in the
> same process. Dedicated workers could be simply switched back to the renderer process
> and we might need a setup for shared workers that can support both in-process and IPC
> connections w/o much code required specifically for the latter.

Interesting point about the difference a multithreaded V8 could make. There would be a cost to making that switch, but it could be worth it.

Created attachment 56060 [details]
Patch

(In reply to comment #28)
> (From update of attachment 55815 [details])
> Looks great, would r+ with "change on landing" but since cq is needed, we need a perfect patch.
> 
> One small thing:
> 
> WebKit/chromium/src/WebWorkerBase.cpp:217
>  +      // The controller might be 0 when the worker context is in the process of shutting down.
> I see the discussion in the comments about this check. It is not really needed. There is another method which returns a V8 proxy (WorkerScriptController::proxy()) which indeed returns 0 when the termination of JS in the worker is requested. The WorkerScriptController::controllerForContext() always returns a non-null controller in worker context.
> See http://trac.webkit.org/changeset/56580. That change split the old function that could return 0 if either the context is not WorkerContext or the termination was requested, to be able to make these checks separately. This is why I think there is no real case when we need an "if(!controller)" here and rather could just have an assert, or even better no check at all since the very next statement is going to crash reliably anyways.

hope it's perfect now :)

[Dmitry Titov]

Comment on attachment 56060 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 56060 [details]
Patch

Rejecting patch 56060 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
WebKit/chromium/src/WebWorkerClientImpl.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/chromium/TestWebWorker.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebKitTools/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/2313110

[Adam Barth]

"Need a short description and bug URL (OOPS!)" was left in the second ChangeLog.  I'll clean it up for you.

[Adam Barth]

Created attachment 56158 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 56158 [details]
Patch for landing

Clearing flags on attachment: 56158

Committed r59555: <http://trac.webkit.org/changeset/59555>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.