IndexedDB: compare strings without decoding
Created attachment 108202 [details] Patch
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
Created attachment 108226 [details] Patch
Created attachment 108231 [details] Patch
Looks good to me. Tony, would you do the official reviewing?
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.
Created attachment 108239 [details] Patch
Comment on attachment 108239 [details] Patch Clearing flags on attachment: 108239 Committed r95682: <http://trac.webkit.org/changeset/95682>
All reviewed patches have been landed. Closing bug.