Bug 36595 – Forward DatabaseTracker::canEstablishDatabase to chromium layer.

• Component: New Bugs
• Status: RESOLVED FIXED

Forward DatabaseTracker::canEstablishDatabase to chromium layer.

Created attachment 51633 [details]
Patch

[WebKit Review Bot]

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.

[Jeremy Orlow]

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>);
>

(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!

(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>);
> >

[Jeremy Orlow]

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

[WebKit Commit Bot]

Comment on attachment 51633 [details]
Patch

Clearing flags on attachment: 51633

Committed r56548: <http://trac.webkit.org/changeset/56548>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andrew Wilson]

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

[Michael Nordman]

> 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());

[Darin Fisher (:fishd, Google)]

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.

[Jeremy Orlow]

Re-opening to reflect the fact that Darin has provided additional comments on this.

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

[Andrew Wilson]

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.