Bug 36595 - Forward DatabaseTracker::canEstablishDatabase to chromium layer.
: Forward DatabaseTracker::canEstablishDatabase to chromium layer.
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-03-25 06:58 PST by
Modified: 2010-03-29 16:53 PST (History)


Attachments
Patch (8.04 KB, patch)
2010-03-25 06:59 PST, jochen@chromium.org
fishd: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-25 06:58:51 PST
Forward DatabaseTracker::canEstablishDatabase to chromium layer.
------- Comment #1 From 2010-03-25 06:59:29 PST -------
Created an attachment (id=51633) [details]
Patch
------- Comment #2 From 2010-03-25 07:03:43 PST -------
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 From 2010-03-25 07:07:38 PST -------
(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.

> +
>      // 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 From 2010-03-25 07:42:07 PST -------
(In reply to comment #2)
> Attachment 51633 [details] [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 From 2010-03-25 07:44:03 PST -------
(In reply to comment #3)
> (From update of attachment 51633 [details] [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 From 2010-03-25 07:52:20 PST -------
(From update of attachment 51633 [details])
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 From 2010-03-25 08:25:49 PST -------
(From update of attachment 51633 [details])
Clearing flags on attachment: 51633

Committed r56548: <http://trac.webkit.org/changeset/56548>
------- Comment #8 From 2010-03-25 08:25:54 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #9 From 2010-03-25 11:28:04 PST -------
(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 From 2010-03-25 18:41:55 PST -------
> 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 From 2010-03-26 16:56:33 PST -------
(From update of attachment 51633 [details])
> +++ 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 From 2010-03-29 03:23:01 PST -------
Re-opening to reflect the fact that Darin has provided additional comments on this.
------- Comment #13 From 2010-03-29 04:26:27 PST -------
(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 From 2010-03-29 16:53:47 PST -------
(From update of attachment 51633 [details])
>
> +        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.