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

Tue Sep 21 05:32:50 PDT 2010

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

Jeremy Orlow <jorlow at chromium.org> changed:

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

--- Comment #19 from Jeremy Orlow <jorlow at chromium.org>  2010-09-21 05:32:50 PST ---
(From update of attachment 68163)
View in context: https://bugs.webkit.org/attachment.cgi?id=68163&action=review

> WebCore/storage/IDBDatabaseBackendImpl.cpp:154
> +    ASSERT_UNUSED(mode, !mode); // FIXME: Delete this.

Make it more clear that we want to delete the param, not the line of code.

> WebCore/storage/IDBDatabaseBackendInterface.h:58
> +    // FIXME: remove the default value for the transaction parameter.

Make it more clear when this will be possible.

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

FIXME: This is only necessary until ....

> WebCore/storage/IDBPendingTransactionMonitor.cpp:40
> +        m_transactions = new Vector<IDBTransactionBackendInterface*>();

NOOOOOO manual memory management!

Why make the vector like this?  at least store it in an ownPtr...but why not just make Vector a member...and do swap's if you need to?

> WebCore/storage/IDBPendingTransactionMonitor.cpp:56
> +        delete m_transactions;

NOOOOO

> WebCore/storage/IDBPendingTransactionMonitor.cpp:68
> +    delete m_transactions;

NOOOOOO

> WebCore/storage/IDBPendingTransactionMonitor.h:58
> +    static Vector<IDBTransactionBackendInterface*>* m_transactions;

monitor is in browser process, right?  if so, it should have BackendImpl pointers....or maybe even refs?

> WebCore/storage/IDBRequest.cpp:174
> +    if (m_transaction.get()) {

no .get()

> WebCore/storage/IDBRequest.cpp:180
> +        m_transaction.clear();

why clear it?  i know we clear objects in a few places, but i'm wondering if it's really worth bothering

> WebCore/storage/IDBRequest.h:97
> +    RefPtr<IDBTransactionBackendInterface> m_transaction;

store an id here...and anywhere else other than backend impls

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

as i mentioned in the other thread, i think this is the wrong error...we should use some idb error.  unknown if there isn't one specced yet (and file a spec bug)

> WebCore/storage/IDBTransactionBackendImpl.cpp:68
> +    if (m_state == IDLE)

some of your methods here can always be called (like abort) and bail if it's not in a state where that makes sense...and some like start assume you only call it when it makes sense.  Maybe you should be consistent?

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

remove newline to be consistient with method below?

> WebCore/storage/IDBTransactionBackendImpl.cpp:137
> +    m_database.clear();

is there any reason we need to do the .clear()

if not, it might be better to get rid of didFinishTransaction here and just inline the trnasaction cordinatior calls to match what you do in stuff like ::start()

> WebCore/storage/IDBTransactionBackendImpl.cpp:146
> +    queue.swap(m_taskQueue);

Why do we need to swap like this?  If stuff is added, it'll be at the end of the list...and you just keep grabbing the first item.  So I think you're ok.

Oh...but then I guess you'll need to make the above logic not reschedule another timer......so I guess this is ok.

> WebCore/storage/IDBTransactionBackendImpl.cpp:151
> +        m_pendingEvents++;

should this happen before we call performTask?  Otherwise it's possible that the task should call didCompleteTaskEvents

But, in general, this seems fairly fragile.  Can we do it any better?  Maybe keep a list of transactions with pending tasks and then call didCompleteTaskEvents at the end of the while loop on each one?

> WebCore/storage/IDBTransactionBackendImpl.h:62
> +        IDLE,

I believe this is not webkit style.  (We do it this way only when matching IDLs.)

maybe s/IDLE/Unused/ or NotStarted?

> WebCore/storage/IDBTransactionBackendImpl.h:73
>      RefPtr<DOMStringList> m_objectStoreNames;

This is a long list.  Maybe add some newlines in to logically group things together?

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

ewwww!!!!!!!

we should not pass around transaction interfaces.  Lets stick with either ids (when something needs to be passed outside of other backendImpls) or IDBTransactionBackendImpls.

note that you just went tot he effort to lookup the _interface_ object a moment ago in the proxy layer

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

dont' do this when the next instrction would crash anyway

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

not needed...you'll crash on the next line anyway

> WebCore/storage/IDBTransactionCoordinator.cpp:71
> +    m_runningTransactions.get(transaction->id())->didCompleteTaskEvents();

ewwww

we should not pass around transaction interfaces.  Lets stick with either ids (when something needs to be passed outside of other backendImpls) or IDBTransactionBackendImpls.

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

not needed

> WebCore/storage/IDBTransactionCoordinator.cpp:83
> +    // Add to started transactions list

maybe a newline above this?  whenever i see code blocks of over 4-5 instructions, i start looking at where there are natural groupings of operations and tend to split there.  maybe i'm overly nit-picky tho?

period at end

> WebCore/storage/IDBTransactionCoordinator.cpp:108
> +IDBTransactionBackendInterface* IDBTransactionCoordinator::transaction(int transactionID)

maybe transactionFromId?  I don't care much

> WebCore/storage/IDBTransactionCoordinator.cpp:125
> +    // that's ready to run. For now we only have a single running

no need to wrap at 80 chars  :-)

2 lines seems ok for somethign this long tho

> WebCore/storage/IDBTransactionCoordinator.cpp:138
> +}; 

extra space

> WebKit/chromium/public/WebIDBTransaction.h:62
> +    virtual operator WebCore::IDBTransactionBackendInterface*() const = 0;

I'm not sure we need this here....the callers should be dealing only with the impl object, right?

> WebKit/chromium/src/IDBDatabaseProxy.h:53
> +    virtual PassRefPtr<IDBObjectStoreBackendInterface> objectStore(const String& name, unsigned short mode, IDBTransactionBackendInterface* transaction = 0);

I don't think you need the default param here

> WebKit/chromium/src/IDBFactoryBackendProxy.cpp:74
> +        ids[i] = transactions.at(i)->id();

Hm.  I guess this works, but I had kinda envisioned doing this at the Chromium level....but i guess i can't come up with any disadvantages to doing it this way and it does seem simpler.  ok.

> WebKit/chromium/src/IDBObjectStoreProxy.cpp:77
> +    IDBTransactionBackendProxy* transactionProxy = static_cast<IDBTransactionBackendProxy*>(transaction);

Add a comment explaining why this is safe maybe?

> WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72
> +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>)

Lets get rid of it from the interface then.

> WebKit/chromium/src/IDBTransactionBackendProxy.h:52
> +    WebKit::WebIDBTransaction* toWebIDBTransaction() const { return m_webIDBTransaction.get(); }

this isn't really converting it, so "to" should probably not be here

> WebKit/chromium/src/WebIDBFactoryImpl.cpp:69
> +    WTF::Vector<WebCore::IDBTransactionBackendInterface*> transactions(pendingIDs.size());

just do a using namespace webocre at the top?

> WebKit/chromium/src/WebIDBTransactionImpl.h:-36
> -namespace WebCore { class IDBTransactionBackendInterface; }

I believe a using WebCore is OK to do at the top here.

> WebKit/chromium/src/WebIDBTransactionImpl.h:50
> +    virtual operator WebCore::IDBTransactionBackendInterface*() const;

needn't be virtual...maybe just make it inline?  Not sure if it makes sense to do this as an operator.  The more I deal with operators, the more I've not been super impressed by them.  Maybe just getWebCoreBackend?  webCoreBackend...or just backend()?

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