[webkit-reviews] review denied: [Bug 22725] Make SQL database storage work in Workers : [Attachment 46968] Updated to latest webkit code; resolved merge conflicts.

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 22725: Make SQL database storage work in Workers
https://bugs.webkit.org/show_bug.cgi?id=22725

Attachment 46968: Updated to latest webkit code; resolved merge conflicts.
https://bugs.webkit.org/attachment.cgi?id=46968&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
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.

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

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

> +   
m_scriptExecutionContext->databaseThread()->unscheduleDatabaseTasks(this);
> +   
m_scriptExecutionContext->postTask(ContextRemoveOpenDatabaseTask::create(holder
));

Can use 'this' for both, for consistency. It's guaranteed protected.


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