Bug 40054

Created attachment 57652 [details] Patch

Created attachment 57653 [details] Patch

Created attachment 57655 [details] Patch

Here's the comments I had queued up from you last patch: WebCore/WebCore.gypi:1691 + 'inspector/InspectorFrontendClientLocal.cpp', hu? WebCore/WebCore.gypi:  + 'inspector/front-end/HAREntry.js', ? WebCore/GNUmakefile.am:2522 + WebCore/storage/IDBWeakFramePtr.h \ Add to gypi, pro, and vcproj WebCore/storage/IDBDatabaseImpl.cpp:37 + IDBDatabaseImpl::IDBDatabaseImpl(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr) Passing via * is slightly better here. See http://webkit.org/coding/RefPtr.html WebCore/storage/IDBDatabaseImpl.h:40 + static PassRefPtr<IDBDatabase> create(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr) ditto WebCore/storage/IDBDatabaseRequest.cpp:52 + if (!(*m_idbDatabase->weakFramePtr())) Maybe we should add a .get()? if (!blahblah().get()) seems nicer. WebCore/storage/IDBDatabaseRequest.cpp:53 + return 0; Is this going to be handled gracefully by the caller? I doubt it. I'd create the request object either way and return it (without calling createObjectStore...though that's just an optimization...make that clear in a comment). Since we're going away, it's no big deal that neither the onsuccess nor the onerror will ever be fired. WebCore/storage/IDBDatabaseRequest.h:39 + class IDBRequest; Alpha order. WebCore/storage/IDBObjectStore.h:42 + // FIXME: This needs to be split into an interface and Impl classes. Remove WebCore/storage/IDBObjectStoreImpl.cpp:51 + RefPtr<DOMStringList> indexNames = DOMStringList::create(); I don't think we need to store a DOMStringList. Can't you just have a vector of IDBIndexImpls and query each of those for its name on demand? It doesn't seem worth the effort of keeping multiple lists up to date at once. You can do this in a future change if you'd like.

Comment on attachment 57655 [details] Patch WebKit/chromium/src/WebIDBObjectStoreImpl.h:13 + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of Get rid of #3 WebKit/chromium/src/WebIDBObjectStoreImpl.h:48 + virtual WebString keyPath() const; newline after this...also put in a fixme for anything you didn't implement (like indexNames)

Attachment 57653 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2846018

Comment on attachment 57655 [details] Patch WebKit/chromium/src/WebIDBDatabaseImpl.h:  + this newline is probably OK...if you want. WebKit/chromium/src/WebIDBDatabaseImpl.h:48 + virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks* callbacks, WebFrame*); Newline before private: WebKit/chromium/src/WebIDBCallbacksImpl.cpp:69 + m_callbacks->onSuccess(IDBDatabaseProxy::create(webKitInstance, weakFramePtr.release())); I don't really see a reason not to combine these lines. WebKit/chromium/src/IndexedDatabaseProxy.h:45 + virtual void open(const String& name, const String& description, PassRefPtr<IDBCallbacks>, PassRefPtr<SecurityOrigin>, PassRefPtr<IDBWeakFramePtr>); This could be a *IDBWeakFramePtr I believe. WebKit/chromium/src/IndexedDatabaseProxy.cpp:62 + Frame* frame = (*framePtr).get(); framePtr-> WebKit/chromium/src/IDBObjectStoreProxy.h:13 + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of Remove WebKit/chromium/src/IDBObjectStoreProxy.h:57 + private: newline before? WebKit/chromium/src/IDBObjectStoreProxy.cpp:13 + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of remove WebKit/chromium/src/IDBDatabaseProxy.h:44 + static PassRefPtr<IDBDatabase> create(PassOwnPtr<WebKit::WebIDBDatabase>, PassRefPtr<IDBWeakFramePtr>); Just pass pointers. WebCore/storage/IDBDatabaseImpl.h:60 + RefPtr<IDBWeakFramePtr> m_weakFramePtr; I don't think it makes sense for this to be here. The frame pointer is really an issue that should be entirely constrained to the request objects. WebCore/storage/IDBDatabaseRequest.h:56 + PassRefPtr<IDBRequest> createObjectStore(const String& name, const String& keyPath = "", bool autoIncrement = false); newline after WebCore/storage/IDBWeakFramePtr.h:49 + bool operator!() const {return !m_frame; } Wait...doesn't this mean you can just do |if (!weakFramePtr)| since the ref ptr overloads that operator and this overloads it as well? WebCore/storage/IndexedDatabaseRequest.h:57 + void disconnectFrame() { m_framePtr->disconnectFrame(); } I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary.

Thanks for the review Jeremy! (In reply to comment #4) > Here's the comments I had queued up from you last patch: > > > WebCore/WebCore.gypi:1691 > + 'inspector/InspectorFrontendClientLocal.cpp', > hu? > > WebCore/WebCore.gypi:  > + 'inspector/front-end/HAREntry.js', > ? > Silly, I know. Fixed. > WebCore/GNUmakefile.am:2522 > + WebCore/storage/IDBWeakFramePtr.h \ > Add to gypi, pro, and vcproj > Added. > > WebCore/storage/IDBDatabaseImpl.cpp:37 > + IDBDatabaseImpl::IDBDatabaseImpl(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr) > Passing via * is slightly better here. See http://webkit.org/coding/RefPtr.html > Agreed. Done. > WebCore/storage/IDBDatabaseImpl.h:40 > + static PassRefPtr<IDBDatabase> create(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr) > ditto > ditto > WebCore/storage/IDBDatabaseRequest.cpp:52 > + if (!(*m_idbDatabase->weakFramePtr())) > Maybe we should add a .get()? if (!blahblah().get()) seems nicer. > It already had a get() but I just preferred the other way. But ok, if you think it's nicer. > WebCore/storage/IDBDatabaseRequest.cpp:53 > + return 0; > Is this going to be handled gracefully by the caller? I doubt it. I'd create the request object either way and return it (without calling createObjectStore...though that's just an optimization...make that clear in a comment). Since we're going away, it's no big deal that neither the onsuccess nor the onerror will ever be fired. > As discussed, this seems reasonable for now. The frame is going away so I am not sure doing something else makes more sense. > WebCore/storage/IDBDatabaseRequest.h:39 > + class IDBRequest; > Alpha order. > Done. > WebCore/storage/IDBObjectStore.h:42 > + // FIXME: This needs to be split into an interface and Impl classes. > Remove > Removed. > WebCore/storage/IDBObjectStoreImpl.cpp:51 > + RefPtr<DOMStringList> indexNames = DOMStringList::create(); > I don't think we need to store a DOMStringList. Can't you just have a vector of IDBIndexImpls and query each of those for its name on demand? It doesn't seem worth the effort of keeping multiple lists up to date at once. You can do this in a future change if you'd like. Hmm, that is your code, I just moved it to the Impl class :)

(In reply to comment #5) > (From update of attachment 57655 [details]) > WebKit/chromium/src/WebIDBObjectStoreImpl.h:13 > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > Get rid of #3 > Got rid. > WebKit/chromium/src/WebIDBObjectStoreImpl.h:48 > + virtual WebString keyPath() const; > newline after this...also put in a fixme for anything you didn't implement (like indexNames) Added.

(In reply to comment #7) > (From update of attachment 57655 [details]) > WebKit/chromium/src/WebIDBDatabaseImpl.h:  > + > this newline is probably OK...if you want. > I want. > WebKit/chromium/src/WebIDBDatabaseImpl.h:48 > + virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks* callbacks, WebFrame*); > Newline before private: > Added. > WebKit/chromium/src/WebIDBCallbacksImpl.cpp:69 > + m_callbacks->onSuccess(IDBDatabaseProxy::create(webKitInstance, weakFramePtr.release())); > I don't really see a reason not to combine these lines. > Indeed, especially now that we pass a raw pointer. > WebKit/chromium/src/IndexedDatabaseProxy.h:45 > + virtual void open(const String& name, const String& description, PassRefPtr<IDBCallbacks>, PassRefPtr<SecurityOrigin>, PassRefPtr<IDBWeakFramePtr>); > This could be a *IDBWeakFramePtr I believe. > Done. > WebKit/chromium/src/IndexedDatabaseProxy.cpp:62 > + Frame* frame = (*framePtr).get(); > framePtr-> > Done. > WebKit/chromium/src/IDBObjectStoreProxy.h:13 > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > Remove > Removed. > WebKit/chromium/src/IDBObjectStoreProxy.h:57 > + private: > newline before? > Added. > WebKit/chromium/src/IDBObjectStoreProxy.cpp:13 > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > remove > Removed. > WebKit/chromium/src/IDBDatabaseProxy.h:44 > + static PassRefPtr<IDBDatabase> create(PassOwnPtr<WebKit::WebIDBDatabase>, PassRefPtr<IDBWeakFramePtr>); > Just pass pointers. > Done. > WebCore/storage/IDBDatabaseImpl.h:60 > + RefPtr<IDBWeakFramePtr> m_weakFramePtr; > I don't think it makes sense for this to be here. The frame pointer is really an issue that should be entirely constrained to the request objects. > Yes, but the IDBDatabase needs to be able to create Request objects of its own, so it need to pass the weak reference to them. > WebCore/storage/IDBDatabaseRequest.h:56 > + PassRefPtr<IDBRequest> createObjectStore(const String& name, const String& keyPath = "", bool autoIncrement = false); > newline after > Added. > WebCore/storage/IDBWeakFramePtr.h:49 > + bool operator!() const {return !m_frame; } > Wait...doesn't this mean you can just do |if (!weakFramePtr)| since the ref ptr overloads that operator and this overloads it as well? > Unfortunately not. The ! operator applies to the object, not to a pointer to an object. So when you apply it to the RefPtr, it will test its m_ptr pointer against NULL. It will not apply the ! operator to the object pointed by m_ptr. > WebCore/storage/IndexedDatabaseRequest.h:57 > + void disconnectFrame() { m_framePtr->disconnectFrame(); } > I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary. I am not sure I agree. Right now, the weak pointer is a private implementation detail of the IDB module. The entry point to that is the IndexedDatabaseRequest class so I think it is neater to let the DOMWindow only know about the IndexedDatabaseRequest instance. Perhaps we should reconsider this if we are to use the weak pointer idea in other places? Thanks, Andrei

(In reply to comment #10) > (In reply to comment #7) > > WebCore/storage/IDBDatabaseImpl.h:60 > > + RefPtr<IDBWeakFramePtr> m_weakFramePtr; > > I don't think it makes sense for this to be here. The frame pointer is really an issue that should be entirely constrained to the request objects. > > > > Yes, but the IDBDatabase needs to be able to create Request objects of its own, so it need to pass the weak reference to them. We need to figure out another way. In Chromium, these pointers don't exist in the browser process which is where the impl exists. I know you're hijacking them at the proxy layer, but it's still pretty ugly. Lets figure out a way. > > WebCore/storage/IndexedDatabaseRequest.h:57 > > + void disconnectFrame() { m_framePtr->disconnectFrame(); } > > I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary. > > I am not sure I agree. Right now, the weak pointer is a private implementation detail of the IDB module. The entry point to that is the IndexedDatabaseRequest class so I think it is neater to let the DOMWindow only know about the IndexedDatabaseRequest instance. Perhaps we should reconsider this if we are to use the weak pointer idea in other places? Is it possible for IndexedDatabaseRequest to go away before disconnectFrame is called? If so, we have a problem. If not then add an assert into ~IndexedDatabaseRequest to ensure the weak pointer is 0. That'll alleviate my worry if so.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #7) > > > WebCore/storage/IDBDatabaseImpl.h:60 > > > + RefPtr<IDBWeakFramePtr> m_weakFramePtr; > > > I don't think it makes sense for this to be here. The frame pointer is really an issue that should be entirely constrained to the request objects. > > > > > > > Yes, but the IDBDatabase needs to be able to create Request objects of its own, so it need to pass the weak reference to them. > > We need to figure out another way. In Chromium, these pointers don't exist in the browser process which is where the impl exists. I know you're hijacking them at the proxy layer, but it's still pretty ugly. Lets figure out a way. > I filed 40071 about this. > > > WebCore/storage/IndexedDatabaseRequest.h:57 > > > + void disconnectFrame() { m_framePtr->disconnectFrame(); } > > > I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary. > > > > I am not sure I agree. Right now, the weak pointer is a private implementation detail of the IDB module. The entry point to that is the IndexedDatabaseRequest class so I think it is neater to let the DOMWindow only know about the IndexedDatabaseRequest instance. Perhaps we should reconsider this if we are to use the weak pointer idea in other places? > > Is it possible for IndexedDatabaseRequest to go away before disconnectFrame is called? If so, we have a problem. If not then add an assert into ~IndexedDatabaseRequest to ensure the weak pointer is 0. That'll alleviate my worry if so. Ok, I will add the assert.

Andrei, I'm sorry for not realizing this sooner, but all we need to do for IndexedDB is annotate all of our function that return an IDBRequest object with [CallWith=ScriptExecutionContext] and then pass that into the IDBRequest when we're creating it. It's already an ActiveDOMObject, so it knows when the script execution context goes away. I'm not sure why this didn't occur to me sooner. Sorry for the run around. :-( For IndexedDatabaseRequest, instead of passing the Frame* on instantiation, we should probably try to derive the frame from the scriptExecutionContext (see if it's a document, and if so cast...otherwise ASSERT and put a FIXME: Make this work within workers) and pass that in instead. This can be done in a subsequent patch if you'd rather, though it's easy enough you might just want to do it here?

Created attachment 57760 [details] Patch

Created attachment 57762 [details] Patch

Comment on attachment 57762 [details] Patch WebCore/ChangeLog:12 + * Android.mk: This list will need to be updated before you land. WebCore/bindings/scripts/CodeGeneratorV8.pm:2537 + $result .= $indent . "if (!scriptContext)\n"; In what cases do we expect this to be possible? WebCore/page/DOMWindow.cpp:475 + m_indexedDatabaseRequest = 0; Is this necessary? I can't think of why it would be. WebCore/storage/IDBDatabaseImpl.cpp:56 + callbacks->onSuccess(objectStore.release()); You need to keep a list of object stores and add it to that. And if it's already there, return an error. At least add a FIXME now. WebCore/storage/IDBDatabaseRequest.cpp:52 + // FIXME: make this work with workers. I think just an assert is fine. Sure it'll crash and burn badly if we're in release mode, but it'd be hard for someone to accidentally enable in workers. :-) WebCore/storage/IDBDatabaseRequest.cpp:58 + return 0; I think you should still return the IDBRequest and just never call onsuccess/onerror since it's going away anyway. Let's not create weird bugs for developers that are impossible to debug. WebCore/storage/IDBObjectStoreImpl.h:45 + String name() const { return m_name; } const Sring& WebCore/storage/IDBObjectStoreImpl.h:46 + String keyPath() const { return m_keyPath; } ditto WebCore/storage/IDBObjectStoreImpl.h:59 + typedef HashMap<String, RefPtr<IDBIndex> > IndexMap; maybe a newline before typedef? WebCore/storage/IDBRequest.h:71 + // FIXME: Have one onSuccess function for each possible result type. I think we can remove this now...should be obvious. WebCore/storage/IndexedDatabaseRequest.cpp:56 + if (!context->isDocument()) { same things here as above. WebKit/chromium/public/WebIDBDatabase.h:62 + virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks*, WebFrame*) This doesn't need the frame. Only IndexedDatabase::open since we'll need to check the content settings. WebKit/chromium/public/WebIDBObjectStore.h:13 + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of delete WebKit/chromium/src/IDBObjectStoreProxy.cpp:66 + return 0; Assert not implemented WebKit/chromium/src/IDBObjectStoreProxy.cpp:71 + // FIXME: implement. Why not do this now? Just add it to the Web interface and redirect...no sense in doing part of the plumbing but not all of it. Close...I should probably do one more scan through though.

Nate, can you please look at the code generator part of this?

(In reply to comment #16) > (From update of attachment 57762 [details]) > WebCore/ChangeLog:12 > + * Android.mk: > This list will need to be updated before you land. > Will do. > WebCore/bindings/scripts/CodeGeneratorV8.pm:2537 > + $result .= $indent . "if (!scriptContext)\n"; > In what cases do we expect this to be possible? > Not entirely sure, but that is the pattern elsewhere in the bindings where the ScriptExecutionContext is needed. > WebCore/page/DOMWindow.cpp:475 > + m_indexedDatabaseRequest = 0; > Is this necessary? I can't think of why it would be. > But DOMWindow::clear() sets all other ref pointers to 0, so the expectation is that all DOMWindow objects are no longer accessible once clear() is called. Why would indexed database be an exception? > WebCore/storage/IDBDatabaseImpl.cpp:56 > + callbacks->onSuccess(objectStore.release()); > You need to keep a list of object stores and add it to that. And if it's already there, return an error. At least add a FIXME now. > Ah yes, will add. > WebCore/storage/IDBDatabaseRequest.cpp:52 > + // FIXME: make this work with workers. > I think just an assert is fine. Sure it'll crash and burn badly if we're in release mode, but it'd be hard for someone to accidentally enable in workers. :-) Ok. > > WebCore/storage/IDBDatabaseRequest.cpp:58 > + return 0; > I think you should still return the IDBRequest and just never call onsuccess/onerror since it's going away anyway. Let's not create weird bugs for developers that are impossible to debug. > Hmm, but we discussed that yesterday and agreed that returning null is better than making it look as if the call succeeded but then failing to fire any of the events. Either way, this should be in the spec. > WebCore/storage/IDBObjectStoreImpl.h:45 > + String name() const { return m_name; } > const Sring& > > WebCore/storage/IDBObjectStoreImpl.h:46 > + String keyPath() const { return m_keyPath; } > ditto > > WebCore/storage/IDBObjectStoreImpl.h:59 > + typedef HashMap<String, RefPtr<IDBIndex> > IndexMap; > maybe a newline before typedef? > > WebCore/storage/IDBRequest.h:71 > + // FIXME: Have one onSuccess function for each possible result type. > I think we can remove this now...should be obvious. > Will fix all of the above. > WebCore/storage/IndexedDatabaseRequest.cpp:56 > + if (!context->isDocument()) { > same things here as above. > > WebKit/chromium/public/WebIDBDatabase.h:62 > + virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks*, WebFrame*) > This doesn't need the frame. Only IndexedDatabase::open since we'll need to check the content settings. > Ok, I could not remember what exactly were we going to do with the WebFrame, so I left it in. Will remove it. > WebKit/chromium/public/WebIDBObjectStore.h:13 > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > delete > > WebKit/chromium/src/IDBObjectStoreProxy.cpp:66 > + return 0; > Assert not implemented > > WebKit/chromium/src/IDBObjectStoreProxy.cpp:71 > + // FIXME: implement. > Why not do this now? Just add it to the Web interface and redirect...no sense in doing part of the plumbing but not all of it. > This CL was about implementing only IDBDatabase::createObjectStore(). I promise to fix all the plumbing but it's easier for me to do so in smaller CLs rather than all at once. > > Close...I should probably do one more scan through though. Thanks for the review, will send a new patch asap.

(In reply to comment #17) > Nate, can you please look at the code generator part of this? LGTM

Created attachment 57882 [details] Patch

Comment on attachment 57882 [details] Patch WebCore/ChangeLog:12 + * Android.mk: regenerate list WebCore/storage/IndexedDatabaseRequest.cpp:57 + // FIXME: make this work with workers. ASSERT_NOT_REACHED() WebCore/storage/IndexedDatabaseRequest.cpp:63 + return 0; Didn't we agree to return an IDBRequest that'll just never fire? WebCore/storage/IndexedDatabaseRequest.cpp:61 + Document* doc = static_cast<Document*>(context); don't abbreviate WebKit/chromium/public/WebIDBDatabase.h:64 + WEBKIT_ASSERT_NOT_REACHED(); Might be slightly better to assign the WebIDBCallbacks to an OwnPtr so you don't leak and to make it obvious to whomever implements this that it's an owning relationship...but this code is going away soon anyway so I don't care much.

r=me with those changes

(In reply to comment #22) > r=me with those changes Thanks! I'll do the changes and land it manually.

Committed r60776