Bug 94972 - IndexedDB: Remove IDBDatabase.setVersion API
Summary: IndexedDB: Remove IDBDatabase.setVersion API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords: WebExposed
Depends on: 101810 102457 102458 102713 102850
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-24 13:58 PDT by Joshua Bell
Modified: 2012-11-27 12:46 PST (History)
8 users (show)

See Also:


Attachments
Patch (28.46 KB, patch)
2012-11-19 16:52 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (29.86 KB, patch)
2012-11-19 16:54 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (42.51 KB, patch)
2012-11-20 17:43 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (33.92 KB, patch)
2012-11-27 10:39 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (34.65 KB, patch)
2012-11-27 11:15 PST, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-08-24 13:58:14 PDT
The IDBDatabase.setVersion() API is from an older version of the spec, and maintained for compatibility until sites using the Chromium implementation of IndexedDB have had a chance to update. The new IDBFactory.open(name, version) / "upgradeneeded" event model is the new way to version databases.

Once a grace period has passed, we should remove it.
Comment 1 David Grogan 2012-11-19 16:52:16 PST
Created attachment 175078 [details]
Patch
Comment 2 David Grogan 2012-11-19 16:54:08 PST
Created attachment 175080 [details]
Patch
Comment 3 David Grogan 2012-11-19 17:01:29 PST
This patch leaves some setVersion artifacts in IDBDatabaseBackendImpl, that class is pretty fragile. There are also some remnants in the webkit api that can't be taken out until chromium code is updated.
Comment 4 WebKit Review Bot 2012-11-19 21:10:12 PST
Comment on attachment 175080 [details]
Patch

Attachment 175080 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14906449

New failing tests:
storage/indexeddb/pending-version-change-on-exit.html
http/tests/inspector/indexeddb/database-data.html
http/tests/inspector/indexeddb/database-structure.html
http/tests/inspector/indexeddb/resources-panel.html
Comment 5 Joshua Bell 2012-11-20 09:58:25 PST
Comment on attachment 175080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175080&action=review

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:314
> +    ASSERT(m_pendingSetVersionCalls.isEmpty());

Why keep this?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:110
> +    // FIXME: Remove these.

And these? I don't see anything remaining that creates them.
Comment 6 David Grogan 2012-11-20 17:43:22 PST
Created attachment 175312 [details]
Patch
Comment 7 David Grogan 2012-11-20 17:44:46 PST
(In reply to comment #5)
> (From update of attachment 175080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175080&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:314
> > +    ASSERT(m_pendingSetVersionCalls.isEmpty());
> 
> Why keep this?
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:110
> > +    // FIXME: Remove these.
> 
> And these? I don't see anything remaining that creates them.

Removed both.
Comment 8 Joshua Bell 2012-11-27 09:39:56 PST
Comment on attachment 175312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175312&action=review

> LayoutTests/ChangeLog:14
> +        IndexedDB: remove setVersion from inspector tests

Looks like this patch was based on another (leading to two changelog entries and the extra diff below). Rebase please?

> LayoutTests/storage/indexeddb/removed-expected.txt:9
> +PASS 'setVersion' in IDBDatabase.prototype is false

FYI this will need rebasing as nearby lines have changed.
Comment 9 David Grogan 2012-11-27 10:39:22 PST
Created attachment 176296 [details]
Patch
Comment 10 Joshua Bell 2012-11-27 11:10:49 PST
Comment on attachment 176296 [details]
Patch

lgtm, w00t! (looks like it will need another rebase on top of r135856 though)
Comment 11 David Grogan 2012-11-27 11:15:05 PST
Created attachment 176306 [details]
Patch
Comment 12 David Grogan 2012-11-27 11:32:33 PST
Tony, could you review this?
Comment 13 WebKit Review Bot 2012-11-27 12:46:30 PST
Comment on attachment 176306 [details]
Patch

Clearing flags on attachment: 176306

Committed r135904: <http://trac.webkit.org/changeset/135904>
Comment 14 WebKit Review Bot 2012-11-27 12:46:35 PST
All reviewed patches have been landed.  Closing bug.