| Summary: | indexeddb: make setVersion fire blocked event if other connections are open | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, jorlow | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
David Grogan
2011-02-03 15:59:33 PST
Created attachment 81132 [details]
Patch
Comment on attachment 81132 [details]
Patch
This first patch doesn't change any behavior, it's just some architectural support. Sending it now to get early feedback from Jeremy.
Comment on attachment 81132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81132&action=review not too far off this will require a couple stages to land, but let's get it all good first and then we can worry aobut that > Source/WebCore/WebCore.gypi:4006 > + 'storage/IDBBlockedEvent.cpp', i think you'll need an idl, right? > Source/WebCore/storage/IDBBlockedEvent.cpp:2 > + * Copyright (C) 2010 Google Inc. All rights reserved. Use this here and elsewhere: http://webkit.org/coding/bsd-license.html 2011 > Source/WebCore/storage/IDBBlockedEvent.cpp:45 > + : IDBEvent(eventNames().abortEvent, 0) // FIXME: set the source? implement the fixme....this should take in a PassRefPtr<IDBAny> source attribute and pass it through to IDBEvent > Source/WebCore/storage/IDBDatabase.cpp:97 > + RefPtr<IDBVersionChangeRequest> request = IDBVersionChangeRequest::create(context, IDBAny::create(this), 0); get rid of the 0 since we'll never have a transaction to pass in > Source/WebCore/storage/IDBDatabase.cpp:135 > + return; 4 spaces > Source/WebCore/storage/IDBDatabase.cpp:137 > + m_backend->decrementConnectionCount(); why not just leave this "close()"? also shouldn't you pass "this" in? > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:98 > + , m_openConnectionCount(0) I thouhght you were going to maintain a set since you'll need that in the next patch? > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:215 > + // Call onblocked here if connection count is >0 Use complete sentences. > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:216 > + (it->second)->incrementConnectionCount(); ()'s unneeded > Source/WebCore/storage/IDBRequest.h:72 > + virtual void onBlocked() { ASSERT_NOT_REACHED(); } maybe move the body into the .cpp since a lot of files compile this > Source/WebCore/storage/IDBVersionChangeRequest.cpp:34 > +#include "Event.h" Please cut down this list. > Source/WebCore/storage/IDBVersionChangeRequest.idl:33 > + EventTarget inherit from IDBRequest...i think it should mostly do the right thing...? > Source/WebCore/storage/IDBVersionChangeRequest.idl:37 > + const unsigned short LOADING = 1; get rid of this stuff? > Source/WebCore/storage/IDBVersionChangeRequest.idl:42 > + attribute EventListener onsuccess; remove onsuccess and on error > Source/WebCore/storage/IDBVersionChangeRequest.idl:46 > + // EventTarget interface get rid of this stuff? > Source/WebKit/chromium/src/IDBCallbacksProxy.cpp:111 > +{ call the IDBRequest version Created attachment 81565 [details]
obsoleted Patch 1
Comment on attachment 81565 [details]
obsoleted Patch 1
This isn't ready to be committed, but close.
I don't think the call to close() in IDBDatabase::~IDBDatabase() is right. If you load the layout test in chromium everything is fine until you hit refresh. The destructor on the IDBDatabase object from the first page load isn't called until after the new page loads, causing setVersion to call blocked when it's supposed to succeed.
I'm not sure what to do about this.
Comment on attachment 81565 [details] obsoleted Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=81565&action=review closer > LayoutTests/storage/indexeddb/set_version_blocked.html:1 > +<html> You need to create expected results. > LayoutTests/storage/indexeddb/set_version_blocked.html:20 > + if ('webkitIndexedDB' in window) { no {} for one liner > LayoutTests/storage/indexeddb/set_version_blocked.html:58 > +function setVersion(tries) { newline > LayoutTests/storage/indexeddb/set_version_blocked.html:65 > +function unexpectedBlockCallback() { put in the shared.js file...and call it "Blocked" not "Block" > Source/WebCore/dom/EventTarget.h:143 > + virtual IDBVersionChangeRequest* toIDBVersionChangeRequest(); alpha order > Source/WebCore/storage/IDBDatabase.cpp:135 > + if (m_noNewTransactions) Stuff like this should be tested in the layout test. > Source/WebCore/storage/IDBDatabaseBackendImpl.h:65 > + virtual void open(); You'll need to add this to the WebKit:: stuff. > Source/WebCore/storage/IDBRequest.cpp:190 > +void IDBRequest::scheduleBlockedEvent() You need to sync your repo. This has all changed a bunch. > Source/WebCore/storage/IDBVersionChangeRequest.h:32 > +#include "Timer.h" Why is this needed? > Source/WebCore/storage/IDBVersionChangeRequest.h:33 > +#include <wtf/Vector.h> why is this needed? > Source/WebCore/storage/IDBVersionChangeRequest.h:37 > +class IDBTransactionBackendInterface; why is this needed? > Source/WebCore/storage/IDBVersionChangeRequest.h:41 > + static PassRefPtr<IDBVersionChangeRequest> create(ScriptExecutionContext* context, PassRefPtr<IDBAny> source) { return adoptRef(new IDBVersionChangeRequest(context, source)); } lately i've been shying away from inlining these. Maybe move it? > Source/WebCore/storage/IDBVersionChangeRequest.h:43 > + virtual void onBlocked(); usually we do a newline between the constructor/destructor stuff and other stuff > Source/WebCore/storage/IDBVersionChangeRequest.h:49 > + no blank line Created attachment 81862 [details]
obsoleted patch 2
Comment on attachment 81565 [details] obsoleted Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=81565&action=review >> LayoutTests/storage/indexeddb/set_version_blocked.html:1 >> +<html> > > You need to create expected results. done >> LayoutTests/storage/indexeddb/set_version_blocked.html:20 >> + if ('webkitIndexedDB' in window) { > > no {} for one liner done >> LayoutTests/storage/indexeddb/set_version_blocked.html:58 >> +function setVersion(tries) { > > newline done >> LayoutTests/storage/indexeddb/set_version_blocked.html:65 >> +function unexpectedBlockCallback() { > > put in the shared.js file...and call it "Blocked" not "Block" done >> Source/WebCore/dom/EventTarget.h:143 >> + virtual IDBVersionChangeRequest* toIDBVersionChangeRequest(); > > alpha order done >> Source/WebCore/storage/IDBDatabase.cpp:135 >> + if (m_noNewTransactions) > > Stuff like this should be tested in the layout test. done >> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65 >> + virtual void open(); > > You'll need to add this to the WebKit:: stuff. This round? It seems that nothing would use it when only doing ref counting. >> Source/WebCore/storage/IDBRequest.cpp:190 >> +void IDBRequest::scheduleBlockedEvent() > > You need to sync your repo. This has all changed a bunch. done >> Source/WebCore/storage/IDBVersionChangeRequest.h:32 >> +#include "Timer.h" > > Why is this needed? it's not, deleted >> Source/WebCore/storage/IDBVersionChangeRequest.h:33 >> +#include <wtf/Vector.h> > > why is this needed? it's not, deleted >> Source/WebCore/storage/IDBVersionChangeRequest.h:37 >> +class IDBTransactionBackendInterface; > > why is this needed? it's not, deleted >> Source/WebCore/storage/IDBVersionChangeRequest.h:41 >> + static PassRefPtr<IDBVersionChangeRequest> create(ScriptExecutionContext* context, PassRefPtr<IDBAny> source) { return adoptRef(new IDBVersionChangeRequest(context, source)); } > > lately i've been shying away from inlining these. Maybe move it? I'm a fan of "no implementation in the .h files"; moved. >> Source/WebCore/storage/IDBVersionChangeRequest.h:43 >> + virtual void onBlocked(); > > usually we do a newline between the constructor/destructor stuff and other stuff done >> Source/WebCore/storage/IDBVersionChangeRequest.h:49 >> + > > no blank line done (In reply to comment #8) > (From update of attachment 81565 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81565&action=review > >> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65 > >> + virtual void open(); > > > > You'll need to add this to the WebKit:: stuff. > > This round? It seems that nothing would use it when only doing ref counting. Oops, you're right. Comment on attachment 81862 [details] obsoleted patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=81862&action=review close > LayoutTests/storage/indexeddb/set_version_blocked.html:31 > + verifyResult(result); In new tests, I'm voiding these because at this point they pretty much just add noise > LayoutTests/storage/indexeddb/set_version_blocked.html:39 > + verifySuccessEvent(event); Ditto....fix throughout > LayoutTests/storage/indexeddb/set_version_blocked.html:42 > + if (connections.length < 3) { no {} > LayoutTests/storage/indexeddb/set_version_blocked.html:44 > + } else { no {} > LayoutTests/storage/indexeddb/set_version_blocked.html:63 > +// Try 1: Close a connection. Stuff like this should be in shouldBe/testPassed/debug()'s. The idea is that you should be able to follow along via the on-screen output. > Source/WebCore/storage/IDBBlockedEvent.cpp:42 > + : IDBEvent(eventNames().blockedEvent, source, false/*canBubble*/) space before /*? > Source/WebCore/storage/IDBBlockedEvent.h:45 > + virtual String version(); newline > Source/WebCore/storage/IDBBlockedEvent.h:48 > + IDBBlockedEvent(PassRefPtr<IDBAny> source, const String& version); newline > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217 > + return; You need to add this to a queue and start the queue once there's only one connection left. And put a FIXME to only fire the event if the DB is still open. > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:260 > void IDBDatabaseBackendImpl::close() On close, you need to re-try the set-version transaction. > Source/WebCore/storage/IDBDatabaseBackendImpl.h:65 > + virtual void open(); shouldn't be virtual. Group up above with the other non-virtual/not-in-idl stuff (like id) > Source/WebCore/storage/IDBRequest.h:85 > + virtual void enqueueEvent(PassRefPtr<Event>); don't make stuff virtual unless necessary > Source/WebCore/storage/IDBRequest.h:86 > + virtual IDBAny* source(); I don't see any reason not to just make this public. But don't make virtual. > Source/WebCore/storage/IDBVersionChangeRequest.cpp:40 > +IDBVersionChangeRequest::IDBVersionChangeRequest(ScriptExecutionContext* context, PassRefPtr<IDBAny> source, const String& version) newline > Source/WebCore/storage/IDBVersionChangeRequest.h:46 > + String m_version; maybe a newline between these? Comment on attachment 81862 [details] obsoleted patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=81862&action=review >> LayoutTests/storage/indexeddb/set_version_blocked.html:31 >> + verifyResult(result); > > In new tests, I'm voiding these because at this point they pretty much just add noise done >> LayoutTests/storage/indexeddb/set_version_blocked.html:39 >> + verifySuccessEvent(event); > > Ditto....fix throughout done >> LayoutTests/storage/indexeddb/set_version_blocked.html:42 >> + if (connections.length < 3) { > > no {} done >> LayoutTests/storage/indexeddb/set_version_blocked.html:44 > > no {} done >> LayoutTests/storage/indexeddb/set_version_blocked.html:63 > > Stuff like this should be in shouldBe/testPassed/debug()'s. The idea is that you should be able to follow along via the on-screen output. gotcha >> Source/WebCore/storage/IDBBlockedEvent.cpp:42 >> + : IDBEvent(eventNames().blockedEvent, source, false/*canBubble*/) > > space before /*? done >> Source/WebCore/storage/IDBBlockedEvent.h:45 >> + virtual String version(); > > newline done >> Source/WebCore/storage/IDBBlockedEvent.h:48 >> + IDBBlockedEvent(PassRefPtr<IDBAny> source, const String& version); > > newline done >> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217 >> + return; > > You need to add this to a queue and start the queue once there's only one connection left. And put a FIXME to only fire the event if the DB is still open. done >> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:260 >> void IDBDatabaseBackendImpl::close() > > On close, you need to re-try the set-version transaction. done >> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65 >> + virtual void open(); > > shouldn't be virtual. Group up above with the other non-virtual/not-in-idl stuff (like id) done >> Source/WebCore/storage/IDBRequest.h:85 >> + virtual void enqueueEvent(PassRefPtr<Event>); > > don't make stuff virtual unless necessary done >> Source/WebCore/storage/IDBRequest.h:86 >> + virtual IDBAny* source(); > > I don't see any reason not to just make this public. But don't make virtual. done >> Source/WebCore/storage/IDBVersionChangeRequest.cpp:40 >> +IDBVersionChangeRequest::IDBVersionChangeRequest(ScriptExecutionContext* context, PassRefPtr<IDBAny> source, const String& version) > > newline done >> Source/WebCore/storage/IDBVersionChangeRequest.h:46 >> + String m_version; > > maybe a newline between these? done Created attachment 82197 [details]
Patch
Comment on attachment 82197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82197&action=review almost there > LayoutTests/storage/indexeddb/set_version_queue.html:84 > + debug("inSetVersion2"); testPassed > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217 > + prpCallbacks->onBlocked(); Usually the first thing we do in a function is assign the prp types to normal RefPtr types. I know we don't do that some in these files, but we should probably try to get back to it. So move that stuff up and then s/prpC/c/ here. > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:218 > + RefPtr<PendingSetVersionCall> pendingSetVersionCall = adoptRef(new PendingSetVersionCall); the class should have a factory and new should be private. > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:219 > + pendingSetVersionCall->m_callbacks = prpCallbacks; use callbacks > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:269 > + if (m_openConnectionCount == 1) { Just do a return if > 1 so the rest isn't nested. newline after the return too, i'd think > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:271 > + // FIXME: Do something better with ec? ASSERT(!ec || whatever else it might be that's OK)...and remove the fixme > Source/WebCore/storage/IDBDatabaseBackendImpl.h:102 > + struct PendingSetVersionCall : RefCounted<PendingSetVersionCall> { I'm not sure we really use structs much in WebKit. Just forward declare the class here and define it in the .cpp Comment on attachment 82197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82197&action=review >> LayoutTests/storage/indexeddb/set_version_queue.html:84 >> + debug("inSetVersion2"); > > testPassed done >> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217 >> + prpCallbacks->onBlocked(); > > Usually the first thing we do in a function is assign the prp types to normal RefPtr types. I know we don't do that some in these files, but we should probably try to get back to it. So move that stuff up and then s/prpC/c/ here. done >> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:218 >> + RefPtr<PendingSetVersionCall> pendingSetVersionCall = adoptRef(new PendingSetVersionCall); > > the class should have a factory and new should be private. done >> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:219 >> + pendingSetVersionCall->m_callbacks = prpCallbacks; > > use callbacks done >> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:269 >> + if (m_openConnectionCount == 1) { > > Just do a return if > 1 so the rest isn't nested. newline after the return too, i'd think done >> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:271 >> + // FIXME: Do something better with ec? > > ASSERT(!ec || whatever else it might be that's OK)...and remove the fixme done. I don't know what might be ok so went conservative with ASSERT(!ec) >> Source/WebCore/storage/IDBDatabaseBackendImpl.h:102 >> + struct PendingSetVersionCall : RefCounted<PendingSetVersionCall> { > > I'm not sure we really use structs much in WebKit. Just forward declare the class here and define it in the .cpp done Created attachment 82209 [details]
Patch
Created attachment 82358 [details]
Patch
Comment on attachment 82358 [details] Patch Removes small change to webkit whose bug was https://bugs.webkit.org/show_bug.cgi?id=54329 Comment on attachment 82358 [details]
Patch
rubber stamp assuming nothing else changed
Comment on attachment 82358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82358&action=review > Source/WebKit/chromium/ChangeLog:8 > + * public/WebIDBCallbacks.h: remove these 2 lines Comment on attachment 82358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82358&action=review >> Source/WebKit/chromium/ChangeLog:8 >> + * public/WebIDBCallbacks.h: > > remove these 2 lines done Created attachment 82378 [details]
Patch
Comment on attachment 82378 [details] Patch Rejecting attachment 82378 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2 Last 500 characters of output: st.h patching file Source/WebCore/storage/IDBVersionChangeRequest.idl patching file Source/WebKit/chromium/ChangeLog patching file Source/WebKit/chromium/src/IDBCallbacksProxy.cpp patching file Source/WebKit/chromium/src/IDBCallbacksProxy.h patching file Source/WebKit/chromium/src/WebIDBCallbacksImpl.cpp patching file Source/WebKit/chromium/src/WebIDBCallbacksImpl.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Jeremy Orlow', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/7905790 Comment on attachment 82378 [details]
Patch
Crap. The patch I just landed will mess this up. Sorry.
Created attachment 82700 [details]
Patch
Comment on attachment 82700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82700&action=review > Source/WebCore/dom/EventTarget.cpp:192 > +IDBVersionChangeRequest* EventTarget::toIDBVersionChangeRequest() alpha order |