Bug 96575 – IndexedDB: Print console warning about setVersion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: Print console warning about setVersion

[David Grogan]

Created attachment 163732 [details]
Patch

[David Grogan]

jsbell@, could you look at this?

This is copy/pasted from alecflett's cursor warnings.

[Joshua Bell]

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 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 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]

Created attachment 163959 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 163975 [details]
Patch

[David Grogan]

Created attachment 163998 [details]
Patch

[David Grogan]

Created attachment 164018 [details]
Patch

[David Grogan]

Tony, could you review this?

[Tony Chang]

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]

Of course the histogram can be a separate change and doesn't need to block this change.

[David Grogan]

Created attachment 164027 [details]
Patch

[David Grogan]

(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 on attachment 164027 [details]
Patch

Once per object seems OK.

[WebKit Review Bot]

Comment on attachment 164027 [details]
Patch

Clearing flags on attachment: 164027

Committed r128627: <http://trac.webkit.org/changeset/128627>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

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]

Reverted r128627 for reason:

Breaks indexeddb content_browsertests in chromium

Committed r128642: <http://trac.webkit.org/changeset/128642>

[David Grogan]

Created attachment 164820 [details]
ToT

[David Grogan]

Created attachment 164832 [details]
Patch for landing

[David Grogan]

chromium r157696 disabled all the content_browsertests that this should break, so it'll hopefully stick this time.

[WebKit Review Bot]

Comment on attachment 164832 [details]
Patch for landing

Clearing flags on attachment: 164832

Committed r129090: <http://trac.webkit.org/changeset/129090>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.