[Webkit-unassigned] [Bug 22725] Make SQL database storage work in Workers
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 8 17:32:26 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=22725
--- Comment #7 from Dmitry Titov <dimich at chromium.org> 2010-01-08 17:32:25 PST ---
(From update of attachment 46164)
Database in Workers is great, and the patch look good. Here is the first look's
result (not complete yet):
> Index: WebCore/ChangeLog
This is quite a significant change, and ChangeLog could use more comments on
what and why was actually refactored.
Right now, it has no comments at all except the generic description on top.
Depending on how people search trac or ChangeLogs, more info is useful.
> Index: WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
> +#if ENABLE(DATABASE)
> +#include "Database.h"
> +#endif
Database.h already contains this #ifdef inside. No need to duplicate.
> Index: WebCore/dom/Document.cpp
> +bool Document::isDatabaseReadOnly() const
> +{
> + Page* page = document()->page();
> + if (!page || page->settings()->privateBrowsingEnabled())
> + return true;
> + return false;
> +}
> +
> +void Document::databaseExceededQuota(const String& name)
> +{
> + Page* currentPage = page();
> + ASSERT(currentPage);
> + currentPage->chrome()->client()->exceededDatabaseQuota(document()->frame(), name);
> +}
One of those expects page() to be possibly 0, while another asserts on it. If
there is indeed such a difference in expectations, it'd be nice to have a
comment explaining it.
> +bool Document::isJavaScriptThread() const
> +{
> + return WTF::isMainThread();
> +}
I think this new virtual function is redundant. For the cases it is used in, it
always means "not the database thread", and it seems the database thread ID is
available in all points of use.
> Index: WebCore/dom/Document.h
> - Page* page() const; // can be NULL
> + virtual Page* page() const; // can be NULL
page() is a pretty 'hot' function. Making it virtual may not improve perf, a
lot of work has been done in WebKit to remove 'hot' virtuals...
In fact, I think SEC does not need this virtual - looking at how it is used in
the patch, it is in fact detecting that the context is not a document (it
always returns 0 if context is not a Document).
We already have ScriptExecutionContext::isDocument() that can be used instead
of checking the returning page for 0.
> + virtual bool isJavaScriptThread() const;
Probably unnecessary method, see above.
> Index: WebCore/dom/ScriptExecutionContext.cpp
> ===================================================================
> --- WebCore/dom/ScriptExecutionContext.cpp (revision 52995)
> +++ WebCore/dom/ScriptExecutionContext.cpp (working copy)
> @@ -28,7 +28,8 @@
> #include "ScriptExecutionContext.h"
>
> #include "ActiveDOMObject.h"
> -#include "Document.h"
> +#include "DatabaseTask.h"
> +#include "DatabaseThread.h"
> #include "MessagePort.h"
> #include "SecurityOrigin.h"
> #include "WorkerContext.h"
> @@ -56,6 +57,9 @@ public:
> };
>
> ScriptExecutionContext::ScriptExecutionContext()
> +#if ENABLE(DATABASE)
> + : m_hasOpenDatabases(false)
> +#endif
> {
> }
>
> @@ -72,8 +76,64 @@ ScriptExecutionContext::~ScriptExecution
> ASSERT((*iter)->scriptExecutionContext() == this);
> (*iter)->contextDestroyed();
> }
> + if (m_databaseThread) {
> + ASSERT(m_databaseThread->terminationRequested());
> + m_databaseThread = 0;
> + }
> }
>
> +#if ENABLE(DATABASE)
> +
> +DatabaseThread* ScriptExecutionContext::databaseThread()
> +{
> + if (!m_databaseThread && !m_hasOpenDatabases) {
> + // Create the database thread on first request - but not if at least one database was already opened,
> + // because in that case we already had a database thread and terminated it and should not create another.
> + m_databaseThread = DatabaseThread::create();
> + if (!m_databaseThread->start())
> + m_databaseThread = 0;
> + }
> +
> + return m_databaseThread.get();
> +}
> +
> +void ScriptExecutionContext::addOpenDatabase(Database* database)
> +{
> + if (!m_openDatabaseSet)
> + m_openDatabaseSet.set(new DatabaseSet());
> +
> + ASSERT(!m_openDatabaseSet->contains(database));
> + m_openDatabaseSet->add(database);
> +}
> +
> +void ScriptExecutionContext::removeOpenDatabase(Database* database)
> +{
> + ASSERT(m_openDatabaseSet && m_openDatabaseSet->contains(database));
> + if (!m_openDatabaseSet)
> + return;
> + m_openDatabaseSet->remove(database);
> +}
> +
> +void ScriptExecutionContext::stopDatabases(DatabaseTaskSynchronizer *cleanupSync)
> +{
> + if (m_openDatabaseSet) {
> + DatabaseSet::iterator i = m_openDatabaseSet->begin();
> + DatabaseSet::iterator end = m_openDatabaseSet->end();
> + for (; i != end; ++i) {
> + (*i)->stop();
> + if (m_databaseThread)
> + m_databaseThread->unscheduleDatabaseTasks(*i);
> + }
> + }
> +
> + if (m_databaseThread)
> + m_databaseThread->requestTermination(cleanupSync);
> + else if (cleanupSync)
> + cleanupSync->taskCompleted();
> +}
> +
> +#endif
> +
> void ScriptExecutionContext::processMessagePortMessagesSoon()
> {
> postTask(ProcessMessagesSoonTask::create());
> Index: WebCore/dom/ScriptExecutionContext.h
> + class Database;
> + class DatabaseTaskSynchronizer;
> + class DatabaseThread;
@if ENABLE(DATABASE) ?
> + class Page;
If 'virtual Page page();' is not here, this can be removed as well.
> + void stopDatabases(DatabaseTaskSynchronizer *cleanupSync);
> + // Certain tasks get marked specially so that we don't forget to run
> + // them while shutting down.
WebKit usually does not try to wrap lines at 80 characters. It's also not clear
what "we don't forget" means..
Perhaps 'so they are not discarded when context is shutting down its message
queue'?
> Index: WebCore/loader/FrameLoader.cpp
> #if ENABLE(DATABASE)
> + // TODO(ericu): Anything about worker databases here?
> && !m_frame->document()->hasOpenDatabases()
If doc has workers, the page is not cacheable because they are ActiveDOMObjects
that can not be suspended.
If they ever will be, the Worker::canSuspend() will have to verify this for all
internal objects, including databases.
So this TODO can be removed (or re-phrased).
> +#if ENABLE(DATABASE) // TODO(ericu): Likewise, anything about worker databases?
> if (m_frame->document()->hasOpenDatabases())
Ditto.
> Index: WebCore/storage/Database.cpp
> -#include "InspectorController.h"
> +#if ENABLE(INSPECTOR)
> +#include "InspectorController.h"
> #endif
Not sure why this include moved and gained ENABLE(INSPECTOR) protector...
> #if ENABLE(INSPECTOR)
> - if (Page* page = document->frame()->page())
> - page->inspectorController()->didOpenDatabase(database.get(), document->securityOrigin()->host(), name, expectedVersion);
> + if (Page* page = database->executionContext()->page())
> + page->inspectorController()->didOpenDatabase(database.get(), context->securityOrigin()->host(), name, expectedVersion);
That I think is the only place in this patch that uses virtual page() method.
Can just check for executionContext()->isDocument() and cast for now.
We'll need to figure out how to hook up inspector to workers separately...
> Database::~Database()
> {
> - // Deref m_document on the main thread.
> - callOnMainThread(derefDocument, m_document.release().releaseRef());
> + if (!m_executionContext->isJavaScriptThread()) {
> + // The reference to the ScriptExecutionContext needs to be cleared on
> + // the JavaScript thread, not the Database thread. However, if we're
> + // already on that thread, for example cleaning up all databases during
> + // worker thread shutdown, we don't want to post a task because it might
> + // hit the queue after the WorkerThreadShutdownFinishTask and never get
> + // executed.
> + m_executionContext->postTask(DerefContextTask::create(
> + m_executionContext.release().releaseRef()));
The comment sounds a bit confusing. If it is bad that sometimes deref won't
happen if we have to post the task to the context's thread, how it is better to
never deref at all?
I will do more review soon, please feel free to update the patch (or wait until
I go through all of it). I'm leaning to r- it at the end due to 'virtual Page
page()', but in general it looks good.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list