Bug 96575

Summary: IndexedDB: Print console warning about setVersion
Product: WebKit Reporter: David Grogan <dgrogan>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
ToT
none
Patch for landing none

Description David Grogan 2012-09-12 16:28:25 PDT
IndexedDB: Print console warning about setVersion
Comment 1 David Grogan 2012-09-12 16:34:51 PDT
Created attachment 163732 [details]
Patch
Comment 2 David Grogan 2012-09-12 16:37:54 PDT
jsbell@, could you look at this?

This is copy/pasted from alecflett's cursor warnings.
Comment 3 Joshua Bell 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.)
Comment 4 WebKit Review Bot 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
Comment 5 David Grogan 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.
Comment 6 David Grogan 2012-09-13 14:02:03 PDT
Created attachment 163959 [details]
Patch
Comment 7 WebKit Review Bot 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
Comment 8 David Grogan 2012-09-13 15:06:57 PDT
Created attachment 163975 [details]
Patch
Comment 9 David Grogan 2012-09-13 16:31:04 PDT
Created attachment 163998 [details]
Patch
Comment 10 David Grogan 2012-09-13 18:03:58 PDT
Created attachment 164018 [details]
Patch
Comment 11 David Grogan 2012-09-13 18:04:36 PDT
Tony, could you review this?
Comment 12 Tony Chang 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.
Comment 13 Tony Chang 2012-09-13 18:22:30 PDT
Of course the histogram can be a separate change and doesn't need to block this change.
Comment 14 David Grogan 2012-09-13 19:13:31 PDT
Created attachment 164027 [details]
Patch
Comment 15 David Grogan 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.
Comment 16 Tony Chang 2012-09-14 10:07:53 PDT
Comment on attachment 164027 [details]
Patch

Once per object seems OK.
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2012-09-14 10:27:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 James Robinson 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.
Comment 20 James Robinson 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>
Comment 21 David Grogan 2012-09-19 18:53:27 PDT
Created attachment 164820 [details]
ToT
Comment 22 David Grogan 2012-09-19 20:53:52 PDT
Created attachment 164832 [details]
Patch for landing
Comment 23 David Grogan 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.
Comment 24 WebKit Review Bot 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>
Comment 25 WebKit Review Bot 2012-09-19 21:49:30 PDT
All reviewed patches have been landed.  Closing bug.