[webkit-reviews] review denied: [Bug 55095] IndexedDB: fire versionchange events when calling setVersion : [Attachment 83570] Patch

[bugzilla-daemon at webkit.org]

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 83570: Patch
https://bugs.webkit.org/attachment.cgi?id=83570&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83570&action=review

overall looks pretty good...minus the connection id thing

> LayoutTests/storage/indexeddb/set_version_queue.html:71
> +    versionChangeNotifications.push("connection " + connectionNum +

no need to wrap..and you could maybe just put it in the closure above?

> Source/WebCore/ChangeLog:12
> +	   4) When a connections call setVersion, backend calls

When a connection calls setVersion, the backend calls

> Source/WebCore/storage/IDBDatabase.cpp:177
> +bool IDBDatabase::dispatchEvent(PassRefPtr<Event> event)

Is there any reason the default dispatchEvent won't do?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:50
> +    int connectionId() { return m_connectionId; }

newline before private..

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:218
> +void IDBDatabaseBackendImpl::open(int connectionId,
PassRefPtr<IDBDatabaseCallbacks> callbacks)

You should just have one version of this function.  And it should simply take
in the PassRefPtr

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:229
> +void IDBDatabaseBackendImpl::close(int connectionId)

This should take in an IDBDatabaseCallbacks ptr...and assert it's there before
you remove it.

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:101
> +    int m_nextId;

no longer necessary

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:106
> +    typedef HashMap<int, RefPtr<IDBDatabaseCallbacks> >
DatabaseCallbacksMap;

This can probably just be a HashSet.  Actually use a LinkHashSet so that you
can iterate over them deterministically.  Make a comment that this is
important.

> Source/WebCore/storage/IDBDatabaseCallbacks.h:34
> +#include "SerializedScriptValue.h"

not needed...you likely will need PlatformString. if you delete tho.

> Source/WebCore/storage/IDBRequest.cpp:154
> +

extra newline

> Source/WebKit/chromium/public/WebIDBDatabaseCallbacks.h:38
> +

no newlines

> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.h:55
> +

no double newline