[webkit-reviews] review denied: [Bug 72066] IndexedDB: Close database connections, abort transactions, and terminate requests on stop() : [Attachment 115407] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Joshua Bell <jsbell at chromium.org>'s
request for review:
Bug 72066: IndexedDB: Close database connections, abort transactions, and
terminate requests on stop()
https://bugs.webkit.org/show_bug.cgi?id=72066

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115407&action=review


OVERRIDE is new in webkit.  Now is as good a time as any to start using it.  r-
since the cq will reject the ChangeLog anyway.

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

You need to remove the OOPS! or the cq won't land this.  Please add a test or
explain why this isn't testable.

> Source/WebCore/dom/DocumentEventQueue.h:55
> +    virtual bool enqueueEvent(PassRefPtr<Event>);

Nit: Can you add OVERRIDE here?

> Source/WebCore/storage/IDBDatabase.cpp:237
> +

Nit: Did you mean to add this blank line?

>> Source/WebCore/storage/IDBRequest.cpp:58
>> +	, m_stopped(false)
> 
> Track stopped (= document is stopped) vs. finished (the IDBRequest has been
fulfilled)

Nit: Can we name the variables to make this more clear?  E.g.,
m_documentStopped and m_requestFulfilled.

> Source/WebCore/storage/IDBRequest.h:91
>      virtual bool hasPendingActivity() const;
> +    virtual void stop();

OVERRIDE

> Source/WebCore/storage/IDBTransaction.h:85
>      virtual bool hasPendingActivity() const;
>      virtual bool canSuspend() const;
> -    virtual void contextDestroyed();
> +    virtual void stop();

OVERRIDE

> Source/WebCore/workers/WorkerEventQueue.h:49
> +    virtual bool enqueueEvent(PassRefPtr<Event>);

Nit: OVERRIDE