[Webkit-unassigned] [Bug 44700] IDBObjectStore::get should run in a transaction.

Mon Sep 20 10:53:42 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=44700

Jeremy Orlow <jorlow at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68067|review?                     |review-
               Flag|                            |

--- Comment #12 from Jeremy Orlow <jorlow at chromium.org>  2010-09-20 10:53:42 PST ---
(From update of attachment 68067)
a few of my comments

View in context: https://bugs.webkit.org/attachment.cgi?id=68067&action=review

> WebCore/storage/IDBDatabase.cpp:60
> +    // FIXME: remove this method.

put above function definition

> WebCore/storage/IDBDatabaseBackendImpl.cpp:153
> +    // FIXME: Do not expose this method via IDL.

move up

> WebCore/storage/IDBDatabaseBackendImpl.cpp:154
>      ASSERT_UNUSED(mode, !mode); // FIXME: Handle non-standard modes.

Change the fixme to be a "delete this".  It should be deleted when we remove it from the IDL.

> WebCore/storage/IDBDatabaseBackendImpl.cpp:156
> +    return objectStore.release();

For stuff like this, I usually just do a .get() on what's returned which will coerce itself type wise...of course that will cause a needless ref count/de-ref.  Your choice.

> WebCore/storage/IDBDatabaseBackendInterface.h:58
> +    virtual PassRefPtr<IDBObjectStoreBackendInterface> objectStore(const String& name, unsigned short mode, IDBTransactionBackendInterface* transaction = 0) = 0;

Put a fixme in to remove the = 0 since it's just a compat hack.

> WebCore/storage/IDBFactoryBackendImpl.cpp:147
> +    m_transactionCoordinator->abortPendingTransactions(pendingIDs);

Why do we still use ids here?  THis is in the backendImpl, so we shouldn't need to.

> WebCore/storage/IDBObjectStore.cpp:70
>      return request;

.release()

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:84
> +    if (transaction) {

Would this ever not be true?

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:90
> +        OwnPtr<ScriptExecutionContext::Task> getTask = adoptPtr(newTask(this, &IDBObjectStoreBackendImpl::get, key, callbacks));

Why do you have to adopt the newTask pointer?  Shouldn't it return a passOwnPtr?

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:91
> +        transaction->scheduleTask(getTask.leakPtr());

Why do you call leak?  Can't you pass a passownptr?

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:354
> +

delete

> WebCore/storage/IDBObjectStoreBackendImpl.h:75
> +    class IDBTask : public ScriptExecutionContext::Task {

I think add a create method that returns a passOwnPtr

> WebCore/storage/IDBObjectStoreBackendImpl.h:78
> +            : m_obj(obj), m_method(method), m_param1(param1), m_param2(param2) {

{ on new line

> WebCore/storage/IDBObjectStoreBackendImpl.h:81
> +        virtual void performTask(ScriptExecutionContext*) {

ditto

> WebCore/storage/IDBObjectStoreBackendImpl.h:84
> +        }

newline after

> WebCore/storage/IDBObjectStoreBackendImpl.h:93
> +    ScriptExecutionContext::Task* newTask(T* object, Method method, const Param1& param1, const Param2& param2) {

Should return a passOwnPtr

> WebCore/storage/IDBObjectStoreBackendImpl.h:94
> +        return new IDBTask<T, Method, Param1, Param2 >(object, method, param1, param2);

no space after Param2

> WebCore/storage/IDBObjectStoreBackendImpl.h:106
> +

no newline

> WebCore/storage/IDBPendingTransactionMonitor.cpp:46
> +        if (!m_ids)

tabbed in too much

> WebCore/storage/IDBPendingTransactionMonitor.cpp:64
> +    if (m_ids) {

Can't you just bail if this is true?  If so, then you can declare the vector below

> WebCore/storage/IDBPendingTransactionMonitor.cpp:69
> +    if (pendingIds.size()) {

Why would this be true and above wouldn't?

> WebCore/storage/IDBPendingTransactionMonitor.cpp:75
> +void IDBPendingTransactionMonitor::didCompleteEventsForTransaction(int id, IDBFactoryBackendInterface* idbFactory)

why are we using an id?

> WebCore/storage/IDBRequest.cpp:60
> +    if (transactionId)

one place you do > 0 one place just check if it's == 0...which one is right?

> WebCore/storage/IDBRequest.cpp:110
> +    // FIXME: the transaction id should be the one of the setVersion transaction.

True, but only because this is only used by a method that is always executed in a setVersion transaction.  Maybe make the comment a bit more clear.

> WebCore/storage/IDBRequest.cpp:172
> +    if (m_transactionId > 0) {

one place you do > 0 one place just check if it's == 0...which one is right?

> WebCore/storage/IDBRequest.cpp:173
> +        // Now that we processed all pending events, let the transaction monitor

maybe wrap to 2 lines rather than 3?

> WebCore/storage/IDBRequest.cpp:176
> +        ASSERT(scriptExecutionContext()->isDocument());

Add a FIXME for handling the workers case.

> WebCore/storage/IDBRequest.h:40
> +#include "IDBTransaction.h"

not necessary

> WebCore/storage/IDBTransaction.cpp:69
> +    if (!objectStoreBackend.get()) {

don't need the .get()

> WebCore/storage/IDBTransaction.cpp:70
> +        throwError(NOT_SUPPORTED_ERR);

Do you need to add the string for this error somewhere?

> WebCore/storage/IDBTransactionBackendImpl.cpp:51
> +    , m_timer(this, &IDBTransactionBackendImpl::timerFired)

The timer is just here until we add threading, right?  Maybe add a FIXME?

> WebCore/storage/IDBTransactionBackendImpl.cpp:65
> +

newline doesn't really seem necessary (maybe delete?)

> WebCore/storage/IDBTransactionBackendImpl.cpp:67
> +

newline doesn't really seem necessary (maybe delete?)

> WebCore/storage/IDBTransactionBackendImpl.cpp:72
> +void IDBTransactionBackendImpl::step()

step doesn't seem like a very good name

> WebCore/storage/IDBTransactionBackendImpl.cpp:75
> +

newline doesn't really seem necessary (maybe delete?)

> WebCore/storage/IDBTransactionBackendImpl.cpp:76
> +    if (!m_taskQueue.isEmpty()) {

if / else seems cleaner and less lines of code

> WebCore/storage/IDBTransactionBackendImpl.cpp:101
> +    m_timer.startOneShot(0);

should we verify that it's not currently active?

> WebCore/storage/IDBTransactionBackendInterface.h:54
> +    virtual void step() = 0;

step is not very descriptive

> WebCore/storage/IDBTransactionCoordinator.cpp:61
> +        RefPtr<IDBTransactionBackendInterface> transaction = m_pendingTransactions.get(id);

It doesn't need to be removed?

> WebCore/storage/IDBTransactionCoordinator.cpp:62
> +        ASSERT(transaction);

not necessary

> WebCore/storage/IDBTransactionCoordinator.cpp:69
> +    if (m_runningTransactions.contains(transactionID)) {

When can this be false?  Should we assert it's not one of the other types?

> WebCore/storage/IDBTransactionCoordinator.cpp:70
> +      RefPtr<IDBTransactionBackendInterface> transaction = m_runningTransactions.get(transactionID);

4 spaces

> WebCore/storage/IDBTransactionCoordinator.cpp:92
> +    // Remove from pending transactions list

. at end

> WebCore/storage/IDBTransactionCoordinator.cpp:123
> +    // FIXME: this would allocate a thread to the next transaction

Capital T on This.
Maybe s/would/should/?

> WebCore/storage/IDBTransactionCoordinator.cpp:126
> +    if (m_startedTransactions.isEmpty())

maybe if (... || ...)
    return ?

> WebCore/storage/IDBTransactionCoordinator.cpp:132
> +    IDBTransactionBackendInterface* transactionPtr = *(m_startedTransactionQueue.begin());

I believe you don't need ()'s

> WebCore/storage/IDBTransactionCoordinator.cpp:134
> +    RefPtr<IDBTransactionBackendInterface> transaction = m_startedTransactions.get(transactionPtr->id());

transaction.get() should == transactionPtr, right?  My suggestion:

RefPtr<> transaction = *_startedTransactionQueue.begin();
ASSERT(transaction.get() == m_startedTransactions.get(transaction->id()).get());
...

> WebCore/storage/IDBTransactionCoordinator.cpp:136
> +    // Add to running transactions list.

comment isn't useful

> WebCore/storage/IDBTransactionCoordinator.cpp:140
> +    static_cast<IDBTransactionBackendImpl*>(transactionPtr)->run();

Don't do this.  I guess you should be storing impl ptrs rather than interface ptrs?

> WebCore/storage/IDBTransactionCoordinator.h:71
> +    ListHashSet<IDBTransactionBackendInterface*> m_startedTransactionQueue;

Maybe this should be a refptr too...just to be safe?  If not, maybe we can put in more asserts to verify that anything in here is also in m_startedTransactions?

> WebKit/chromium/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=44700

You need a lot more changelog here (but just about the WEbKit parts)

> WebKit/chromium/public/WebIDBObjectStore.h:61
>          WEBKIT_ASSERT_NOT_REACHED();

put on same line for void methods.

> WebKit/chromium/src/IDBObjectStoreProxy.cpp:77
> +    OwnPtr<WebKit::WebIDBTransactionImpl> transPtr = adoptPtr(new WebKit::WebIDBTransactionImpl(transaction));

Noooooo

WebKit::WebIDBTransactionImpl webTransaction = static_cast<IDBTransactionBackendProxy*>(transaction)->getWebObject() (or something like that).

> WebKit/chromium/src/IDBTransactionBackendProxy.cpp:79
> +void IDBTransactionBackendProxy::step()

hmmm.....this is probably why we should stick with ids between the frontend and backend interfaces.

> WebKit/chromium/src/IDBTransactionBackendProxy.cpp:84
> +bool IDBTransactionBackendProxy::isFinished() const

lets try to get rid of these

> WebKit/chromium/src/WebIDBObjectStoreImpl.cpp:69
> +void WebIDBObjectStoreImpl::get(const WebIDBKey& key, WebIDBCallbacks* callbacks, const WebIDBTransaction& transaction)

Ok...I think I like this afterall....basically the semantics we have is pass by reference when not transfering ownership and otherwise pass by ptr

> WebKit/chromium/src/WebIDBTransactionImpl.cpp:82
> +

extra new line

> WebKit/chromium/src/WebIDBTransactionImpl.h:50
> +#if WEBKIT_IMPLEMENTATION

this is always true in src

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.