RESOLVED FIXED 96575
IndexedDB: Print console warning about setVersion
https://bugs.webkit.org/show_bug.cgi?id=96575
Summary IndexedDB: Print console warning about setVersion
David Grogan
Reported 2012-09-12 16:28:25 PDT
IndexedDB: Print console warning about setVersion
Attachments
Patch (124.90 KB, patch)
2012-09-12 16:34 PDT, David Grogan
no flags
Patch (123.81 KB, patch)
2012-09-13 14:02 PDT, David Grogan
no flags
Patch (123.76 KB, patch)
2012-09-13 15:06 PDT, David Grogan
no flags
Patch (127.23 KB, patch)
2012-09-13 16:31 PDT, David Grogan
no flags
Patch (127.22 KB, patch)
2012-09-13 18:03 PDT, David Grogan
no flags
Patch (123.75 KB, patch)
2012-09-13 19:13 PDT, David Grogan
no flags
ToT (122.88 KB, patch)
2012-09-19 18:53 PDT, David Grogan
no flags
Patch for landing (122.96 KB, patch)
2012-09-19 20:53 PDT, David Grogan
no flags
David Grogan
Comment 1 2012-09-12 16:34:51 PDT
David Grogan
Comment 2 2012-09-12 16:37:54 PDT
jsbell@, could you look at this? This is copy/pasted from alecflett's cursor warnings.
Joshua Bell
Comment 3 2012-09-12 17:00:30 PDT
Comment on attachment 163732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163732&action=review Nits, otherwise lgtm. > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:202 > + DEFINE_STATIC_LOCAL(String, consoleMessage, ("setVersion is deprecated and will be removed in a future version of chrome. Use the \"upgradeneeded\" API instead.")); Looks like the ASCIILiteral() constructor should be used now. http://trac.webkit.org/changeset/126968 http://trac.webkit.org/wiki/EfficientStrings Also, just wordsmithing, how about: The setVersion() method is non-standard and will be removed. Use the "upgradeneeded" event instead. (Most importantly, drop reference to "Chrome" in the WebKit code base.)
WebKit Review Bot
Comment 4 2012-09-12 18:26:01 PDT
Comment on attachment 163732 [details] Patch Attachment 163732 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13839300 New failing tests: http/tests/inspector/indexeddb/database-data.html http/tests/inspector/indexeddb/database-structure.html http/tests/inspector/indexeddb/resources-panel.html
David Grogan
Comment 5 2012-09-13 13:51:50 PDT
Comment on attachment 163732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163732&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:202 >> + DEFINE_STATIC_LOCAL(String, consoleMessage, ("setVersion is deprecated and will be removed in a future version of chrome. Use the \"upgradeneeded\" API instead.")); > > Looks like the ASCIILiteral() constructor should be used now. http://trac.webkit.org/changeset/126968 http://trac.webkit.org/wiki/EfficientStrings > > Also, just wordsmithing, how about: > > The setVersion() method is non-standard and will be removed. Use the "upgradeneeded" event instead. > > (Most importantly, drop reference to "Chrome" in the WebKit code base.) SG. Done.
David Grogan
Comment 6 2012-09-13 14:02:03 PDT
WebKit Review Bot
Comment 7 2012-09-13 14:50:57 PDT
Comment on attachment 163959 [details] Patch Attachment 163959 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13849313 New failing tests: http/tests/inspector/indexeddb/database-data.html http/tests/inspector/indexeddb/database-structure.html http/tests/inspector/indexeddb/resources-panel.html
David Grogan
Comment 8 2012-09-13 15:06:57 PDT
David Grogan
Comment 9 2012-09-13 16:31:04 PDT
David Grogan
Comment 10 2012-09-13 18:03:58 PDT
David Grogan
Comment 11 2012-09-13 18:04:36 PDT
Tony, could you review this?
Tony Chang
Comment 12 2012-09-13 18:22:13 PDT
Ojan says we should only spam the console once per page, rather than once per call. I think blob.webkitSlice does this. Also, abarth suggests starting to collect histogram usage data about how many people are using the old API. blob.webkitSlice does this, but it counts total calls, which isn't that useful. Look at how mutation observers do the counting per document.
Tony Chang
Comment 13 2012-09-13 18:22:30 PDT
Of course the histogram can be a separate change and doesn't need to block this change.
David Grogan
Comment 14 2012-09-13 19:13:31 PDT
David Grogan
Comment 15 2012-09-13 19:23:11 PDT
(In reply to comment #12) > Ojan says we should only spam the console once per page, rather than once per call. I think blob.webkitSlice does this. That's a good idea. I don't think webkitSlice does that though, after looking at https://bugs.webkit.org/attachment.cgi?id=164019&action=review. Looking through the rest of webkit, I couldn't find any console warnings that were limited to once per page. The latest patch warns once per object, which should be pretty close to once per page. The benefit of eliminating the remaining extra messages doesn't seem worth adding thread-safe code that deals with pages that open databases from a worker. > Also, abarth suggests starting to collect histogram usage data about how many people are using the old API. blob.webkitSlice does this, but it counts total calls, which isn't that useful. Look at how mutation observers do the counting per document. SG. Thanks for the pointer.
Tony Chang
Comment 16 2012-09-14 10:07:53 PDT
Comment on attachment 164027 [details] Patch Once per object seems OK.
WebKit Review Bot
Comment 17 2012-09-14 10:27:52 PDT
Comment on attachment 164027 [details] Patch Clearing flags on attachment: 164027 Committed r128627: <http://trac.webkit.org/changeset/128627>
WebKit Review Bot
Comment 18 2012-09-14 10:27:57 PDT
All reviewed patches have been landed. Closing bug.
James Robinson
Comment 19 2012-09-14 12:41:03 PDT
This breaks several chromium browser tests. Example failure diff: Value of: actual_text Actual: "Test for crbug.com/108071\n\nOn success, you will see a series of \"PASS\" messages .... TEST COMPLETE" Expected: expected_text Which is: "CONSOLE MESSAGE: The setVersion() method is non-standard and will be removed. Use the \"upgradeneeded\" event instead.\nTest for crbug.com/108071\n\nOn success, you will see a series of \"PASS\" messages .... TEST COMPLETE" Unfortunately this blocks rolls, so I think I'll have to revert. Please fix the chromium-side tests to use the correct API before relanding.
James Robinson
Comment 20 2012-09-14 12:49:23 PDT
Reverted r128627 for reason: Breaks indexeddb content_browsertests in chromium Committed r128642: <http://trac.webkit.org/changeset/128642>
David Grogan
Comment 21 2012-09-19 18:53:27 PDT
David Grogan
Comment 22 2012-09-19 20:53:52 PDT
Created attachment 164832 [details] Patch for landing
David Grogan
Comment 23 2012-09-19 20:57:53 PDT
chromium r157696 disabled all the content_browsertests that this should break, so it'll hopefully stick this time.
WebKit Review Bot
Comment 24 2012-09-19 21:49:25 PDT
Comment on attachment 164832 [details] Patch for landing Clearing flags on attachment: 164832 Committed r129090: <http://trac.webkit.org/changeset/129090>
WebKit Review Bot
Comment 25 2012-09-19 21:49:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.