Dartifying some IndexedDB APIs.

Dartifying some IndexedDB APIs.

BUG=

Committed: https://code.google.com/p/dart/source/detail?r=18338

[nweiz, 2013-02-05 02:03:18]

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
File sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart (right):

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:348: {int version, void
onUpgradeNeeded(VersionChangeEvent)}) {
It's probably worthwhile to add some code that throws an ArgumentError if
version is supplied without onUpgradeNeeded or vice versa.

Also, doesn't this have an onBlocked callback as well?

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:362: return new
Future.immediateError(e);
Catch and report the stack trace.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:410: Future
_completeRequest(Request request, Completer completer) {
Why does this take an external completer? It seems like it should just create
and return one itself.

Add a TODO here to make the completer explicitly synchronous.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:553: Future<Object>
put(value, [key]) {
Why "Future<Object>" rather than just "Future"?

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:585: * will cause the
transaction to automatically be committed- all processing
Nit: "committed- all" -> "comitted--all"

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:587: * the current
transaction.
I would make this warning more urgent and scary, maybe include some bold text.
Nothing else in Dart's async model works like this, and it's going to be very
easy to write code that fails in mysterious ways.

Including examples of code that works and code that fails would also be useful
here.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:759: var controller = new
StreamController<Cursor>();
Add a TODO to use an explicitly synchronous controller.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:762:
controller.signalError(e);
Add a TODO here to include a stack trace once issue 4061 is fixed.

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart
File tests/html/utils.dart (right):

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart#new...
tests/html/utils.dart:90: guardAsync(() {
guardAsync shouldn't be necessary if you're using expect(..., completes).

[blois, 2013-02-05 03:02:42]

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
File sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart (right):

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:348: {int version, void
onUpgradeNeeded(VersionChangeEvent)}) {
On 2013/02/05 02:03:19, nweiz wrote:
> It's probably worthwhile to add some code that throws an ArgumentError if
> version is supplied without onUpgradeNeeded or vice versa.
> 
> Also, doesn't this have an onBlocked callback as well?

True- my initial reading was that it was only for deleteDatabase calls.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:362: return new
Future.immediateError(e);
On 2013/02/05 02:03:19, nweiz wrote:
> Catch and report the stack trace.

Done.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:410: Future
_completeRequest(Request request, Completer completer) {
On 2013/02/05 02:03:19, nweiz wrote:
> Why does this take an external completer? It seems like it should just create
> and return one itself.
> 
> Add a TODO here to make the completer explicitly synchronous.

I made it take one so it would be typed properly. Not necessary, but thought it
could be helpful when people are debugging stuff.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:553: Future<Object>
put(value, [key]) {
On 2013/02/05 02:03:19, nweiz wrote:
> Why "Future<Object>" rather than just "Future"?

Removed, but it was a preference to be explicit as the result is key which is
defined as an object.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:587: * the current
transaction.
On 2013/02/05 02:03:19, nweiz wrote:
> I would make this warning more urgent and scary, maybe include some bold text.
> Nothing else in Dart's async model works like this, and it's going to be very
> easy to write code that fails in mysterious ways.
> 
> Including examples of code that works and code that fails would also be useful
> here.

Done.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:759: var controller = new
StreamController<Cursor>();
On 2013/02/05 02:03:19, nweiz wrote:
> Add a TODO to use an explicitly synchronous controller.

Done.

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:762:
controller.signalError(e);
On 2013/02/05 02:03:19, nweiz wrote:
> Add a TODO here to include a stack trace once issue 4061 is fixed.

Done.

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart
File tests/html/utils.dart (right):

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart#new...
tests/html/utils.dart:90: guardAsync(() {
On 2013/02/05 02:03:19, nweiz wrote:
> guardAsync shouldn't be necessary if you're using expect(..., completes).

The error messages that I get from exceptions thrown from within the methods is
quite a bit better with guardAsync. Tests fail all the same, but the info is
better with guardAsync.

[nweiz, 2013-02-05 03:05:39]

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
File sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart (right):

https://codereview.chromium.org/12159005/diff/13001/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:410: Future
_completeRequest(Request request, Completer completer) {
On 2013/02/05 03:02:42, blois wrote:
> On 2013/02/05 02:03:19, nweiz wrote:
> > Why does this take an external completer? It seems like it should just
create
> > and return one itself.
> > 
> > Add a TODO here to make the completer explicitly synchronous.
> 
> I made it take one so it would be typed properly. Not necessary, but thought
it
> could be helpful when people are debugging stuff.

Trying to get Futures typed correctly is a losing battle, since as soon as any
chaining is done all typing just goes out the window.

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart
File tests/html/utils.dart (right):

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart#new...
tests/html/utils.dart:90: guardAsync(() {
On 2013/02/05 03:02:42, blois wrote:
> On 2013/02/05 02:03:19, nweiz wrote:
> > guardAsync shouldn't be necessary if you're using expect(..., completes).
> 
> The error messages that I get from exceptions thrown from within the methods
is
> quite a bit better with guardAsync. Tests fail all the same, but the info is
> better with guardAsync.

How so? What's different about it?

[blois, 2013-02-06 23:34:13]

Updated with additional cleanup & fixes for latest Chrome bits (see
Transaction.completed future).

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart
File tests/html/utils.dart (right):

https://codereview.chromium.org/12159005/diff/13001/tests/html/utils.dart#new...
tests/html/utils.dart:90: guardAsync(() {
On 2013/02/05 03:05:39, nweiz wrote:
> On 2013/02/05 03:02:42, blois wrote:
> > On 2013/02/05 02:03:19, nweiz wrote:
> > > guardAsync shouldn't be necessary if you're using expect(..., completes).
> > 
> > The error messages that I get from exceptions thrown from within the methods
> is
> > quite a bit better with guardAsync. Tests fail all the same, but the info is
> > better with guardAsync.
> 
> How so? What's different about it?

Resolution: issues unrelated to this code (unclosed DBs), fixed the core issue.

[nweiz, 2013-02-07 20:38:10]

lgtm

https://codereview.chromium.org/12159005/diff/13002/sdk/lib/indexed_db/dart2j...
File sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart (right):

https://codereview.chromium.org/12159005/diff/13002/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:352: 'version specified but
not onUpgradeNeeded'));
This is inaccurate if onUpgradeNeeded is specified but not version.

https://codereview.chromium.org/12159005/diff/13002/tests/html/indexeddb_3_te...
File tests/html/indexeddb_3_test.dart (right):

https://codereview.chromium.org/12159005/diff/13002/tests/html/indexeddb_3_te...
tests/html/indexeddb_3_test.dart:75: var cursors =
objectStore.openCursor(direction:'prev').asBroadcastStream();
Style nit: space after ":"

[sra1, 2013-02-08 02:30:28]

lgtm

https://chromiumcodereview.appspot.com/12159005/diff/13002/tests/html/indexed...
File tests/html/indexeddb_1_test.dart (right):

https://chromiumcodereview.appspot.com/12159005/diff/13002/tests/html/indexed...
tests/html/indexeddb_1_test.dart:17: return chainSteps([
This does not seem much of a win over an explicit .then chain

html.window.indexedDB.deleteDatabase(dbName)
  .then(
    (_) {
      return html.window.indexedDB.open(dbName, version: 1,
          onUpgradeNeeded: (e) {});
    })
  .then(
    (db) {
      db.close();
    })
  .then(
    (_) {
      return html.window.indexedDB.open(dbName, version: 2,
          onUpgradeNeeded: (e) {
            // Bug 8265, we're getting the wrong type here.
            //expect(e.oldVersion, 1);
            //expect(e.newVersion, 2);
            upgraded = true;
          });
      })
  .then(
    (_db) {
      expect(upgraded, isTrue);
    });

[blois, 2013-02-08 22:18:56]

https://codereview.chromium.org/12159005/diff/13002/sdk/lib/indexed_db/dart2j...
File sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart (right):

https://codereview.chromium.org/12159005/diff/13002/sdk/lib/indexed_db/dart2j...
sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:352: 'version specified but
not onUpgradeNeeded'));
On 2013/02/07 20:38:10, nweiz wrote:
> This is inaccurate if onUpgradeNeeded is specified but not version.

Done.

https://codereview.chromium.org/12159005/diff/13002/tests/html/indexeddb_1_te...
File tests/html/indexeddb_1_test.dart (right):

https://codereview.chromium.org/12159005/diff/13002/tests/html/indexeddb_1_te...
tests/html/indexeddb_1_test.dart:17: return chainSteps([
On 2013/02/08 02:30:28, sra1 wrote:
> This does not seem much of a win over an explicit .then chain
> 
> html.window.indexedDB.deleteDatabase(dbName)
>   .then(
>     (_) {
>       return html.window.indexedDB.open(dbName, version: 1,
>           onUpgradeNeeded: (e) {});
>     })
>   .then(
>     (db) {
>       db.close();
>     })
>   .then(
>     (_) {
>       return html.window.indexedDB.open(dbName, version: 2,
>           onUpgradeNeeded: (e) {
>             // Bug 8265, we're getting the wrong type here.
>             //expect(e.oldVersion, 1);
>             //expect(e.newVersion, 2);
>             upgraded = true;
>           });
>       })
>   .then(
>     (_db) {
>       expect(upgraded, isTrue);
>     });

True- when I had the guardAsync around the methods in chain it eliminated a good
amount of overhead, but no more. Gone.

https://codereview.chromium.org/12159005/diff/13002/tests/html/indexeddb_3_te...
File tests/html/indexeddb_3_test.dart (right):

https://codereview.chromium.org/12159005/diff/13002/tests/html/indexeddb_3_te...
tests/html/indexeddb_3_test.dart:75: var cursors =
objectStore.openCursor(direction:'prev').asBroadcastStream();
On 2013/02/07 20:38:10, nweiz wrote:
> Style nit: space after ":"

Done.