[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 #23 from Andrei Popescu <andreip at google.com>  2010-09-22 16:37:22 PST ---
(In reply to comment #22)
> (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.
> 

Hmm, let's chat about this tomorrow. Still not sure it's a great improvement.

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

Ok, will do,

> > > 
> > > > 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.


But the object store backend cannot possibly hold a reference to the transaction backend, since multiple transactions can be associated with the same object store backend at the same time.  So the transaction needs to be stored in the frontend object and passed into the  object store's methods as needed.

However, the frontend can only store transaction interface pointers, as Chromium uses proxy objects in the renderer. And since it's the frontend object that supplies the transaction parameter to the object store's backend methods, it follows that these methods can only get an interface pointer and not an impl pointer. Or perhaps I am missing something?

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