Bug 39602 - Add IDBDatabase's attributes
Summary: Add IDBDatabase's attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-24 10:05 PDT by Jeremy Orlow
Modified: 2010-05-26 04:45 PDT (History)
3 users (show)

See Also:


Attachments
Patch (29.07 KB, patch)
2010-05-25 07:58 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (36.92 KB, patch)
2010-05-26 03:31 PDT, Jeremy Orlow
steveblock: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2010-05-24 10:05:04 PDT
Add IDBDatabase's attributes
Comment 1 Jeremy Orlow 2010-05-25 07:58:30 PDT
Created attachment 57019 [details]
Patch
Comment 2 Jeremy Orlow 2010-05-25 08:03:16 PDT
+ darin for WebKit API review
+ andrei for informal review of the whole thing
Comment 3 Darin Fisher (:fishd, Google) 2010-05-25 08:04:21 PDT
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.
Comment 4 Jeremy Orlow 2010-05-25 08:12:55 PDT
(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?
Comment 5 Andrei Popescu 2010-05-25 08:46:56 PDT
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?
Comment 6 Darin Fisher (:fishd, Google) 2010-05-25 09:15:54 PDT
> > 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.
Comment 7 Jeremy Orlow 2010-05-26 03:31:59 PDT
Created attachment 57086 [details]
Patch
Comment 8 Andrei Popescu 2010-05-26 03:39:29 PDT
LGTM. Steve should have a look, too.
Comment 9 Steve Block 2010-05-26 04:29:09 PDT
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?
Comment 10 Jeremy Orlow 2010-05-26 04:34:19 PDT
(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.
Comment 11 Jeremy Orlow 2010-05-26 04:35:06 PDT
(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.
Comment 12 Steve Block 2010-05-26 04:37:49 PDT
(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
Comment 13 Jeremy Orlow 2010-05-26 04:43:53 PDT
Committed r60230: <http://trac.webkit.org/changeset/60230>
Comment 14 Jeremy Orlow 2010-05-26 04:45:54 PDT
(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.