[webkit-reviews] review denied: [Bug 58762] localStorage string values truncated at \x00 characters on browser restart : [Attachment 99775] 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 99775: patch
https://bugs.webkit.org/attachment.cgi?id=99775&action=review

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

looking pretty good

> Source/WebCore/ChangeLog:7
> +

Data migrations scare me and people like release engineers and QA...might be
worth a bit more of a note with something like DATA MIGRATION in all caps so it
can catch the right people's attention if they're skimming the ChangeLog.  :-)

> Source/WebCore/manual-tests/localstorage-value-truncation.html:18
> +	   document.write("</p><p>The expected value is: '123\x0056', the
expected length is: 6</p>");

You should also be able to write a big pass or fail for whether it seems
correct.

> Source/WebCore/storage/StorageAreaSync.cpp:270
> +    SQLiteStatement query(m_database, "SELECT key, value FROM ItemTable");

This will be very heavyweight.	There must be some way to examine just the
column.  If not, at least limit it to one row.

> Source/WebCore/storage/StorageAreaSync.cpp:273
> +    query.finalize();

The destructor finalizes, so maybe a bit cleaner to just wrap the block in {}'s
so query goes out of scope at this point.

> Source/WebCore/storage/StorageAreaSync.cpp:284
> +    size_t numOfCommands = sizeof(commands) / sizeof(commands[0]);

Does webkit have any macro for this?  I vaguely remember the answer being no,
but just double checking.  Alternately, putting a NULL as the last element and
looping until you see it might be slightly cleaner.

> Source/WebCore/storage/StorageAreaSync.cpp:286
> +    SQLiteTransaction transaction(m_database, false);

If you're feeling particularly heroic, creating an enum for read only and read
write and replacing the second param for SQLiteTransactions would be awesome. 
Prob in another patch.	cc me if you decide to do it.

> Source/WebCore/storage/StorageAreaSync.cpp:289
> +	   if (!m_database.executeCommand(commands[i])) {

This worries me some...  If this happens, will localStorage not be able to
proceed?  I kind of think in such a case we should just drop all the old data
(well, save it to some special table name so that it is at least possible to
recover it) and move on...

And there should be a NOTREACHED here since someone with it under debugger
would probably like to know if we fail.  :-)