[webkit-reviews] review denied: [Bug 44700] IDBObjectStore::get should run in a transaction. : [Attachment 68163] Patch

Tue Sep 21 05:32:49 PDT 2010

Jeremy Orlow <jorlow at chromium.org> has denied Andrei Popescu
<andreip at google.com>'s request for review:
Bug 44700: IDBObjectStore::get should run in a transaction.
https://bugs.webkit.org/show_bug.cgi?id=44700

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

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

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()?