WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(123.81 KB, patch)
2012-09-13 14:02 PDT
,
David Grogan
no flags
Details
Formatted Diff
Diff
Patch
(123.76 KB, patch)
2012-09-13 15:06 PDT
,
David Grogan
no flags
Details
Formatted Diff
Diff
Patch
(127.23 KB, patch)
2012-09-13 16:31 PDT
,
David Grogan
no flags
Details
Formatted Diff
Diff
Patch
(127.22 KB, patch)
2012-09-13 18:03 PDT
,
David Grogan
no flags
Details
Formatted Diff
Diff
Patch
(123.75 KB, patch)
2012-09-13 19:13 PDT
,
David Grogan
no flags
Details
Formatted Diff
Diff
ToT
(122.88 KB, patch)
2012-09-19 18:53 PDT
,
David Grogan
no flags
Details
Formatted Diff
Diff
Patch for landing
(122.96 KB, patch)
2012-09-19 20:53 PDT
,
David Grogan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
David Grogan
Comment 1
2012-09-12 16:34:51 PDT
Created
attachment 163732
[details]
Patch
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
Created
attachment 163959
[details]
Patch
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
Created
attachment 163975
[details]
Patch
David Grogan
Comment 9
2012-09-13 16:31:04 PDT
Created
attachment 163998
[details]
Patch
David Grogan
Comment 10
2012-09-13 18:03:58 PDT
Created
attachment 164018
[details]
Patch
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
Created
attachment 164027
[details]
Patch
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
Created
attachment 164820
[details]
ToT
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.
Top of Page
Format For Printing
XML
Clone This Bug