[webkit-reviews] review requested: [Bug 22725] Make SQL database storage work in Workers : [Attachment 47061] Removed DOMWindow.cpp change, fixed the last(?) few comments.

[bugzilla-daemon at webkit.org]

Eric U. <ericu at chromium.org> has asked	for review:
Bug 22725: Make SQL database storage work in Workers
https://bugs.webkit.org/show_bug.cgi?id=22725

Attachment 47061: Removed DOMWindow.cpp change, fixed the last(?) few comments.
https://bugs.webkit.org/attachment.cgi?id=47061&action=review

------- Additional Comments from Eric U. <ericu at chromium.org>
> --- 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.