[webkit-reviews] review denied: [Bug 53728] indexeddb: make setVersion fire blocked event if other connections are open : [Attachment 81132] 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 53728: indexeddb: make setVersion fire blocked event if other connections
are open
https://bugs.webkit.org/show_bug.cgi?id=53728

Attachment 81132: Patch
https://bugs.webkit.org/attachment.cgi?id=81132&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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