[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


Eric U. <ericu at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46968|0                           |1
        is obsolete|                            |
  Attachment #47061|                            |review?, commit-queue?
               Flag|                            |




--- Comment #25 from Eric U. <ericu at chromium.org>  2010-01-20 13:33:10 PST ---
Created an attachment (id=47061)
 --> (https://bugs.webkit.org/attachment.cgi?id=47061)
Removed DOMWindow.cpp change, fixed the last(?) few comments.

> --- Comment #24 from Dmitry Titov <dimich at chromium.org>  2010-01-20 12:40:39 PST ---
> (From update of attachment 46968)
> Just a few more. Lets try to get to a clean r+ so we can flip cq+ on it.
>
>> Index: WebCore/page/DOMWindow.cpp
>>      Document* document = m_frame->document();
>> -    if (!document->securityOrigin()->canAccessDatabase())
>> +    if (!document->securityOrigin()->canAccessDatabase()) {
>> +        ec = SECURITY_ERR;
>>          return 0;
>> +    }
>
> This is a new addition to the previously reviewed patch. It will take less time
> if we keep the scope of the change rather then expand it. In this particular
> case, it's a change of already shipped behavior which may have compatibility
> issues. Basically, if some website will happen to be broken, this change may
> have to be reverted. Adding Database to Workers is one thing (no compatibility
> issues since the stuff is new), changing behavior of Database on the regular
> pages is a different matter, and should have a separate bug. Lets move this
> into separate bug.

Gotcha.  I've logged the bug and reverted this part of the patch.  I'll
resubmit
that part once this is in.

>> Index: WebCore/storage/Database.cpp
>>
>>  Database::~Database()
>>  {
>> +    // The reference to the ScriptExecutionContext needs to be cleared on the JavaScript thread.  If we're on that thread already, we can just let the RefPtr's destruction do the dereffing.
>> +    if (!m_scriptExecutionContext->isContextThread()) {
>> +        m_scriptExecutionContext->postTask(DerefContextTask::create());
>> +        m_scriptExecutionContext.release().releaseRef();
>
> This is something we'll want to cleanup, to possibly avoid having a destructor
> that can run on either thread, since Database may have ownership of
> non-thread-safe objects... But it's outside of scope of this change since this
> one simply refactors the existing code.

Yeah, it's kind of scary in there right now, thread-wise.

>>  void Database::close()
>>  {
>>      if (!m_opened)
>>          return;
>>
>> +    // Must ref() before calling databaseThread()->recordDatabaseClosed().
>> +    RefPtr<Database> holder = this;
>
> The pattern used often in WebKit is to create a RefPtr named 'protect' at the
> very beginning of the function and then not use it in the code (since 'this' is
> guaranteed to be valid), like this:
> RefPtr<Database> protect = this;

Changed the name to protect and moved it up to the top.  I debated putting it
after the easy-out "if (!m_opened) return;", but that's a rare failure path, so
I
assumed we wouldn't put optimization over consistency.

>> +    m_scriptExecutionContext->databaseThread()->unscheduleDatabaseTasks(this);
>> +    m_scriptExecutionContext->postTask(ContextRemoveOpenDatabaseTask::create(holder));
>
> Can use 'this' for both, for consistency. It's guaranteed protected.

Done.

>> Index: LayoutTests/ChangeLog
>
> The Layouttest change should go with the DOMWindow change, in a different bug.
>
>> +        No bug.  This is part of the checkin for
>> +        https://bugs.webkit.org/show_bug.cgi?id=22725
>
> Normally, all the ChangeLog files that are part of the same checkin, refer to
> the same bug number. So the first line here would not be needed.

Gotcha.  Just as well that the test change will be in a separate checkin, then.

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