Add IDBDatabase's attributes
Created attachment 57019 [details] Patch
+ darin for WebKit API review + andrei for informal review of the whole thing
Comment on attachment 57019 [details] Patch WebKit/chromium/public/WebIDBDatabase.h:43 + // FIXME: These should be purely virtual. no need for this FIXME. it is helpful for API method to have default implementations. that way it is easier to change the API. WebKit/chromium/src/IDBDatabaseProxy.cpp:72 + WebKit::WebVector<WebKit::WebString> webStrings = m_webIDBDatabase->objectStores(); would it be helpful to define a WebStringList class that works like WebNodeList? that way the cost of copying the strings out can be deferred to the embedder.
(In reply to comment #3) > (From update of attachment 57019 [details]) > WebKit/chromium/public/WebIDBDatabase.h:43 > + // FIXME: These should be purely virtual. > no need for this FIXME. it is helpful for API method to have default > implementations. that way it is easier to change the API. In the DOMStorage code, I've only added these in as long as needed in order to get the patches landed. I'm happy to leave them in forever, but I feel like the extra effort to keep things clean (and ensure we're properly implementing all the interfaces) is good. If you're all right with it, I'd like to try and keep this stuff purely virtual....at least until someone else depends on the Chromium API and so we need to keep backwards compat code in longer. > > WebKit/chromium/src/IDBDatabaseProxy.cpp:72 > + WebKit::WebVector<WebKit::WebString> webStrings = m_webIDBDatabase->objectStores(); > would it be helpful to define a WebStringList class that works like WebNodeList? > that way the cost of copying the strings out can be deferred to the embedder. That's probably a good idea. Can I do it in a subsequent patch please?
Looks good, I have two comments: 1. I think you should leave a FIXME in the IDBDatabaseRequest IDL file, to make it clear that it's incomplete. Right now, all the methods are missing. 2. Jonas' draft spec drops the 'modifyDatabase' param from the IndexedDatabaseRequest::open() method. https://docs.google.com/a/google.com/View?id=dfs2skx2_4g3s5f857 I think we should then drop it too, as it's not entirely clear what is it for. And, as discussed, we should be implementing his draft anyway, right?
> > WebKit/chromium/src/IDBDatabaseProxy.cpp:72 > > + WebKit::WebVector<WebKit::WebString> webStrings = m_webIDBDatabase->objectStores(); > > would it be helpful to define a WebStringList class that works like WebNodeList? > > that way the cost of copying the strings out can be deferred to the embedder. > > That's probably a good idea. Can I do it in a subsequent patch please? Yes, certainly.
Created attachment 57086 [details] Patch
LGTM. Steve should have a look, too.
Comment on attachment 57086 [details] Patch The class names are very confusing! Is there a description of what they all mean somewhere? WebCore/storage/IDBDatabaseImpl.cpp:50 + } Why the empty impl?
(In reply to comment #9) > (From update of attachment 57086 [details]) > The class names are very confusing! Is there a description of what they all mean somewhere? > > WebCore/storage/IDBDatabaseImpl.cpp:50 > + } > Why the empty impl? I can avoid including DOMSecurityList.h in the header. As the file gets fleshed out, I'm sure there'll be more stuff that we can just forward declare rather than define.
(In reply to comment #9) > (From update of attachment 57086 [details]) > The class names are very confusing! Is there a description of what they all mean somewhere? We're working to change the names in the spec. When this happens, we'll rename them here.
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 57086 [details] [details]) > > The class names are very confusing! Is there a description of what they all mean somewhere? > > > > WebCore/storage/IDBDatabaseImpl.cpp:50 > > + } > > Why the empty impl? > > I can avoid including DOMSecurityList.h in the header. I meant the empty destructor
Committed r60230: <http://trac.webkit.org/changeset/60230>
(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #9) > > > (From update of attachment 57086 [details] [details] [details]) > > > The class names are very confusing! Is there a description of what they all mean somewhere? > > > > > > WebCore/storage/IDBDatabaseImpl.cpp:50 > > > + } > > > Why the empty impl? > > > > I can avoid including DOMSecurityList.h in the header. > I meant the empty destructor I know. If I made the destructor inline (even implicitly inline) then all children destructors would also need to be available. It's viral and pretty soon you're including the world just to have a destructor in some cases. So making the destructor explicit, putting it in your .cpp file, and just forward declaring most stuff in the .h instead of including other .h files is a common pattern.