Bug 39602 – Add IDBDatabase's attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Add IDBDatabase's attributes

[Jeremy Orlow]

Created attachment 57019 [details]
Patch

[Jeremy Orlow]

+ darin for WebKit API review
+ andrei for informal review of the whole thing

[Darin Fisher (:fishd, Google)]

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.

[Jeremy Orlow]

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

[Andrei Popescu]

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?

[Darin Fisher (:fishd, Google)]

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

[Jeremy Orlow]

Created attachment 57086 [details]
Patch

[Andrei Popescu]

LGTM. Steve should have a look, too.

[Steve Block]

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?

[Jeremy Orlow]

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

[Jeremy Orlow]

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

[Steve Block]

(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

[Jeremy Orlow]

Committed r60230: <http://trac.webkit.org/changeset/60230>

[Jeremy Orlow]

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