[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





--- Comment #9 from Eric U. <ericu at chromium.org>  2010-01-08 19:58:19 PST ---
(In reply to comment #7)
> > 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.

I've added a bunch; how's that?

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

Fixed.

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

Hmm...I'll look around a bit.  That's actually code I relocated, that was
already there.  The assumptions about page() have more to do with the call
sites from which I pulled the code than they really should, given the new
abstraction.  I'll keep looking into that, but as I've addressed everything
else, I'm uploading this new patch for your review.

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

The database thread is available for checking anywhere you can call
isJavaScriptThread.  However, that's not a sufficient check; since the JS
thread isn't the main thread in worker context, it's also important to verify
that we're not on the main thread there.

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

Fixed, as below.

> > +    virtual bool isJavaScriptThread() const;
> 
> Probably unnecessary method, see above.

See above.

> > Index: WebCore/dom/ScriptExecutionContext.h
> 
> > +    class Database;
> > +    class DatabaseTaskSynchronizer;
> > +    class DatabaseThread;
> 
> @if ENABLE(DATABASE) ?

Fixed.

> > +    class Page;
> 
> If 'virtual Page page();' is not here, this can be removed as well.

Done.

> > +        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'?

Fixed.  Incidentally, I couldn't find a reference to a preferred line width
limit; what's the proper way to wrap very long comments or statements?  Do you
just let them go forever?

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

Excellent; thanks.  Fixed.

> > +#if ENABLE(DATABASE) // TODO(ericu): Likewise, anything about worker databases?
> >          if (m_frame->document()->hasOpenDatabases())
> 
> Ditto.

Fixed.

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

I saw that include guard in some other files, so I assumed it was omitted
accidentally here.  I see it unguarded in yet others--not sure what the right
thing is.  I'll put it back for now.

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

Done.

> >  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?

We do nothing here; our destructor completes, calls the RefPtr destructor, and
that takes care of it.  In the cases where we're posting the task to the JS
thread, we're guaranteed to complete it.  I've made the comment clearer, mainly
by deleting most of it ;'>.

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