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

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Jeremy Orlow <jorlow at chromium.org>  2010-09-22 10:24:20 PST ---
(In reply to comment #21)
> hanks for the review. All fixed except:
> 
> (In reply to comment #19)
> > 
> > > 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?
> > 
> 
> No, it's in the renderer! Remember we went through all this when we added the pending transaction monitor. Eventually this will live in TLS but for now it is static. And since it's static, we need to maintain the vector's lifetime manually. I am not sure how an OwnPtr can help.

Oh yeah.  I remember now.  The reason we can't use an own ptr is because we can't have objects with constructors the global scope.  Maybe we could make a static function with an own ptr that owns this though?  That would be replaced by some TLS based solution in the future.


> > > 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
> > 
> 
> We don't need the transaction anymore after this but the request object will only be released later when GC happens. I was therefore opting for clearing the pointer so that we don't needlessly keep the transaction object alive.

Actually it does seem like transactions could introduce some hairy cycles. Probably a good idea.


> > > 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)
> > 
> 
> Ok, moved to use IDBDatabaseException::NO_SUPPORTED_ERR, which is a clone of the standard DOM one. I wonder why we have to copy standard DOM exception codes in our spec?

Please start a mailing list thread on this.  Do we have issues with needing strings then?  (If so, let's do this in another patch maybe?)  Do we test this case in the layout tests?


> > > WebCore/storage/IDBTransactionBackendImpl.cpp:151
> > > +        m_pendingEvents++;
> > 
> > should this happen before we call performTask?  Otherwise it's possible that the task should call didCompleteTaskEvents
> >
> 
> That should not happen synchronously. The events complete asynchronously.

I still think you should move it.


> > > 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?
> > 
> 
> We're passing a WebIDBTransaction const ref to the WebIDBObjectStore::get() and such. The get() method needs to pass an IDBTransactionBackendInterface ptr to IDBObjectStoreBackendImpl::get(). It therefore needs to obtain an IDBTransactionBackendInterface from a WebIDBTransaction. This operator achieves that.

Gotcha....well see my other comment for the operator stuff you added.  I feel as though a virtual method would be much more clear.


> > > 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
> > 
> 
> We're overriding a method from the interface. I think we do?

99% sure we don't and shouldn't.


> > 
> > > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72
> > > +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>)
> > 
> > Lets get rid of it from the interface then.
> >
> 
> If we do, then we must allow the ObjectStore backend impls to use transaction backend impls. So far, we got away with using only interfaces everywhere. Maybe it's ok to leave as is?

We have many cases where backend impls hold references to other backend impls.  I don't see what's different here.  I'm pretty sure the plumbing should be trivial.

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