[webkit-reviews] review denied: [Bug 27966] Race condition in the database code : [Attachment 34299] patch + test case

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 7 14:16:07 PDT 2009


Eric Seidel <eric at webkit.org> has denied Dumitru Daniliuc <dumi at chromium.org>'s
request for review:
Bug 27966: Race condition in the database code
https://bugs.webkit.org/show_bug.cgi?id=27966

Attachment 34299: patch + test case
https://bugs.webkit.org/attachment.cgi?id=34299&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Thank you!  This is much much more reviewable.

Why do we have one transaction coordinator per thread instead of per database? 
if it was per database, then we would not need the hash to hold these queues,
we could just hold them on each datbase?  Do transations span across datbases?

I find the test case very hard to read using anonymous functions.  If I were
writing that I would have given the functions nice names, even if they were
only defined locally.

It seems teh select/insert tests differ only in "db1" vs. "db2".  Probably
should be a single funtion which takes a name argument, no?

Likewise the openDatbasse call could be a function to make this more readable. 
In general the test has too much copy/paste code. :(

Since no one else has stepped up yet, I think you're stuck with me! ;)

In general this looks great.  The layout test needs some cleanup.  And I need
better understanding (ideally with explanation in teh ChagneLog) as to why it's
designed this way with teh DatbaseThread holding the transaction coordinator
instead of the datbase.  Also confusing that m_database is actually a
datbaseThread it seems.

Why is a securityORiginCopy needeD?
 62	return database->securityOriginCopy()->databaseIdentifier() + ":" +
database->stringIdentifier();

In general the ChangeLog could be more detailed.

anyway, I look forward to the next round!  This seems like a good fix.	Just
needs to be presented more clearly.


More information about the webkit-reviews mailing list