superreview requested: [Bug 813833] Promise-based SQLite interface : [Attachment 694999] Promise-based SQLite interface, v8

[bugzill...@mozilla.org]

Gregory Szorc [:gps] <g...@mozilla.com> has asked Dave Townsend (:Mossop)
<dtownsen...@oxymoronical.com> for superreview:
Bug 813833: Promise-based SQLite interface
https://bugzilla.mozilla.org/show_bug.cgi?id=813833

Attachment 694999: Promise-based SQLite interface, v8
https://bugzilla.mozilla.org/attachment.cgi?id=694999&action=edit


------- Additional Comments from Gregory Szorc [:gps] <g...@mozilla.com>
Requesting SR per Marco's suggestion.

[bugzill...@mozilla.org]

Dave Townsend (:Mossop) <dtownsen...@oxymoronical.com> has not granted
Gregory Szorc [:gps] <g...@mozilla.com>'s request for superreview:

Bug 813833: Promise-based SQLite interface
https://bugzilla.mozilla.org/show_bug.cgi?id=813833

Attachment 694999: Promise-based SQLite interface, v8
https://bugzilla.mozilla.org/attachment.cgi?id=694999&action=edit

------- Additional Comments from Dave Townsend (:Mossop)
<dtownsen...@oxymoronical.com>
Review of attachment 694999:
-----------------------------------------------------------------

The UnopenedConnection object seems very strange to me, what is the point of
it? Why not just have an Sqlite.Connection which you then call open() on, or
just Sqlite.openConnection()? I'd like to understand that before signing off on
this.

::: toolkit/modules/Sqlite.jsm
@@ +194,5 @@
> + TRANSACTION_IMMEDIATE: Ci.mozIStorageConnection.TRANSACTION_IMMEDIATE,
> + TRANSACTION_EXCLUSIVE: Ci.mozIStorageConnection.TRANSACTION_EXCLUSIVE,
> +
> + get connectionReady() {
> + return this._connection.connectionReady;

this._connection may be null, maybe you want to call _ensureOpen() first.

@@ +208,5 @@
> + *
> + * It is recommended to only use this within transactions (which are
> + * handled as sequential statements via Tasks).
> + */
> + get lastInsertRowID() {

These always struck me as a big footgun in the sqlite API. Can we make this
better and just include this information along with each statement result or is
it expensive to get?

@@ +220,5 @@
> + *
> + * The same caveats regarding asynchronous execution for
> + * `lastInsertRowID` also apply here.
> + */
> + get affectedRows() {

As above.

@@ +423,5 @@
> + /**
> + * Whether a transaction is currently in progress.
> + */
> + get transactionInProgress() {
> + return this._connection.transactionInProgress;

this._connection may be null, maybe you want to call _ensureOpen() first.

@@ +454,5 @@
> + this._ensureOpen();
> +
> + if (this.transactionInProgress) {
> + throw new Error("A transaction is already active. Only one transaction
" +
> + "can be active at a time.");

This seems unfortunate, is this a limitation in sqlite or just this jsm? Can we
add a way to wait for the transaction to end?

[bugzill...@mozilla.org]

Gregory Szorc [:gps] <g...@mozilla.com> has asked Dave Townsend (:Mossop)
<dtownsen...@oxymoronical.com> for superreview:

Bug 813833: Promise-based SQLite interface
https://bugzilla.mozilla.org/show_bug.cgi?id=813833

Attachment 697602: Promise-based SQLite interface, v9
https://bugzilla.mozilla.org/attachment.cgi?id=697602&action=edit

------- Additional Comments from Gregory Szorc [:gps] <g...@mozilla.com>

https://tbpl.mozilla.org/?tree=Try&rev=1e43e6387dff

Just a reminder that this is blocking Firefox Health Report and needs to land
in 20.

[bugzill...@mozilla.org]

Dave Townsend (:Mossop) <dtownsen...@oxymoronical.com> has not granted

Gregory Szorc [:gps] <g...@mozilla.com>'s request for superreview:

[bugzill...@mozilla.org]

Gregory Szorc [:gps] <g...@mozilla.com> has asked Dave Townsend (:Mossop)
<dtownsen...@oxymoronical.com> for superreview:

Bug 813833: Promise-based SQLite interface
https://bugzilla.mozilla.org/show_bug.cgi?id=813833

Attachment 698067: Promise-based SQLite interface, v10
https://bugzilla.mozilla.org/attachment.cgi?id=698067&action=edit

------- Additional Comments from Gregory Szorc [:gps] <g...@mozilla.com>

OpenedConnectionFactory -> openConnection (with inlined open)

[bugzill...@mozilla.org]

Dave Townsend (:Mossop) <dtownsen...@oxymoronical.com> has granted
Gregory Szorc [:gps] <g...@mozilla.com>'s request for superreview: