IndexedDB: integer version layout tests

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: integer version layout tests

[David Grogan]

Created attachment 155086 [details]
Patch

[David Grogan]

Created attachment 155087 [details]
Patch

[David Grogan]

Created attachment 155088 [details]
Patch

[David Grogan]

Josh and Alec, could you take a look at this?

[Joshua Bell]

Comment on attachment 155088 [details]
Patch

Didn't finish going through, but initial feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=155088&action=review

> LayoutTests/storage/indexeddb/resources/intversion-abort-in-initial-upgradeneeded.js:11
> +    dbname = self.location.pathname.substring(1 + self.location.pathname.lastIndexOf("/"));

This is getting a bit long to type and fragile to copy/paste, how about a helper in shared.js, e.g. setDBNameFromTestPath() ? It could log something pretty to the output.

> LayoutTests/storage/indexeddb/resources/intversion-abort-in-initial-upgradeneeded.js:21
> +    request.onupgradeneeded = upgradeNeeded;

Add onblocked handler? (Your suggestion elsewhere - I think we want to do that in general going forward so we get TEXT fail w/ diff rather than TIMEOUT.)

Ditto for the other tests.

> LayoutTests/storage/indexeddb/resources/intversion-and-setversion.js:49
> +    db = trans.db;

Is this line needed?

> LayoutTests/storage/indexeddb/resources/intversion-and-setversion.js:59
> +    db = event.target.db;

Or this one? If I understand things correctly, db should have remained the same object the whole time.

> LayoutTests/storage/indexeddb/resources/intversion-blocked.js:25
> +      event = evt;

Should be indented 2 more spaces.

> LayoutTests/storage/indexeddb/resources/intversion-blocked.js:71
> +    evalAndLog("gotBlockedEvent = true");

I don't know if we do anywhere else, but in one of these tests it wouldn't hurt to say: shouldBe("event.target", "connection1");

> LayoutTests/storage/indexeddb/resources/intversion-close-between-events.js:37
> +    transaction.oncomplete = function(e)

In other tests, we don't include the e parameter if not used, and { on the same line as function keyword for anonymous functions. I'm not sure we're totally consistent, though.

> LayoutTests/storage/indexeddb/resources/intversion-close-between-events.js:56
> +function openSuccess(evt)

Do we know what the state of db should be here? To test if db is closed we can call db.transaction() on it (it will raise if db is closed).

> LayoutTests/storage/indexeddb/resources/intversion-close-between-events.js:66
> +      finishJSTest();

Should be indented two more spaces.

> LayoutTests/storage/indexeddb/resources/intversion-close-in-oncomplete.js:33
> +    evalAndLog("db.createObjectStore(\"os\")");

For readability, use single quotes instead of escaped quotes?

[Joshua Bell]

Comment on attachment 155088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155088&action=review

> LayoutTests/storage/indexeddb/resources/intversion-high-version-first.js:6
> +description("Test that if two open requests are waiting at the same time, the one with the higher version gets called first");

Wow... the consequences of this are non-intuitive!

> LayoutTests/storage/indexeddb/resources/intversion-high-version-first.js:45
> +    request2.onsuccess = successCallback2;

... so 2 succeeds because 1 closes promptly, but 3 and 4 get queued and 4 wins, 3 is blocked?

I would have expected that all 3 are treated equally, 4 wins, and 2 and 3 get VersionError.

> LayoutTests/storage/indexeddb/resources/intversion-open-with-version.js:26
> +    shouldBeEqualToString("Object.prototype.toString.apply(request)", "[object IDBOpenDBRequest]");

Other tests use String(foo) rather than the ...apply() call. While I'm a pedantic fan of the latter, the former is easier to read. :)

[David Grogan]

Comment on attachment 155088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155088&action=review

>> LayoutTests/storage/indexeddb/resources/intversion-abort-in-initial-upgradeneeded.js:11
>> +    dbname = self.location.pathname.substring(1 + self.location.pathname.lastIndexOf("/"));
> 
> This is getting a bit long to type and fragile to copy/paste, how about a helper in shared.js, e.g. setDBNameFromTestPath() ? It could log something pretty to the output.

Added.  Also added preamble() to shared.js.

>> LayoutTests/storage/indexeddb/resources/intversion-abort-in-initial-upgradeneeded.js:21
>> +    request.onupgradeneeded = upgradeNeeded;
> 
> Add onblocked handler? (Your suggestion elsewhere - I think we want to do that in general going forward so we get TEXT fail w/ diff rather than TIMEOUT.)
> 
> Ditto for the other tests.

Done, I think everywhere.

>> LayoutTests/storage/indexeddb/resources/intversion-and-setversion.js:49
>> +    db = trans.db;
> 
> Is this line needed?

Not strictly needed, but I like to retrieve all the genericly named objects in the function so as to be resilient against changes in the other earlier function.  If I intend to reuse an object later in a different function I try to either declare it in the global scope or give it an explicit name, a la connection1.

What do you do?

>> LayoutTests/storage/indexeddb/resources/intversion-blocked.js:25
>> +      event = evt;
> 
> Should be indented 2 more spaces.

Moved to new function.

>> LayoutTests/storage/indexeddb/resources/intversion-blocked.js:71
>> +    evalAndLog("gotBlockedEvent = true");
> 
> I don't know if we do anywhere else, but in one of these tests it wouldn't hurt to say: shouldBe("event.target", "connection1");

Added to versionChangeHandler above.

>> LayoutTests/storage/indexeddb/resources/intversion-close-between-events.js:37
>> +    transaction.oncomplete = function(e)
> 
> In other tests, we don't include the e parameter if not used, and { on the same line as function keyword for anonymous functions. I'm not sure we're totally consistent, though.

Fixed.

>> LayoutTests/storage/indexeddb/resources/intversion-close-between-events.js:56
>> +function openSuccess(evt)
> 
> Do we know what the state of db should be here? To test if db is closed we can call db.transaction() on it (it will raise if db is closed).

It should be open, since db.close hasn't been called yet.  Added a transaction line.

>> LayoutTests/storage/indexeddb/resources/intversion-close-between-events.js:66
>> +      finishJSTest();
> 
> Should be indented two more spaces.

Thanks.  I need to get vim to autoindent 4 spaces if the file matches third_party/WebKit.

>> LayoutTests/storage/indexeddb/resources/intversion-close-in-oncomplete.js:33
>> +    evalAndLog("db.createObjectStore(\"os\")");
> 
> For readability, use single quotes instead of escaped quotes?

Done.

>> LayoutTests/storage/indexeddb/resources/intversion-high-version-first.js:6
>> +description("Test that if two open requests are waiting at the same time, the one with the higher version gets called first");
> 
> Wow... the consequences of this are non-intuitive!

Yeah, I'm not sure I'm reading the spec correctly either.  It could use some clarification on this point.  I intend to file a bug.

>> LayoutTests/storage/indexeddb/resources/intversion-high-version-first.js:45
>> +    request2.onsuccess = successCallback2;
> 
> ... so 2 succeeds because 1 closes promptly, but 3 and 4 get queued and 4 wins, 3 is blocked?
> 
> I would have expected that all 3 are treated equally, 4 wins, and 2 and 3 get VersionError.

Now that I'm re-reading this, I feel like you're right.  Either way, we don't do it right yet.  Firefox fires upgradeneeded at 2 then 3 then 4, so I don't think they have figured it out either.

>> LayoutTests/storage/indexeddb/resources/intversion-open-with-version.js:26
>> +    shouldBeEqualToString("Object.prototype.toString.apply(request)", "[object IDBOpenDBRequest]");
> 
> Other tests use String(foo) rather than the ...apply() call. While I'm a pedantic fan of the latter, the former is easier to read. :)

Ah yes, I switched to String(foo) halfway through these tests.

[David Grogan]

Created attachment 155343 [details]
Patch

[David Grogan]

Created attachment 155360 [details]
Patch

[David Grogan]

Created attachment 155622 [details]
Patch

[David Grogan]

Created attachment 155623 [details]
Patch

[David Grogan]

Removed intversion-high-version-first (for now) because it causes the new code to permanently stay in m_runningVersionChangeTransaction, causing the next test to timeout.

[David Grogan]

Created attachment 155662 [details]
Patch

[David Grogan]

Created attachment 155674 [details]
Patch

[David Grogan]

Tony, could you review this?

I'm going to land a series of patches in the coming days (hopefully by friday!) that will enable some of this functionality incrementally.  It'd be nice to land these all at once and then just include updated expected files with the code as necessary.

Josh didn't LGTM here but he did say it looked good in an offline thread.

[David Grogan]

Nate,

Tony, our usual reviewer, is OOO the next couple of days.  Could you review this?

[David Grogan]

Created attachment 155885 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155885 [details]
Patch for landing

Clearing flags on attachment: 155885

Committed r124383: <http://trac.webkit.org/changeset/124383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

*** Bug 84309 has been marked as a duplicate of this bug. ***