| Summary: | IndexedDB: Replace numeric constants with strings | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alec Flett <alecflett> | ||||||||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Alec Flett <alecflett> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, dgrogan, jsbell, ojan, tony, webkit.review.bot | ||||||||||||||||||||||||
| Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||
| Bug Depends on: | 85559 | ||||||||||||||||||||||||||
| Bug Blocks: | 85315 | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Alec Flett
2012-04-25 15:03:02 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 ---- Created attachment 139295 [details]
Patch
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. 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 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. (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 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 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
Created attachment 139904 [details]
Patch
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. Created attachment 139908 [details]
Patch
new patch - just fixes formatting inconsistency in tests 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 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 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
Created attachment 140057 [details]
Patch
ok, all comments addressed, and found out how to do to static constant AtomicStrings correctly. 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 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
Created attachment 140073 [details]
Patch
x Comment on attachment 140073 [details]
Patch
tony@ - r? cq?
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 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 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). Created attachment 140105 [details]
Patch
Comment on attachment 140105 [details]
Patch
jsbell@ - this didn't land cleanly before (just due to your 2^53 landing today) - cq?
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 on attachment 140105 [details] Patch Clearing flags on attachment: 140105 Committed r116040: <http://trac.webkit.org/changeset/116040> All reviewed patches have been landed. Closing bug. reopening to try relanding Created attachment 140537 [details]
Patch
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 on attachment 140537 [details] Patch Clearing flags on attachment: 140537 Committed r116337: <http://trac.webkit.org/changeset/116337> All reviewed patches have been landed. Closing bug. 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. (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. |