[Webkit-unassigned] [Bug 40054] [indexedDB] It is impossible to create object stores

[bugzilla-daemon at webkit.org]

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





--- Comment #18 from Andrei Popescu <andreip at google.com>  2010-06-03 08:35:05 PST ---
(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.

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