Bug 36795

Summary: [chromium] Problems passing allowDatabase() off to the chrome layer
Product: WebKit Reporter: Andrew Wilson <atwilson>
Component: New BugsAssignee: jochen
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, dimich, ericu, fishd, jorlow, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Andrew Wilson 2010-03-29 17:23:04 PDT
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
Comment 1 jochen 2010-03-30 04:13:56 PDT
Created attachment 52022 [details]
Patch
Comment 2 jochen 2010-03-30 04:17:34 PDT
Created attachment 52023 [details]
Patch
Comment 3 Jeremy Orlow 2010-03-30 04:18:37 PDT
Drew, does this look good to you?
Comment 4 WebKit Review Bot 2010-03-30 04:20:42 PDT
Attachment 52023 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1593077
Comment 5 jochen 2010-03-30 05:04:12 PDT
Created attachment 52024 [details]
Patch
Comment 6 jochen 2010-03-30 05:08:42 PDT
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.
Comment 7 Jeremy Orlow 2010-03-30 05:15:18 PDT
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 @@
> +2010-03-30  Jochen Eisinger  <jochen@chromium.org>
> +
> +        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  <rafaelw@chromium.org>
>  
>          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>);
>
Comment 8 jochen 2010-03-30 05:49:28 PDT
Created attachment 52026 [details]
Patch
Comment 9 Jeremy Orlow 2010-03-30 05:57:58 PDT
r=me
Comment 10 WebKit Commit Bot 2010-03-30 06:46:02 PDT
Comment on attachment 52026 [details]
Patch

Clearing flags on attachment: 52026

Committed r56782: <http://trac.webkit.org/changeset/56782>
Comment 11 WebKit Commit Bot 2010-03-30 06:46:08 PDT
All reviewed patches have been landed.  Closing bug.