[webkit-reviews] review denied: [Bug 58762] localStorage string values truncated at \x00 characters on browser restart : [Attachment 100391] patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at webkit.org> has denied Jonathan Dong
<jonathan.dong at torchmobile.com.cn>'s request for review:
Bug 58762: localStorage string values truncated at \x00 characters on browser
restart
https://bugs.webkit.org/show_bug.cgi?id=58762

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

------- Additional Comments from Jeremy Orlow <jorlow at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100391&action=review


Should be r+able if this stuff is addressed.  Thanks!

> Source/WebCore/storage/StorageAreaSync.cpp:276
> +	   SQLiteStatement query(m_database, "SELECT value FROM ItemTable where
rowid = 1");

Hm...will this always work?  What if someone inserts something, inserts another
something, then deletes that first something.  Won't then there be items in it
but no rowid = 1?  I think you just want s/where rowid = 1/limit 1/.  Right?

> Source/WebCore/storage/StorageAreaSync.cpp:277
> +	   if (query.isColumnDeclaredAsBlob(0))

Wait, will this work if there was no data in the table?

> Source/WebCore/storage/StorageAreaSync.cpp:294
> +    while (commands[i]) {

Probably a bit cleaner to do for (size_t i = 0; commands[i]; ++i)

> Source/WebCore/storage/StorageAreaSync.cpp:299
> +	       // save the ItemTable for future restore by renaming it.

Mention that this will essentially delete the current database, but that's
better than continually hitting this case and never being able to use the local
storage.  Mention that if this is ever hit, it's definitely a bug, and that's
why we assert not reached below.

Actually, you should probably move the assert not reached to just above this
line.