[webkit-reviews] review denied: [Bug 55095] IndexedDB: fire versionchange events when calling setVersion : [Attachment 84351] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 1 19:25:52 PST 2011
Jeremy Orlow <jorlow at chromium.org> has denied David Grogan
<dgrogan at chromium.org>'s request for review:
Bug 55095: IndexedDB: fire versionchange events when calling setVersion
https://bugs.webkit.org/show_bug.cgi?id=55095
Attachment 84351: Patch
https://bugs.webkit.org/attachment.cgi?id=84351&action=review
------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84351&action=review
> Source/WebCore/storage/IDBDatabase.cpp:145
> + EventQueue* eventQueue =
static_cast<Document*>(scriptExecutionContext())->eventQueue();
assert it's a document before static casting (IsDocument())
> Source/WebCore/storage/IDBDatabase.cpp:146
> + for (size_t i = 0; i < m_enqueuedEvents.size(); ++i) {
Is this really necessary? What's the worst case without it?
> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:170
> +
callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::NOT_ALLOWED_E
RR, "Connection was closed before set version transaction was created"));
Did we agree on NOT_ALLOWED_ERR? ABORT_ERR might make more sense?
Is this possible? Seems like it wouldn't be and that we should be blocking it
in the frontend. If so, just do an assert. If not, why isn't it?
> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:173
> + for (DatabaseCallbacksSet::const_iterator it =
m_databaseCallbacksSet.begin(); it != m_databaseCallbacksSet.end(); ++it) {
Are we supposed to send this even when nothing's blocked?
> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:234
> RefPtr<PendingSetVersionCall> pendingSetVersionCall =
m_pendingSetVersionCalls.takeFirst();
Can you update the code I just landed to use takeFirst (in EventQueue). That
seems better.
> Source/WebCore/storage/IDBRequest.cpp:193
> +
newline seems kinda random...but no big deal
> Source/WebKit/chromium/src/IDBDatabaseProxy.cpp:95
> +void IDBDatabaseProxy::setVersion(const String& version,
PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<IDBDatabaseCallbacks>
dbcallbacks, ExceptionCode& ec)
databaseCallbacks (don't abbreviate and camelCase)
here and elsewhere
> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:102
> + // Use the callbacks that ::open gave us to ensure that the backend in
ASSERT they're not null before passing in. (The only time we wouldn't assert
is if we were about to do something that'd _crash_ if it was null.)
More information about the webkit-reviews
mailing list