|Summary:||IndexedDB: compare strings without decoding|
|Product:||WebKit||Reporter:||Joshua Bell <jsbell>|
|Component:||New Bugs||Assignee:||Joshua Bell <jsbell>|
|Severity:||Normal||CC:||dgrogan, hans, tony, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Joshua Bell 2011-09-21 11:59:18 PDT
IndexedDB: compare strings without decoding
Comment 2 Hans Wennborg 2011-09-21 13:29:40 PDT
Comment on attachment 108202 [details] Patch Looks good! Just a couple of nits. View in context: https://bugs.webkit.org/attachment.cgi?id=108202&action=review > ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=68554 The ChangeLog looks a little unusual. It's normally the title of the patch (one line), followed by the URL, "reviewed by...", and then a description. > Source/WebCore/storage/IDBLevelDBCoding.cpp:304 > + const char* q, const char* limitQ) I don't think there's any need for a line break here > Source/WebCore/storage/IDBLevelDBCoding.cpp:316 > + int x = memcmp(p, q, lmin * 2); feel free to do "if (int x = memcmp(...))". I like it because it saves a line and keeps x in the inner scope > Source/WebCore/storage/IDBLevelDBCoding.cpp:480 > + return compareEncodedStringsWithLength(p, limitA, q, limitB); i suppose we can now remove the declaration of s and t above? > Source/WebCore/storage/IDBLevelDBCoding.h:57 > + const char* q, const char* limitQ); no line break
Comment 5 Hans Wennborg 2011-09-21 14:19:52 PDT
Looks good to me. Tony, would you do the official reviewing?
Comment 6 Tony Chang 2011-09-21 14:36:00 PDT
Comment on attachment 108231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108231&action=review > Source/WebCore/storage/IDBLevelDBCoding.cpp:303 > +int compareEncodedStringsWithLength(const char* p, const char* limitP, const char* q, const char* limitQ) I find these 1 letter variable names hard to read, but it looks like that's how all the code around it is. It would be great to have a follow up patch that uses more descriptive variable names in this file. > Source/WebCore/storage/IDBLevelDBCoding.cpp:314 > + const unsigned lmin = lenP < lenQ ? lenP : lenQ; Nit: Should lmin be a size_t since that's what memcmp takes? Also, it might be more readable to explicitly static_cast<size_t>() after asserting that lenP and lenQ are >= 0.
Comment 8 WebKit Review Bot 2011-09-21 16:43:25 PDT
Comment on attachment 108239 [details] Patch Clearing flags on attachment: 108239 Committed r95682: <http://trac.webkit.org/changeset/95682>
Comment 9 WebKit Review Bot 2011-09-21 16:43:29 PDT
All reviewed patches have been landed. Closing bug.