Bug 43744 – Beginnings of IndexedDB persistance + IDBDatabase.description fleshed out

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Beginnings of IndexedDB persistance + IDBDatabase.description fleshed out

[Jeremy Orlow]

Created attachment 63927 [details]
Patch

[WebKit Review Bot]

Attachment 63927 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3746002

[Marcus Bulach]

exciting stuff! some general drive-by comments:

41 function thirdSuccess() {
   
brackets on the next line here and all above
 
   
2         request = indexedDB.open("another", "test of the description attribute");   
there's a bunch of places with this string, perhaps declare it as a global?
var test_description = "test of the description attribute";
   
54     String description() const { return m_description; }
I thought we were not caching anything yet?

39 static bool extractMetaData
as for the table name and metadataQuery below, s/D/d/

60 static bool setMetaData
ditto

64     sqliteDatabase->executeCommand("DELETE FROM MetaData");
well, as long as it's consistent, change it either way.. :)

66     SQLiteStatement insert(*sqliteDatabase, "INSERT INTO MetaData VALUES (?, ?, ?)");
I'm not sure what's the SQLite dialect, but if possible, it's safer to name the columns rather than positional arguments:
INSERT INTO Metada (name, description, version) VALUES (?, ?, ?)


72     insert.bindText(1, name);
getColumnText is 0-based, but bindText is 1-based, how convenient. :)

81     // FIXME: Should we assert there's only one row?
should it just assert that the call above:
  64     sqliteDatabase->executeCommand("DELETE FROM MetaData");
returns true?

31         [CallWith=ScriptExecutionContext] IDBRequest open(in DOMString name, in [Optional,ConvertUndefinedOrNullToNullString] DOMString description);
add a space:
s/Optional,/Optional, /

35 #include "SQLiteDatabase.h"
36 #include "SecurityOrigin.h"
sort order

52 static PassOwnPtr<SQLiteDatabase> openSQLiteDatabase
we surely need to sanitize this function..

81     for (const char** cur = commands; *cur; ++cur) {
is there an ARRAYSIZE or some variation of it?

[Jeremy Orlow]

(In reply to comment #3)
> exciting stuff! some general drive-by comments:
> 
> 41 function thirdSuccess() {
> 
> brackets on the next line here and all above

done
 
> 2         request = indexedDB.open("another", "test of the description attribute");   
> there's a bunch of places with this string, perhaps declare it as a global?
> var test_description = "test of the description attribute";

I don't really see the point.  I mean this test, like most, is kind of crappy/contrived code.  I don't really know what I'd name it.  After all, it's just one of many test_descriptions, so that doesn't seem very accurate.  To be honest, I think it's more clear as is.
 
> 54     String description() const { return m_description; }
> I thought we were not caching anything yet?

This is specced to stay the value it was when the DB was initialized.  So this is actually for correctness, not an optimization.
 
> 39 static bool extractMetaData
> as for the table name and metadataQuery below, s/D/d/
> 
> 60 static bool setMetaData
> ditto

Now consistent.
 
> 64     sqliteDatabase->executeCommand("DELETE FROM MetaData");
> well, as long as it's consistent, change it either way.. :)
> 
> 66     SQLiteStatement insert(*sqliteDatabase, "INSERT INTO MetaData VALUES (?, ?, ?)");
> I'm not sure what's the SQLite dialect, but if possible, it's safer to name the columns rather than positional arguments:
> INSERT INTO Metada (name, description, version) VALUES (?, ?, ?)

k

> 72     insert.bindText(1, name);
> getColumnText is 0-based, but bindText is 1-based, how convenient. :)

Yes.  Quite lovely.

> 81     // FIXME: Should we assert there's only one row?
> should it just assert that the call above:

Hu?

>   64     sqliteDatabase->executeCommand("DELETE FROM MetaData");
> returns true?

We don't really care whether it succeeds.
 
> 31         [CallWith=ScriptExecutionContext] IDBRequest open(in DOMString name, in [Optional,ConvertUndefinedOrNullToNullString] DOMString description);
> add a space:
> s/Optional,/Optional, /

done

> 35 #include "SQLiteDatabase.h"
> 36 #include "SecurityOrigin.h"
> sort order

Q comes before e in ASCII
 
> 52 static PassOwnPtr<SQLiteDatabase> openSQLiteDatabase
> we surely need to sanitize this function..

Good point...prob need this before landing.

> 81     for (const char** cur = commands; *cur; ++cur) {
> is there an ARRAYSIZE or some variation of it?

I think it's only in certain platform layers, but I'm not sure why.  Is there any other code that establishes another way of doing this that's standard?

[Jeremy Orlow]

(In reply to comment #4)
> > 52 static PassOwnPtr<SQLiteDatabase> openSQLiteDatabase
> > we surely need to sanitize this function..
> 
> Good point...prob need this before landing.

Changed the host encoding in security origin to be used for this.
 
> > 81     for (const char** cur = commands; *cur; ++cur) {
> > is there an ARRAYSIZE or some variation of it?
> 
> I think it's only in certain platform layers, but I'm not sure why.  Is there any other code that establishes another way of doing this that's standard?

arraysize seems to work.  switched it to use that

[Jeremy Orlow]

Hmm...before landing I guess I should also un-hard-code /tmp/test.  :-)

[Andrei Popescu]

 WebCore/manual-tests/indexed-database.html
>  document.getElementById("description").innerHTML = "<font color

Wow, font tag! Make it a marquee element instead? :)

> WebCore/storage/IDBDatabaseBackendImpl.cpp
>  extractMetaData(SQLiteDatabase* sqliteDatabase, const String& expectedName, String& foundDescription, String& foundVersion)

Hmm, what is the WK style wrt reference arguments? foundDescription/Version are output params so should they be pointers instead? That's what the Google styleguide recommends and I think it's a good recommendation.


> String foundDescription = "";
>   bool result = extractMetaData(m_sqliteDatabase.get(), m_name, foundDescription, m_version);
>     m_description = description.isNull() ? foundDescription : description;
> 
>     if (!result || m_description != foundDescription)
>         setMetaData(m_sqliteDatabase.get(), m_name, m_description, m_version);

This transforms null strings to empty strings but that's not what the spec says. Add a FIXME.

[Jeremy Orlow]

(In reply to comment #7)
>  WebCore/manual-tests/indexed-database.html
> >  document.getElementById("description").innerHTML = "<font color
> 
> Wow, font tag! Make it a marquee element instead? :)
> 
> > WebCore/storage/IDBDatabaseBackendImpl.cpp
> >  extractMetaData(SQLiteDatabase* sqliteDatabase, const String& expectedName, String& foundDescription, String& foundVersion)
> 
> Hmm, what is the WK style wrt reference arguments? foundDescription/Version are output params so should they be pointers instead? That's what the Google styleguide recommends and I think it's a good recommendation.

I believe what I did is the proper style within WebKit.
 
> > String foundDescription = "";
> >   bool result = extractMetaData(m_sqliteDatabase.get(), m_name, foundDescription, m_version);
> >     m_description = description.isNull() ? foundDescription : description;
> > 
> >     if (!result || m_description != foundDescription)
> >         setMetaData(m_sqliteDatabase.get(), m_name, m_description, m_version);
> 
> This transforms null strings to empty strings but that's not what the spec says. Add a FIXME.

The spec is in flux.  Will do.

[Jeremy Orlow]

Created attachment 64011 [details]
Patch

[WebKit Review Bot]

Attachment 64011 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3771024

[Steve Block]

Comment on attachment 64011 [details]
Patch

WebCore/storage/IDBFactoryBackendImpl.cpp:101
 +      // FIXME: Everything from now on should be done on another thread.
Shouldn't the call to setDescription be on another thread too, as it calls setMetaData() ?

WebCore/page/SecurityOrigin.h:161
 +      static String encodeForFileName(const String&);
I'm not sure this should be part of SecurityOrigin - it has little to do with origins or security!

WebCore/manual-tests/indexed-database.html:41
 +          indexedDB.open("another", "xyz");
It would be good to test that having re-opened these databases with new descriptions, JavaScript doesn't see the new descriptions until the page is reloaded.

WebCore/manual-tests/indexed-database.html:49
 +          } else if (window.anotherDB.description != "test of the description attribute") {
This test looks wrong, at least for the use pattern described at the top

WebCore/manual-tests/indexed-database.html:47
 +              // Since we passed in nothing, the description should not be reset.
This comment looks wrong

WebCore/manual-tests/indexed-database.html:13
 +  <p>Now click <a href="javascript:updateDescription()">here</a>, close the browser, come back, and click <a href="javascript:readDescription()">here</a>.  If everything worked well, this should be a success here: <span id=description>...</span></p>
It looks like this page suffers from race conditions. If the open() commands in the inline script are still executing when the user starts clicking these links, the test won't proceed as expected.

WebCore/manual-tests/indexed-database.html:13
 +  <p>Now click <a href="javascript:updateDescription()">here</a>, close the browser, come back, and click <a href="javascript:readDescription()">here</a>.  If everything worked well, this should be a success here: <span id=description>...</span></p>
Is it better to use onclick, rather than the javascript protocol?

[Jeremy Orlow]

(In reply to comment #11)
> (From update of attachment 64011 [details])
> WebCore/storage/IDBFactoryBackendImpl.cpp:101
>  +      // FIXME: Everything from now on should be done on another thread.
> Shouldn't the call to setDescription be on another thread too, as it calls setMetaData() ?

Yes, but I think that'll be an implementation detail in that class.

This is all a work in progress.  I could add a million fixmes, but I've tried to use my judgement to place in the most important ones.  If you think it's confusing, I can take the existing one out.

> WebCore/page/SecurityOrigin.h:161
>  +      static String encodeForFileName(const String&);
> I'm not sure this should be part of SecurityOrigin - it has little to do with origins or security!

It has everything to do with security and the original use of it was for origins.  I agree it's probably not the right place, but I also can't think of any better place.  Can you?  Maybe in WTF somewhere?

> WebCore/manual-tests/indexed-database.html:41
>  +          indexedDB.open("another", "xyz");
> It would be good to test that having re-opened these databases with new descriptions, JavaScript doesn't see the new descriptions until the page is reloaded.

That's exactly what this test does.

> WebCore/manual-tests/indexed-database.html:49
>  +          } else if (window.anotherDB.description != "test of the description attribute") {
> This test looks wrong, at least for the use pattern described at the top

It's using another DB to test 2 different things.  What's wrong about it?

> WebCore/manual-tests/indexed-database.html:47
>  +              // Since we passed in nothing, the description should not be reset.
> This comment looks wrong

It's correct.

> WebCore/manual-tests/indexed-database.html:13
>  +  <p>Now click <a href="javascript:updateDescription()">here</a>, close the browser, come back, and click <a href="javascript:readDescription()">here</a>.  If everything worked well, this should be a success here: <span id=description>...</span></p>
> It looks like this page suffers from race conditions. If the open() commands in the inline script are still executing when the user starts clicking these links, the test won't proceed as expected.

True.  I'll disable the links until the page is ready.

> WebCore/manual-tests/indexed-database.html:13
>  +  <p>Now click <a href="javascript:updateDescription()">here</a>, close the browser, come back, and click <a href="javascript:readDescription()">here</a>.  If everything worked well, this should be a success here: <span id=description>...</span></p>
> Is it better to use onclick, rather than the javascript protocol?

I don't know, but does it really matter?

[Steve Block]

WebCore/manual-tests/indexed-database.html:5
 +  <p>This page opens up a database with the name "name" and a description of "description".  Result of open: <span id=result>Pending...</span></p>
Is this correct?

> > WebCore/page/SecurityOrigin.h:161
> >  +      static String encodeForFileName(const String&);
> > I'm not sure this should be part of SecurityOrigin - it has little to do with origins or security!
> It has everything to do with security and the original use of it was for
> origins.  I agree it's probably not the right place, but I also can't think
> of any better place.  Can you?  Maybe in WTF somewhere?
Yes but this method now has little to do with the fact it's a SecurityOrigin. WTF sounds like the right place, but I'm not sure where exactly.

> > WebCore/manual-tests/indexed-database.html:41
> >  +          indexedDB.open("another", "xyz");
> > It would be good to test that having re-opened these databases with new descriptions, JavaScript doesn't see the new descriptions until the page is reloaded.
> That's exactly what this test does.
I meant testing that the old database object doesn't have the new description, but I guess you're testing that in the layout test.

[Jeremy Orlow]

(In reply to comment #13)
> WebCore/manual-tests/indexed-database.html:5
>  +  <p>This page opens up a database with the name "name" and a description of "description".  Result of open: <span id=result>Pending...</span></p>
> Is this correct?

Ha.  Not anymore.  Will make it more vague.

> > > WebCore/page/SecurityOrigin.h:161
> > >  +      static String encodeForFileName(const String&);
> > > I'm not sure this should be part of SecurityOrigin - it has little to do with origins or security!
> > It has everything to do with security and the original use of it was for
> > origins.  I agree it's probably not the right place, but I also can't think
> > of any better place.  Can you?  Maybe in WTF somewhere?
> Yes but this method now has little to do with the fact it's a SecurityOrigin. WTF sounds like the right place, but I'm not sure where exactly.

Didn't see a good place in WTF...maybe in WebCore/platform?  I don't see a good file there either, so maybe I'll make one called "FileUtils.h"?  Or I could put it in FileSystem.h?

> > > WebCore/manual-tests/indexed-database.html:41
> > >  +          indexedDB.open("another", "xyz");
> > > It would be good to test that having re-opened these databases with new descriptions, JavaScript doesn't see the new descriptions until the page is reloaded.
> > That's exactly what this test does.
> I meant testing that the old database object doesn't have the new description, but I guess you're testing that in the layout test.

Got it.

[Jeremy Orlow]

Created attachment 64117 [details]
Patch

[Early Warning System Bot]

Attachment 64117 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3775054

[Jeremy Orlow]

Created attachment 64119 [details]
Patch

[Jeremy Orlow]

I think we've settled on what the spec should say and it's different from this, but since it's almost done being reviewed, I think it might be easiest to submit it as is and then do another patch to make it match the new prescribed behavior.  (It's mostly just changes to the test that'll happen.)

[Steve Block]

Comment on attachment 64119 [details]
Patch

WebCore/WebCore.xcodeproj/project.pbxproj: 
 +  			developmentRegion = English;
I don't think this should be removed

[Jeremy Orlow]

Committed r65509: <http://trac.webkit.org/changeset/65509>