Summary: | IndexedDB: Print console warning about setVersion | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||||||||||||||
Component: | New Bugs | Assignee: | David Grogan <dgrogan> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | alecflett, dglazkov, jamesr, jsbell, tony, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 96548 | ||||||||||||||||||||
Attachments: |
|
Description
David Grogan
2012-09-12 16:28:25 PDT
Created attachment 163732 [details]
Patch
jsbell@, could you look at this? This is copy/pasted from alecflett's cursor warnings. 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.) 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 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. Created attachment 163959 [details]
Patch
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 Created attachment 163975 [details]
Patch
Created attachment 163998 [details]
Patch
Created attachment 164018 [details]
Patch
Tony, could you review this? 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. Of course the histogram can be a separate change and doesn't need to block this change. Created attachment 164027 [details]
Patch
(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. Comment on attachment 164027 [details]
Patch
Once per object seems OK.
Comment on attachment 164027 [details] Patch Clearing flags on attachment: 164027 Committed r128627: <http://trac.webkit.org/changeset/128627> All reviewed patches have been landed. Closing bug. 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. Reverted r128627 for reason: Breaks indexeddb content_browsertests in chromium Committed r128642: <http://trac.webkit.org/changeset/128642> Created attachment 164820 [details]
ToT
Created attachment 164832 [details]
Patch for landing
chromium r157696 disabled all the content_browsertests that this should break, so it'll hopefully stick this time. Comment on attachment 164832 [details] Patch for landing Clearing flags on attachment: 164832 Committed r129090: <http://trac.webkit.org/changeset/129090> All reviewed patches have been landed. Closing bug. |