Bug 68554 – IndexedDB: compare strings without decoding

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: compare strings without decoding

[Joshua Bell]

Created attachment 108202 [details]
Patch

[Hans Wennborg]

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

[Joshua Bell]

Created attachment 108226 [details]
Patch

[Joshua Bell]

Created attachment 108231 [details]
Patch

[Hans Wennborg]

Looks good to me.

Tony, would you do the official reviewing?

[Tony Chang]

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.

[Joshua Bell]

Created attachment 108239 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108239 [details]
Patch

Clearing flags on attachment: 108239

Committed r95682: <http://trac.webkit.org/changeset/95682>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.