Bug 84894 - IndexedDB: Replace numeric constants with strings
Summary: IndexedDB: Replace numeric constants with strings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords: WebExposed
Depends on: 85559
Blocks: 85315
  Show dependency treegraph
 
Reported: 2012-04-25 15:03 PDT by Alec Flett
Modified: 2012-06-08 14:58 PDT (History)
7 users (show)

See Also:


Attachments
Patch (195.53 KB, patch)
2012-04-27 15:42 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.21 MB, application/zip)
2012-04-28 02:59 PDT, WebKit Review Bot
no flags Details
Patch (272.70 KB, patch)
2012-05-02 16:08 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (272.73 KB, patch)
2012-05-02 16:26 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (5.69 MB, application/zip)
2012-05-02 18:08 PDT, WebKit Review Bot
no flags Details
Patch (272.86 KB, patch)
2012-05-03 11:47 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (6.13 MB, application/zip)
2012-05-03 12:49 PDT, WebKit Review Bot
no flags Details
Patch (273.63 KB, patch)
2012-05-03 13:02 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.17 MB, application/zip)
2012-05-03 14:01 PDT, WebKit Review Bot
no flags Details
Patch (274.24 KB, patch)
2012-05-03 15:10 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (275.79 KB, patch)
2012-05-07 09:19 PDT, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-04-25 15:03:02 PDT
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
Comment 1 Alec Flett 2012-04-25 15:13:00 PDT
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
----
Comment 2 Alec Flett 2012-04-27 15:42:40 PDT
Created attachment 139295 [details]
Patch
Comment 3 Alec Flett 2012-04-27 15:49:26 PDT
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.
Comment 4 WebKit Review Bot 2012-04-27 16:01:12 PDT
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.
Comment 5 Joshua Bell 2012-04-27 16:33:13 PDT
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.
Comment 6 Joshua Bell 2012-04-27 16:36:38 PDT
(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.
Comment 7 WebKit Review Bot 2012-04-28 02:59:05 PDT
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
Comment 8 WebKit Review Bot 2012-04-28 02:59:13 PDT
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
Comment 9 Alec Flett 2012-05-02 16:08:16 PDT
Created attachment 139904 [details]
Patch
Comment 10 Alec Flett 2012-05-02 16:11:49 PDT
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.
Comment 11 Alec Flett 2012-05-02 16:26:23 PDT
Created attachment 139908 [details]
Patch
Comment 12 Alec Flett 2012-05-02 16:26:50 PDT
new patch - just fixes formatting inconsistency in tests
Comment 13 Joshua Bell 2012-05-02 17:15:06 PDT
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?
Comment 14 WebKit Review Bot 2012-05-02 18:08:40 PDT
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
Comment 15 WebKit Review Bot 2012-05-02 18:08:48 PDT
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
Comment 16 Alec Flett 2012-05-03 11:47:11 PDT
Created attachment 140057 [details]
Patch
Comment 17 Alec Flett 2012-05-03 11:48:18 PDT
ok, all comments addressed, and found out how to do to static constant AtomicStrings correctly.
Comment 18 WebKit Review Bot 2012-05-03 12:49:46 PDT
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
Comment 19 WebKit Review Bot 2012-05-03 12:49:53 PDT
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
Comment 20 Alec Flett 2012-05-03 13:02:07 PDT
Created attachment 140073 [details]
Patch
Comment 21 Alec Flett 2012-05-03 13:03:31 PDT
x
Comment 22 Alec Flett 2012-05-03 13:03:57 PDT
Comment on attachment 140073 [details]
Patch

tony@ - r? cq?
Comment 23 WebKit Review Bot 2012-05-03 14:01:48 PDT
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
Comment 24 WebKit Review Bot 2012-05-03 14:01:55 PDT
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
Comment 25 Tony Chang 2012-05-03 14:06:19 PDT
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).
Comment 26 Alec Flett 2012-05-03 15:10:01 PDT
Created attachment 140105 [details]
Patch
Comment 27 Alec Flett 2012-05-03 15:11:05 PDT
Comment on attachment 140105 [details]
Patch

jsbell@ - this didn't land cleanly before (just due to your 2^53 landing today) - cq?
Comment 28 Alec Flett 2012-05-03 15:19:41 PDT
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
Comment 29 WebKit Review Bot 2012-05-03 16:40:51 PDT
Comment on attachment 140105 [details]
Patch

Clearing flags on attachment: 140105

Committed r116040: <http://trac.webkit.org/changeset/116040>
Comment 30 WebKit Review Bot 2012-05-03 16:41:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Alec Flett 2012-05-04 09:47:43 PDT
reopening to try relanding
Comment 32 Alec Flett 2012-05-07 09:19:33 PDT
Created attachment 140537 [details]
Patch
Comment 33 Alec Flett 2012-05-07 09:22:39 PDT
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.
Comment 34 WebKit Review Bot 2012-05-07 12:11:26 PDT
Comment on attachment 140537 [details]
Patch

Clearing flags on attachment: 140537

Committed r116337: <http://trac.webkit.org/changeset/116337>
Comment 35 WebKit Review Bot 2012-05-07 12:12:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Pavel Podivilov 2012-06-04 07:54:55 PDT
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.
Comment 37 Joshua Bell 2012-06-06 10:20:35 PDT
(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.