[webkit-reviews] review denied: [Bug 25711] HTML5 Database becomes locked if a transaction is in progress when the page is refreshed. : [Attachment 31408] New patch.

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Ben Murdoch <benm at google.com>'s
request for review:
Bug 25711: HTML5 Database becomes locked if a transaction is in progress when
the page is refreshed.
https://bugs.webkit.org/show_bug.cgi?id=25711

Attachment 31408: New patch.
https://bugs.webkit.org/attachment.cgi?id=31408&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
It seems unfortunate that we have to add another database tracking mechanism.
There is already the DatabaseTracker and per Document set of databases. The
Document set seems to hold similar documents as the new per-db-thread set. 

> +	   Database* database = task->database();
> +	   if (database->opened()) 
> +	       recordDatabaseOpen(database);
> +	   else
> +	       recordDatabaseClosed(database);
> +

This seems unoptimal. The task loop is not really the right place. Could you
remove this code, protect the m_databases set with a mutex and record the
databases directly from Database ctor/dtor? Then this

> +    DatabaseSet::iterator end = m_databases.end();
> +    for (DatabaseSet::iterator it = m_databases.begin(); it != end; ++it)
> +	   (*it)->close();

could just check for closed() state before invoking close() (or it could be
done in close() itself)

Minor nits:

>      void close();
> +    bool opened() const { return m_open; }

Names should match. Rename m_open to m_opened.

> +    // If we sleep for more the 30 seconds while blocked on SQLITE_BUSY,
give up.
> +    static const int MAX_SQLITE_BUSY_WAIT_TIME = 30000;

WebKit style is to use camel case for constants too. This does not seem to need
to be in header, top of the .cpp would be better.

r- for now, I think at least the recordDatabaseOpen/recordDatabaseClosed in
task loop should be cleaned up.