[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 #8 from Andrei Popescu <andreip at google.com>  2010-06-02 09:49:24 PST ---
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 :)

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