Bug 84894 – IndexedDB: Replace numeric constants with strings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alec Flett]

From the Chromium bug: http://crbug.com/115669

Proposed on the public-webapps list, with support from Opera and Mozilla:

http://lists.w3.org/Archives/Public/public-webapps/2012JanMar/0814.html

Ignoring the *Sync APIs, this would involve changing:

Methods:
IDBDatabase.transaction() - mode
IDBObjectStore.openCursor() - direction
IDBIndex.openCursor() - direction
IDBIndex.openKeyCursor() - direction

Attributes (read-only):
IDBRequest.readyState
IDBCursor.direction
IDBTransaction.mode

Editor's draft of spec has been updated:
http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html

[Alec Flett]

The approach I'll take is similar to jsbell@'s comments in the other bug:

----
As a short term fix for this:

(1) Change the IDL to accept string, not short
(2) Accept both strings (e.g. "next") and numbers coerced to strings (e.g. "0")
(3) Issue a console error if the "0" form is used, to try and wean users off of using the constant
(4) Don't drop the constant until we drop the prefix
----

[Alec Flett]

Created attachment 139295 [details]
Patch

[Alec Flett]

Josh/David - 
this patch isn't ready for full review for checkin, but I wanted to clear a few aspects of the approach.

In particular:
* I'm not changing the *Backend APIs - they will remain enum-based. This is both easier, more efficient and I think safer: Easier because we don't have to do a split chrome/webkit landing, more efficient because we don't have to pass strings through IPC (though the effect is minor because these APIs are not frequently called in a tight loop) and safer because it frees up the backend from having to worry if it's ever passed a bad mode / direction / etc.

* I'm using AtomicString here for the values and I'm trying to figure out if theres a webkit-approved way to do this (neither DEFINE_GLOBAL nor DEFINE_STATIC_LOCAL seem to generate code that allows for static member variables. The real issue is that we need to get at these strings from more than one class. Open to suggestions.

* Any other issues (like the one I learned today from Josh about how 'null' gets passed through as '"null"' for APIs that expect strings, according to WebIDL. Yay!) - you'll also notice that I had to muck with Optional=DefaultIsNullString to get the overloads working in transaction(). I've tried about 20 varations in IDL, this seemed to be the only magic Web[Kit]IDL incantation that worked.

I'll be going through this with a fine-grained comb to address FIXMEs, so don't worry too much about line-by-line issues yet.

[WebKit Review Bot]

Attachment 139295 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/stor..." exit_code: 1
Source/WebCore/ChangeLog:13:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 101 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joshua Bell]

Comment on attachment 139295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139295&action=review

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:52
> +const AtomicString IDBCursor::s_directionUndefined("undefined");

Will this appear in the final patch?

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:220
> +unsigned short IDBCursor::getDirection(const String& directionString, ExceptionCode& ec)

A name like directionToString / stringToDirection might be more evocative.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:258
> +        ec = IDBDatabaseException::NON_TRANSIENT_ERR;

ASSERT_NOT_REACHED here

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:259
> +        return IDBCursor::s_directionUndefined;

... and okay to return s_directionNext rather than making up a value.

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:187
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("Numeric transaction modes are deprecated in WebKit. They will be removed from the engine in the near future."));

Based on feedback I've seen about error messages, it might be worth including e.g. "IDBDatabase.transaction" in the string so callers know specifically what call the error is coming from.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:62
> +    if (directionString != IDBCursor::s_directionNext

This seems redundant with the below call to getDirection().

> Source/WebCore/Modules/indexeddb/IDBIndex.idl:37
> +        [CallWith=ScriptExecutionContext] IDBRequest openCursor(in [Optional] IDBKeyRange range, in [Optional] DOMString direction)

As discussed in person, I think we should delete the versions from the IDL (and implementation) that accept shorts and rely on the generated binding code to convert 0 (number) -> "0" (string) and allow "0" string as an alias for "next" (with warnings). This will make the IDL overloading unambiguous, and should simplify the code dramatically.

> Source/WebCore/Modules/indexeddb/IDBRequest.h:62
> +        PENDING = 1,

Just a rename to match the spec, right? (I wholeheartedly approve.)

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:218
> +unsigned short IDBTransaction::getMode(const String& modeString, ExceptionCode& ec)

Ditto on the xxxToYyyy renaming suggestion. I think there's precedent in IDBLevelDB. getXXXX should be reserved for accessors.

> Source/WebCore/Modules/indexeddb/IDBTransaction.idl:34
> +        // Legacy numeric modes

Add a FIXME/bug for removing these eventually.

> LayoutTests/storage/indexeddb/cursor-advance-expected.txt:1
> +CONSOLE MESSAGE: Numeric direction values are deprecated in WebKit. They will be removed from the engine in the near future.

Just in case you weren't planning on this already: please update all the tests (wheeee) to the new style, and have a new test for legacy calls, which should be the only test where these warnings show up in -expected.txt.

[Joshua Bell]

(In reply to comment #3)
> In particular:
> * I'm not changing the *Backend APIs - they will remain enum-based. 

Agreed.

> * I'm using AtomicString here for the values and I'm trying to figure out if theres a webkit-approved way to do this

No clue. :(

>  I've tried about 20 varations in IDL, this seemed to be the only magic Web[Kit]IDL incantation that worked.

haraken@ is a good person to ping with IDL questions. DefaultIsNullString seems like the right approach, though. 

> I'll be going through this with a fine-grained comb to address FIXMEs, so don't worry too much about line-by-line issues yet.

[WebKit Review Bot]

Comment on attachment 139295 [details]
Patch

Attachment 139295 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12550535

New failing tests:
http/tests/inspector/indexeddb/database-data.html

[WebKit Review Bot]

Created attachment 139347 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alec Flett]

Created attachment 139904 [details]
Patch

[Alec Flett]

jsbell@ - finally wrapped this one up - all your earlier comments should be addressed. Tests have been mass-updated (yay, perl has a reason to exist..) and a new test for the legacy values has been added.

[Alec Flett]

Created attachment 139908 [details]
Patch

[Alec Flett]

new patch - just fixes formatting inconsistency in tests

[Joshua Bell]

Comment on attachment 139908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139908&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=84894

Add a brief mention to the ChangeLog that this is motivated by a spec change (i.e. it's not a bug fix, refactor, optimization, just to mess with webdevs, etc). E.g. move the comment from LayoutTests/ChangeLog here.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:221
> +    if (directionString.isNull() || directionString == IDBCursor::s_directionNext)

When can the null case occur? If it's coming in via binding to a method (i.e. if the argument is not specified), would it be cleaner to test for that in the method itself?

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:233
> +    ec = IDBDatabaseException::NON_TRANSIENT_ERR;

FYI, the spec was updated and now indicates this should be "TypeError" (so you can use TYPE_ERR here I believe)

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:187
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("Numeric transaction modes are deprecated in IDBDatabase.transaction. The strings 'readonly' or 'readwrite' are preferred."));

Nitpicky wording suggestion: Numeric transaction modes are deprecated in IDBDatabase.transaction. Use "readonly" or "readwrite".

(Be prescriptive, and use double-quotes rather than apostrophes.)

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:133
>          // FIXME: May need to change when specced: http://www.w3.org/Bugs/Public/show_bug.cgi?id=11406

Spec was updated; this is is now "TypeError" (so can use TYPE_ERR)

Is this whole |if| block redundant with the stringToDirection() check below?

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:303
> +

Remove one of these blank lines.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.idl:56
> +        // https://bugs.webkit.org/show_bug.cgi?id=85326 is fixed.

Can put these comments on one line

> Source/WebCore/Modules/indexeddb/IDBRequest.idl:-47
> -        const unsigned short LOADING = 1;

I'd forgotten about this enumeration. I'm slightly worried that some web apps will be relying on these. The apps would have to update to: if (request.readyState === 2 || request.readyState === 'done') ... - make sure we call this case out in any announcements.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:54
> +const AtomicString IDBTransaction::s_modeReadOnlyLegacy("0");

When https://bugs.webkit.org/show_bug.cgi?id=85326 is fixed there will be similar s_XXXLegacy("0") strings for the direction "enum", correct?

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:222
> +        || modeString == IDBTransaction::s_modeReadOnly)

No need for a line break here.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:234
> +

Remove blank line.

> LayoutTests/ChangeLog:8
> +        Update IDBObjectStore.openCursor, IDBIndex.openCursor, IDBIndex.openKeyCursor,

More important to have this comment in the WebCore/ChangeLog. Could just move it there.

> LayoutTests/storage/indexeddb/cursor-advance-expected.txt:-36
> -request = store.openCursor(null, null)

This still works, right? Just cleanup?

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:206
> +                    // Joe (weight 150 - skipping 180, 180, 180)

Just an incorrect comment?

[WebKit Review Bot]

Comment on attachment 139908 [details]
Patch

Attachment 139908 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12609093

New failing tests:
http/tests/inspector/indexeddb/database-data.html
storage/indexeddb/key-generator.html

[WebKit Review Bot]

Created attachment 139929 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alec Flett]

Created attachment 140057 [details]
Patch

[Alec Flett]

ok, all comments addressed, and found out how to do to static constant AtomicStrings correctly.

[WebKit Review Bot]

Comment on attachment 140057 [details]
Patch

Attachment 140057 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12621140

New failing tests:
http/tests/inspector/indexeddb/database-data.html

[WebKit Review Bot]

Created attachment 140070 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alec Flett]

Created attachment 140073 [details]
Patch

[Alec Flett]

x

[Alec Flett]

Comment on attachment 140073 [details]
Patch

tony@ - r? cq?

[WebKit Review Bot]

Comment on attachment 140073 [details]
Patch

Attachment 140073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12610348

New failing tests:
storage/indexeddb/key-generator.html

[WebKit Review Bot]

Created attachment 140086 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Chang]

Comment on attachment 140073 [details]
Patch

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).

[Alec Flett]

Created attachment 140105 [details]
Patch

[Alec Flett]

Comment on attachment 140105 [details]
Patch

jsbell@ - this didn't land cleanly before (just due to your 2^53 landing today) - cq?

[Alec Flett]

Comment on attachment 140105 [details]
Patch

sorry tony@ can you try that (r?) again? I got a conflict trying to land the last one - I fixed and it should land cleanly now

[WebKit Review Bot]

Comment on attachment 140105 [details]
Patch

Clearing flags on attachment: 140105

Committed r116040: <http://trac.webkit.org/changeset/116040>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alec Flett]

reopening to try relanding

[Alec Flett]

Created attachment 140537 [details]
Patch

[Alec Flett]

Comment on attachment 140537 [details]
Patch

tony@: r? cq? 

third time's the charm. Last patch broke some chromium tests - this is virtually idential to the last version which you reviewed, with a minor update to one test expectation. The Chromium tests have been updated but disabled until this patch lands, then I'll re-enable them when webkit rolls.

[WebKit Review Bot]

Comment on attachment 140537 [details]
Patch

Clearing flags on attachment: 140537

Committed r116337: <http://trac.webkit.org/changeset/116337>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Podivilov]

Comment on attachment 140537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140537&action=review

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:301
> +    return openCursor(context, key, direction, ec);

Should be keyRange, not key. This call leads to infinite recursion.

[Joshua Bell]

(In reply to comment #36)
> (From update of attachment 140537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140537&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:301
> > +    return openCursor(context, key, direction, ec);
> 
> Should be keyRange, not key. This call leads to infinite recursion.

Thanks, I've filed https://bugs.webkit.org/show_bug.cgi?id=88431 and uploaded a patch.