Bug 36595 - Forward DatabaseTracker::canEstablishDatabase to chromium layer.
Summary: Forward DatabaseTracker::canEstablishDatabase to chromium layer.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-25 06:58 PDT by jochen
Modified: 2010-03-29 16:53 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.04 KB, patch)
2010-03-25 06:59 PDT, jochen
fishd: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2010-03-25 06:58:51 PDT
Forward DatabaseTracker::canEstablishDatabase to chromium layer.
Comment 1 jochen 2010-03-25 06:59:29 PDT
Created attachment 51633 [details]
Patch
Comment 2 WebKit Review Bot 2010-03-25 07:03:43 PDT
Attachment 51633 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/DatabaseObserver.cpp:54:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jeremy Orlow 2010-03-25 07:07:38 PDT
Comment on attachment 51633 [details]
Patch

r+ from me, but leaving it r? for now to give some others a chance to chime in.

> diff --git a/WebKit/chromium/public/WebFrameClient.h b/WebKit/chromium/public/WebFrameClient.h
> index c6f51b6..5ffc2e3 100644
> --- a/WebKit/chromium/public/WebFrameClient.h
> +++ b/WebKit/chromium/public/WebFrameClient.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2009 Google Inc. All rights reserved.
> + * Copyright (C) 2010 Google Inc. All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -263,6 +263,9 @@ public:
>      // Controls whether scripts are allowed to execute for this frame.
>      virtual bool allowScript(WebFrame*, bool enabledPerSettings) { return enabledPerSettings; }
>  
> +    // Controls whether access to Web Databases is allowed for this frame.
> +    virtual bool allowDatabase(const WebSecurityOrigin&, const WebString&, const WebString&, unsigned long) { return true; }

If anything, I'd probably default this to false...but do what you think is best.

> +
>      // Notifies the client that the frame would have executed script if script were enabled.
>      virtual void didNotAllowScript(WebFrame*) { }
>  
> diff --git a/WebKit/chromium/src/DatabaseObserver.cpp b/WebKit/chromium/src/DatabaseObserver.cpp
> index 54e93e1..1f13351 100644
> --- a/WebKit/chromium/src/DatabaseObserver.cpp
> +++ b/WebKit/chromium/src/DatabaseObserver.cpp
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2009 Google Inc. All rights reserved.
> + * Copyright (C) 2010 Google Inc. All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -32,13 +32,37 @@
>  #include "DatabaseObserver.h"
>  
>  #include "Database.h"
> +#include "Document.h"
> +#include "ScriptExecutionContext.h"
>  #include "WebDatabase.h"
>  #include "WebDatabaseObserver.h"
> +#include "WebFrameClient.h"
> +#include "WebFrameImpl.h"
> +#include "WebSecurityOrigin.h"
> +#include "WebWorkerImpl.h"
> +#include "WorkerContext.h"
> +#include "WorkerThread.h"
>  
>  using namespace WebKit;
>  
>  namespace WebCore {
>  
> +bool DatabaseObserver::canEstablishDatabase(ScriptExecutionContext* scriptExecutionContext, const String& name, const String& displayName, unsigned long estimatedSize)
> +{
> +    ASSERT(isMainThread());
> +    ASSERT(scriptExecutionContext->isDocument() || scriptExecutionContext->isWorkerContext());
> +    if (scriptExecutionContext->isDocument()) {
> +        Document* document = static_cast<Document*>(scriptExecutionContext);
> +        WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());
> +        return webFrame->client()->allowDatabase(WebSecurityOrigin(scriptExecutionContext->securityOrigin()), 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);
> +    }

I don't know for sure whether this is the proper way to do this.  Seems plausible tho.

> +}
> +
>  void DatabaseObserver::databaseOpened(Database* database)
>  {
>      ASSERT(isMainThread());
> diff --git a/WebKit/chromium/src/WebWorkerBase.h b/WebKit/chromium/src/WebWorkerBase.h
> index 0217401..c50d4a3 100644
> --- a/WebKit/chromium/src/WebWorkerBase.h
> +++ b/WebKit/chromium/src/WebWorkerBase.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2009 Google Inc. All rights reserved.
> + * Copyright (C) 2010 Google Inc. All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -45,8 +45,11 @@ class WorkerThread;
>  
>  namespace WebKit {
>  class WebCommonWorkerClient;
> +class WebSecurityOrigin;
> +class WebString;
>  class WebURL;
>  class WebView;
> +class WebWorker;
>  class WebWorkerClient;
>  
>  // Base class for WebSharedWorkerImpl and WebWorkerImpl. It contains common
> @@ -77,6 +80,9 @@ 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; }

If anything, I'd probably default this to false...but do what you think is best.

> +
>      // Executes the given task on the main thread.
>      static void dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
>
Comment 4 jochen 2010-03-25 07:42:07 PDT
(In reply to comment #2)
> Attachment 51633 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/chromium/src/DatabaseObserver.cpp:54:  An else statement can be removed
> when the prior "if" concludes with a return, break, continue or goto statement.
>  [readability/control_flow] [4]
> Total errors found: 1 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

it looks so much better this way!
Comment 5 jochen 2010-03-25 07:44:03 PDT
(In reply to comment #3)
> (From update of attachment 51633 [details])
> r+ from me, but leaving it r? for now to give some others a chance to chime in.
> 
> > diff --git a/WebKit/chromium/public/WebFrameClient.h b/WebKit/chromium/public/WebFrameClient.h
> > index c6f51b6..5ffc2e3 100644
> > --- a/WebKit/chromium/public/WebFrameClient.h
> > +++ b/WebKit/chromium/public/WebFrameClient.h
> > @@ -1,5 +1,5 @@
> >  /*
> > - * Copyright (C) 2009 Google Inc. All rights reserved.
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> >   *
> >   * Redistribution and use in source and binary forms, with or without
> >   * modification, are permitted provided that the following conditions are
> > @@ -263,6 +263,9 @@ public:
> >      // Controls whether scripts are allowed to execute for this frame.
> >      virtual bool allowScript(WebFrame*, bool enabledPerSettings) { return enabledPerSettings; }
> >  
> > +    // Controls whether access to Web Databases is allowed for this frame.
> > +    virtual bool allowDatabase(const WebSecurityOrigin&, const WebString&, const WebString&, unsigned long) { return true; }
> 
> If anything, I'd probably default this to false...but do what you think is
> best.

The old implementation would return true - this way, other embedders using the chromium layer won't get bidden by changed behavior.

> 
> > +
> >      // Notifies the client that the frame would have executed script if script were enabled.
> >      virtual void didNotAllowScript(WebFrame*) { }
> >  
> > diff --git a/WebKit/chromium/src/DatabaseObserver.cpp b/WebKit/chromium/src/DatabaseObserver.cpp
> > index 54e93e1..1f13351 100644
> > --- a/WebKit/chromium/src/DatabaseObserver.cpp
> > +++ b/WebKit/chromium/src/DatabaseObserver.cpp
> > @@ -1,5 +1,5 @@
> >  /*
> > - * Copyright (C) 2009 Google Inc. All rights reserved.
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> >   *
> >   * Redistribution and use in source and binary forms, with or without
> >   * modification, are permitted provided that the following conditions are
> > @@ -32,13 +32,37 @@
> >  #include "DatabaseObserver.h"
> >  
> >  #include "Database.h"
> > +#include "Document.h"
> > +#include "ScriptExecutionContext.h"
> >  #include "WebDatabase.h"
> >  #include "WebDatabaseObserver.h"
> > +#include "WebFrameClient.h"
> > +#include "WebFrameImpl.h"
> > +#include "WebSecurityOrigin.h"
> > +#include "WebWorkerImpl.h"
> > +#include "WorkerContext.h"
> > +#include "WorkerThread.h"
> >  
> >  using namespace WebKit;
> >  
> >  namespace WebCore {
> >  
> > +bool DatabaseObserver::canEstablishDatabase(ScriptExecutionContext* scriptExecutionContext, const String& name, const String& displayName, unsigned long estimatedSize)
> > +{
> > +    ASSERT(isMainThread());
> > +    ASSERT(scriptExecutionContext->isDocument() || scriptExecutionContext->isWorkerContext());
> > +    if (scriptExecutionContext->isDocument()) {
> > +        Document* document = static_cast<Document*>(scriptExecutionContext);
> > +        WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());
> > +        return webFrame->client()->allowDatabase(WebSecurityOrigin(scriptExecutionContext->securityOrigin()), 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);
> > +    }
> 
> I don't know for sure whether this is the proper way to do this.  Seems
> plausible tho.

I've copied this more or less from WebWorkerClientImpl::createWorkerContextProxy


> 
> > +}
> > +
> >  void DatabaseObserver::databaseOpened(Database* database)
> >  {
> >      ASSERT(isMainThread());
> > diff --git a/WebKit/chromium/src/WebWorkerBase.h b/WebKit/chromium/src/WebWorkerBase.h
> > index 0217401..c50d4a3 100644
> > --- a/WebKit/chromium/src/WebWorkerBase.h
> > +++ b/WebKit/chromium/src/WebWorkerBase.h
> > @@ -1,5 +1,5 @@
> >  /*
> > - * Copyright (C) 2009 Google Inc. All rights reserved.
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> >   *
> >   * Redistribution and use in source and binary forms, with or without
> >   * modification, are permitted provided that the following conditions are
> > @@ -45,8 +45,11 @@ class WorkerThread;
> >  
> >  namespace WebKit {
> >  class WebCommonWorkerClient;
> > +class WebSecurityOrigin;
> > +class WebString;
> >  class WebURL;
> >  class WebView;
> > +class WebWorker;
> >  class WebWorkerClient;
> >  
> >  // Base class for WebSharedWorkerImpl and WebWorkerImpl. It contains common
> > @@ -77,6 +80,9 @@ 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; }
> 
> If anything, I'd probably default this to false...but do what you think is
> best.
> 
> > +
> >      // Executes the given task on the main thread.
> >      static void dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
> >
Comment 6 Jeremy Orlow 2010-03-25 07:52:20 PDT
Comment on attachment 51633 [details]
Patch

Oh, well assuming the createWorkerContextProxy code is correct, this should be fine too.  I wonder if it should be factored out so it can be shared, but it seems different enough and they're in different layers that it's probably OK to leave it.

r=me
Comment 7 WebKit Commit Bot 2010-03-25 08:25:49 PDT
Comment on attachment 51633 [details]
Patch

Clearing flags on attachment: 51633

Committed r56548: <http://trac.webkit.org/changeset/56548>
Comment 8 WebKit Commit Bot 2010-03-25 08:25:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Andrew Wilson 2010-03-25 11:28:04 PDT
(In reply to comment #8)
> All reviewed patches have been landed.  Closing bug.

BTW, I suspect that calling ASSERT(isMainThread()) from DatabaseObserver::canEstablishDatabase() will fail when invoked from worker context. So you'll probably need to remove that assertion.
Comment 10 Michael Nordman 2010-03-25 18:41:55 PDT
> BTW, I suspect that calling ASSERT(isMainThread()) from
> DatabaseObserver::canEstablishDatabase() will fail when invoked from worker
> context. So you'll probably need to remove that assertion.

What you probably want here is...
ASSERT(scriptContext->isContextThread());
Comment 11 Darin Fisher (:fishd, Google) 2010-03-26 16:56:33 PDT
Comment on attachment 51633 [details]
Patch

> +++ b/WebKit/chromium/public/WebFrameClient.h

>      virtual bool allowScript(WebFrame*, bool enabledPerSettings) { return enabledPerSettings; }
>  
> +    // Controls whether access to Web Databases is allowed for this frame.
> +    virtual bool allowDatabase(const WebSecurityOrigin&, const WebString&, const WebString&, unsigned long) { return true; }

Every method of this interface must start with a WebFrame pointer.

Given a WebFrame pointer, you can get the security origin, so there
is no need for the WebSecurityOrigin parameter.

The WebString and 'unsigned long' parameters should be named since
there is no telling from this file what they might mean.  It is
important to document the WebKit API and to do that we leverage
self-documenting variable names whenever possible.
Comment 12 Jeremy Orlow 2010-03-29 03:23:01 PDT
Re-opening to reflect the fact that Darin has provided additional comments on this.
Comment 13 jochen 2010-03-29 04:26:27 PDT
(In reply to comment #12)
> Re-opening to reflect the fact that Darin has provided additional comments on
> this.

I've opened #36743 for this.
Comment 14 Andrew Wilson 2010-03-29 16:53:47 PDT
Comment on attachment 51633 [details]
Patch

>
> +        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.