Bug 36795 – [chromium] Problems passing allowDatabase() off to the chrome layer

• Component: New Bugs
• Status: RESOLVED FIXED

[Andrew Wilson]

These comments should have been in bug #36595, but since that's closed I'm putting them here. Sorry these comments are coming so late, but the webkit->chrome interface for workers is really complex and I missed/forgot some of the subtleties when doing my previous review pass.

>
> +        WebWorkerImpl* webWorker = reinterpret_cast<WebWorkerImpl*>(workerLoaderProxy);

I think this line should cast to WebWorkerBase, not WebWorkerImpl since
technically the object could be a WebSharedWorkerImpl instead.

Additionally, I would change WebWorkerBase::allowDatabase() so it is not virtual (no need to be virtual) and have it call commonClient()->allowDatabase() instead of returning true by default.

I'd define allowDatabase() as a virtual function on WebCommonWorkerClient. You can then implement allowDatabase() in the chromium version of WebCommonWorkerClient in chrome/worker/webworkerclient_proxy.h

Created attachment 52022 [details]
Patch

Created attachment 52023 [details]
Patch

[Jeremy Orlow]

Drew, does this look good to you?

[WebKit Review Bot]

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

Created attachment 52024 [details]
Patch

Comment on attachment 52024 [details]
Patch

I'm removing the worker related bits for now. Getting this right requires more work, both on WebKit and on Chrome, so I'd like to postpone that for now.

Taking into account that workers don't fully implement database access yet, I think that's the right way to go for now.

[Jeremy Orlow]

Comment on attachment 52024 [details]
Patch

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 551c957..bdec06d 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,17 @@
> [phone]  Jochen Eisinger  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Remove dysfunctional implementation of canEstablishDatabase for
> +        Workers. I postpone this implementation until Workers can actually
> +        access Web Databases.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=36795
> +
> +        * src/DatabaseObserver.cpp:
> +        (WebCore::DatabaseObserver::canEstablishDatabase):
> +        * src/WebWorkerBase.h:
> +
>  2010-03-29  Rafael Weinstein  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> diff --git a/WebKit/chromium/src/DatabaseObserver.cpp b/WebKit/chromium/src/DatabaseObserver.cpp
> index e89bd5d..9ff5c8a 100644
> --- a/WebKit/chromium/src/DatabaseObserver.cpp
> +++ b/WebKit/chromium/src/DatabaseObserver.cpp
> @@ -50,17 +50,17 @@ namespace WebCore {
>  bool DatabaseObserver::canEstablishDatabase(ScriptExecutionContext* scriptExecutionContext, const String& name, const String& displayName, unsigned long estimatedSize)
>  {
>      ASSERT(scriptExecutionContext->isContextThread());
> -    ASSERT(scriptExecutionContext->isDocument() || scriptExecutionContext->isWorkerContext());
> +    // TODO(jochen): add support for the case

FIXME: not TODO():

> +    // scriptExecutionContext()->isWorker() once workers implement web
> +    // databases.

No 80 line wrapping.

> +    ASSERT(scriptExecutionContext->isDocument());
>      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* worker = static_cast<WorkerContext*>(scriptExecutionContext);
> -        WorkerLoaderProxy* workerLoaderProxy = &worker->thread()->workerLoaderProxy();
> -        WebWorkerImpl* webWorker = reinterpret_cast<WebWorkerImpl*>(workerLoaderProxy);
> -        return webWorker->allowDatabase(WebSecurityOrigin(scriptExecutionContext->securityOrigin()), name, displayName, estimatedSize);
>      }
> +
> +    return true;
>  }
>  
>  void DatabaseObserver::databaseOpened(Database* database)
> diff --git a/WebKit/chromium/src/WebWorkerBase.h b/WebKit/chromium/src/WebWorkerBase.h
> index c50d4a3..d7e51fa 100644
> --- a/WebKit/chromium/src/WebWorkerBase.h
> +++ b/WebKit/chromium/src/WebWorkerBase.h
> @@ -80,9 +80,6 @@ public:
>      virtual void postTaskForModeToWorkerContext(
>          PassOwnPtr<WebCore::ScriptExecutionContext::Task>, const WebCore::String& mode);
>  
> -    // Controls whether access to Web Databases is allowed for this worker.
> -    virtual bool allowDatabase(const WebSecurityOrigin&, const WebString&, const WebString&, unsigned long) { return true; }
> -
>      // Executes the given task on the main thread.
>      static void dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
>

Created attachment 52026 [details]
Patch

[Jeremy Orlow]

r=me

[WebKit Commit Bot]

Comment on attachment 52026 [details]
Patch

Clearing flags on attachment: 52026

Committed r56782: <http://trac.webkit.org/changeset/56782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.