[webkit-reviews] review granted: [Bug 84894] IndexedDB: Replace numeric constants with strings : [Attachment 140073] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Alec Flett
<alecflett at chromium.org>'s request for review:
Bug 84894: IndexedDB: Replace numeric constants with strings
https://bugs.webkit.org/show_bug.cgi?id=84894

Attachment 140073: Patch
https://bugs.webkit.org/attachment.cgi?id=140073&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140073&action=review


> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:147
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("Numeric direction values
are deprecated in IDBIndex.openKeyCursor. Use \"next\", \"nextunique\",
\"prev\", or \"prevunique\"."));

Nit: Maybe build a string using the static methods on IDBCursor here?  That
way, if we rename a function, you'll get a compile error here, although I guess
if you add a new direction, it'll be hard to remember to update this message.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:278
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("Numeric direction values
are deprecated in IDBObjectStore.openCursor. Use\"next\", \"nextunique\",
\"prev\", or \"prevunique\"."));

Ditto.

> Source/WebCore/Modules/indexeddb/IDBTransaction.h:60
> +    // FIXME: is this the right way to do static class members? Or is
> +    // this verboten? Seems like using DEFINE_GLOBAL is almost what I
need..almost..

I think you can remove the FIXME now.

> LayoutTests/ChangeLog:8
> +	   New test for legacy constants.

Since the list of changed files is long, you may want to highlight the new file
here.  Maybe also add a couple sentences about the changes to expected results
(e.g., replace enumerated value names with strings).