[Webkit-unassigned] [Bug 22725] Make SQL database storage work in Workers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22725


Dmitry Titov <dimich at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46193|review?                     |review-
               Flag|                            |




--- Comment #12 from Dmitry Titov <dimich at chromium.org>  2010-01-09 16:11:36 PST ---
(From update of attachment 46193)
I think you can ignore first style bot complaint about indenting stuff inside
namespace in WorkerContext.h. This is a recently-changed rule but this file was
already indented. The other one is good to fix though.


> Index: WebCore/dom/Document.cpp
> +bool Document::isDatabaseReadOnly() const
> +{
> +    Page* page = document()->page();

no need to use document() here. 'this' is a document.


> +void Document::databaseExceededQuota(const String& name)
> +{
> +    Page* currentPage = page();
> +    ASSERT(currentPage);
> +    currentPage->chrome()->client()->exceededDatabaseQuota(document()->frame(), name);

Ditto. document().

> Index: WebCore/dom/Document.h

> +    virtual bool isJavaScriptThread() const;

See previous reply. If it is actually needed, the better name seems to be
isContextThread().


> Index: WebCore/dom/ScriptExecutionContext.h

> +        void stopDatabases(DatabaseTaskSynchronizer *cleanupSync);

The parameter name is usually omitted here.

> +            // Certain tasks get marked specially so that they aren't discarded, and are executed, when the context is shutting down its message queue.
> +            virtual bool isCleanupTask() const { return false; }

I don't think it is possible to keep this contract for all
ScriptExecutionContexts, for example Document does not control the run loop of
the main thread so the tasks can be dropped even if they are marked as 'cleanup
tasks'. Not sure how to change it - maybe have a specific WorkerTask class and
add it there?

> Index: WebCore/storage/Database.cpp

> -    ASSERT(m_document->databaseThread());
> -
> -    m_filename = DatabaseTracker::tracker().fullPathForDatabase(m_mainThreadSecurityOrigin.get(), m_name);
> +    m_filename = DatabaseTracker::tracker().fullPathForDatabase(securityOrigin(), m_name);

Is the removal of ASSERT intentional?

> +private:
> +    explicit DerefContextTask(ScriptExecutionContext *context) : m_context(context)
> +    {
> +    }

the initializers (": m_context(conetxt)") should be on a separate line,
indented 4 spaces.

> Index: WebCore/storage/Database.h

> +    ScriptExecutionContext* executionContext() const { return m_executionContext.get(); }

In other classes, this is called 'scriptExecutionContext()' (see Node.h for
example). Lets rename it, for consistency.

> Index: WebCore/storage/DatabaseTracker.h

>  class DatabaseTracker : public Noncopyable {
>  public:
>      static DatabaseTracker& tracker();
> +    // NOTE: If at some point anyone implements multiple worker threads
> +    // running in the same process, this singleton will have to be synchronized.

Very explosive NOTE you have here :-) This is exactly what happens in
non-Chromium builds - workers run in their own threads in the same process. So
DatabaseTracker will have to be changed now. It can be a separate patch though.

You said you tested a prototype with Chromium bindings, but in Chromium Workers
run in separate processes so there is no multiple Worker threads in the picture
at all. Also, database code is not used from main thread in a Worker process
since there is no Documents loaded. JSC case provides better threading 'test
environment' so it's good to get JSC prototype up and running in order to get
these things right.

Also, in WebKit the comments usually don't use 'NOTE:', the comment by itself
is a note. FIXME perhaps.

> Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp

>  void DatabaseTracker::addOpenDatabase(Database* database)
>  {
> -    ASSERT(isMainThread());
> +    ASSERT(database->executionContext()->isJavaScriptThread());
>      DatabaseObserver::databaseOpened(database);
>  }

Not sure this is correct replacement... It documents an assumption that this
method is ok to call on multiple threads. Is it?

> +class TrackerRemoveOpenDatabaseTask : public ScriptExecutionContext::Task {
> +    virtual void performTask(ScriptExecutionContext *context)
> +    {
> +        ASSERT(context->isJavaScriptThread());

No need for ASSERT - by design the task is executed on the context's thread.

>  void DatabaseTracker::removeOpenDatabase(Database* database)
>  {
> -    if (!isMainThread()) {
> +    if (!database->executionContext()->isJavaScriptThread()) {

Does it mean the main thread is actually ok here? It can be that context's
thread is not the main thread... It would be simpler to just check for database
thread here.

> Index: WebCore/storage/chromium/SQLTransactionClientChromium.cpp

> +    virtual void performTask(ScriptExecutionContext* context)
> +    {
> +        WebCore::DatabaseObserver::databaseModified(m_database.get());
> +    }

'context' is an unused parameter. Can be omitted.

> Index: WebCore/workers/WorkerContext.cpp

> +#if ENABLE(DATABASE)
> +#include "Database.h"
> +#endif

ENABLE() guard is not needed here.

> Index: WebCore/workers/WorkerContext.h

> +#if ENABLE(DATABASE)
> +#include "Database.h"
> +#endif

Ditto.

> +        virtual bool isDatabaseReadOnly() const { return false; /* TODO(ericu) */ }
> +        virtual void databaseExceededQuota(const String&) { /* TODO(ericu) */}

There is no TODO(name) in WebKit. A comment above the methods saying they are
not yet implemented would suffice. It is also a good idea to file bug[s] to
actually implement those.

> Index: WebCore/workers/WorkerThread.cpp

> +class WorkerThreadShutdownFinishTask : public ScriptExecutionContext::Task {
> +    virtual void performTask(ScriptExecutionContext *context)
> +    {
> +        ASSERT(context->isJavaScriptThread());
> +        m_workerContext->thread()->runLoop().terminate();

ASSERT is not needed (the postTask ensures that). Also, m_workerContext is the
same thing as 'context' parameter - better use the parameter.

> +class WorkerThreadShutdownStartTask : public ScriptExecutionContext::Task {
> +    virtual void performTask(ScriptExecutionContext *context)
> +    {
> +        ASSERT(context->isJavaScriptThread());
> +        // We currently ignore any DatabasePolicy used for the document's
> +        // databases; if it's actually used anywhere, this should be revisited.
> +        DatabaseTaskSynchronizer cleanupSync;
> +        m_workerContext->stopDatabases(&cleanupSync);

Same as above.

>  void WorkerThread::stop()

> -    m_runLoop.terminate();
> +    // NOTE: this can likely use the same mechanism as used for databases above.
> +
> +        m_runLoop.postTask(WorkerThreadShutdownStartTask::create(m_workerContext.get()));

How did you verify that the code that calls stop() does not depend on
synchronous result of terminate()?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.