Bug 68554

Summary: IndexedDB: compare strings without decoding
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: New BugsAssignee: Joshua Bell <jsbell>
Severity: Normal CC: dgrogan, hans, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Joshua Bell 2011-09-21 11:59:18 PDT
IndexedDB: compare strings without decoding
Comment 1 Joshua Bell 2011-09-21 12:03:17 PDT
Created attachment 108202 [details]
Comment 2 Hans Wennborg 2011-09-21 13:29:40 PDT
Comment on attachment 108202 [details]

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 3 Joshua Bell 2011-09-21 13:44:23 PDT
Created attachment 108226 [details]
Comment 4 Joshua Bell 2011-09-21 14:08:40 PDT
Created attachment 108231 [details]
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]

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 7 Joshua Bell 2011-09-21 14:51:03 PDT
Created attachment 108239 [details]
Comment 8 WebKit Review Bot 2011-09-21 16:43:25 PDT
Comment on attachment 108239 [details]

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.