836542 - B2G SMS: Merge SmsDatabaseService.js and IndexedDBHelper.jsm IndexedDB wrapping code

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Reuben Morais [:reuben]]

See ensureDb, newTxn, upgradeSchema, etc in:

https://mxr.mozilla.org/mozilla-central/source/dom/sms/src/ril/SmsDatabaseService.js
https://mxr.mozilla.org/mozilla-central/source/dom/base/IndexedDBHelper.jsm

Comment 1

[Reuben Morais [:reuben]]

Taking because why not.

Comment 2

[Reuben Morais [:reuben]]

Attachment: Merge IDBHelper and MMDBService

Looking green on Try: https://tbpl.mozilla.org/?tree=Try&rev=582dedf62c9e

Comment 3

[Gene Lian (I already quit Mozilla)]

Cool stuff!

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 749204 [details] [diff] [review]
Merge IDBHelper and MMDBService

Review of attachment 749204 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +141,5 @@
>      if (!storeNames) {
>        storeNames = [MESSAGE_STORE_NAME];
>      }
>      if (DEBUG) debug("Opening transaction for object stores: " + storeNames);
> +    this.newTxn(txn_type, storeNames[0], function(txn) {

I don't think this will work because it looks like an endless recursive call.  The try you attached don't include Marionette tests as well, so I'm verifying it locally.

Comment 5

[Reuben Morais [:reuben]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> I don't think this will work because it looks like an endless recursive
> call.  The try you attached don't include Marionette tests as well, so I'm
> verifying it locally.

Ah yes, JavaScript :) I think I can do IndexedDBHelper.prototype.newTxn.call(this, … ?

Comment 6

[Gene Lian (I already quit Mozilla)]

Btw, where do you call .initDBHelper(...) to initialize the IndexedDBHelper?

Comment 7

[Reuben Morais [:reuben]]

You are both right, of course, and that's what I get for writing patches at 3AM :)

I'm spinning an updated build of the emulator so I can run the tests, I didn't realize they wouldn't run as part of the emulator mochitests.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Reuben Morais [:reuben] from comment #7)
> You are both right, of course, and that's what I get for writing patches at 3AM :)
> 
> I'm spinning an updated build of the emulator so I can run the tests, I
> didn't realize they wouldn't run as part of the emulator mochitests.

Thank you! That's really appreciated :) Just takes a little bit more polishment.

Comment 9

[Reuben Morais [:reuben]]

I sent an updated version to try with Marionette tests, and it times out without giving much information: https://tbpl.mozilla.org/?tree=Try&rev=acda99a6d9d4

Vicamo, how do I run the tests in dom/mobilemessage/tests/marionette/ locally? I tried all sorts of incantations but couldn't get the tests to actually run.

Comment 10

[Reuben Morais [:reuben]]

Vicamo, can you provide any hints here? I'm pretty sure by now I have tried every possible combination of every possible tool, following 3 different sets of instructions, none of which successfully ran any of the tests in dom/mobilemessage/tests/marionette/, or dom/mobilemessage/tests/test_smsdatabaseservice.xul.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

In Marionette full long, you got an exception on startup:

08:51:23  WARNING -  E/GeckoConsole(  239): [JavaScript Error: "property name upgradeSchema appears more than once in object literal" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 248 column: 2 source: "  },
08:51:23     INFO -  E/GeckoConsole(  239): "}]
08:51:23     INFO -  E/GeckoConsole(  239): While creating services from category 'profile-after-change', could not create service for entry 'MobileMessageDatabaseService', contract ID '@mozilla.org/mobilemessage/rilmobilemessagedatabaseservice;1'

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Reuben Morais [:reuben] from comment #10)
> Vicamo, can you provide any hints here? I'm pretty sure by now I have tried
> every possible combination of every possible tool, following 3 different
> sets of instructions, none of which successfully ran any of the tests in
> dom/mobilemessage/tests/marionette/, or
> dom/mobilemessage/tests/test_smsdatabaseservice.xul.

For marionette, you should be able to run it locally with |./test.sh marionette|.  This will run all marionette test cases for B2G, and if you want test cases for mobilemessage only, try |./test.sh marionette `pwd`/gecko/dom/mobilemessage/tests/marionette/manifest.ini| instead.

For xpcshell, you can have |./test.sh xpcshell|.  I have no quick answer for mochitest because for now.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Steal bug from Reuben.  WIP: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/836542/websms-use-IndexedDBHelper

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/5: correct type of callback aRv argument

1) For nsIRilMobileMessageDatabaseCallback, the first argument |aRv| is a nsresult; for nsIRilMobileMessageDatabaseRecordCallback, it's actually an unsigned short integer.

2) For this second argument of nsIRilMobileMessageDatabaseCallback, a DOM {Sms,Mms}Message, in some cases we may try to create a message instance from an invalid message record.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/5: separate database/service code

Move some pure database code into a separate MobileMessageDB.jsm so that we can finally have some test on it.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/5: allow muliple stores in IndexedDBHelper.newTxn()

In MobileMessageDB/Service, some operations take place between multiple objects stores.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.a/5: use IndexedDBHelper

Really use IndexedDBHelper.  Accommodations to IndexedDBHelper upgradeSchema(), newTxn() are in latter patches.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.b/5: remove ensureDB and refactor upgradeSchemaN

IndexedDBHelper has its own private ensureDB call, drop it.  Extract schema upgrade process and refactor it.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.c/5: remove newTxn and fix init()

IndexedDBHelper has its own nexTxn().  However, its interface is quite different from that in MobileMessageDB.  I'm going to adopt the new interface and fix all the calls in latter patches.  In this patch, I fix the one in init().  We have nothing to do in both transaction oncomplete and onabort event.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.d/5: fix saveMessageRecord()

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.e/5: fix updatePendingTransactionToError()

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.f/5: fix getMessageRecordByTransactionId()

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.g/5: fix getMessageRecordById()

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.h/5: fix deleteMessage()

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.iA/5: bail-out early in updateMessageDeliveryById()

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.iB/5: fix updateMessageDeliveryById()

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.j/5: fix markMessageRead()

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.k/5: fix createThreadCursor()

We can't simply calls |aTransaction.abort()| in |getThreadTxn()| because it might be handled in wrong |ontxnabort| event handler.

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.l/5: fix createMessageCursor()

We don't need an |onerror| event handler in |filterIndex| because it will be handled in |ontxnabort|.

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

test cases WIP.  All existing mobilemessage marionette test cases passed.

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5/5: mochitest : WIP

Comment 33

[Reuben Morais [:reuben]]

Comment on attachment 799638 [details] [diff] [review]
part 3/5: allow muliple stores in IndexedDBHelper.newTxn()

Review of attachment 799638 [details] [diff] [review]:
-----------------------------------------------------------------

Nice :)

Comment 34

[Reuben Morais [:reuben]]

Comment on attachment 799638 [details] [diff] [review]
part 3/5: allow muliple stores in IndexedDBHelper.newTxn()

Review of attachment 799638 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/IndexedDBHelper.jsm
@@ +112,5 @@
>    newTxn: function newTxn(txn_type, store_name, callback, successCb, failureCb) {
>      this.ensureDB(function () {
>        if (DEBUG) debug("Starting new transaction" + txn_type);
>        let txn = this._db.transaction(this.dbStoreNames, txn_type);
> +

Extra whitespace.

Comment 35

[Gene Lian (I already quit Mozilla)]

Comment on attachment 799629 [details] [diff] [review]
part 1/5: correct type of callback aRv argument

Review of attachment 799629 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +138,5 @@
> +      return;
> +    }
> +
> +    let domMessage;
> +    if (aRv == Cr.NS_OK) {

I'd be better to change it to be:

if (aRv == Cr.NS_OK && aMessageRecord != null)

Or handle the null case within the createDomMessageFromRecord().

@@ +1093,4 @@
>      this.newTxn(READ_WRITE, function(error, txn, stores) {
>        if (error) {
>          // TODO bug 832140 check event.target.errorCode
> +        aCallback(Cr.NS_ERROR_FAILURE, null);

This change looks a bit dangerous to me. In the original notifyResult(rv), it always return a non-null domMessage by:

let domMessage = self.createDomMessageFromRecord(aMessageRecord);

ALso, please see how the RadioInterfaceLayer.js uses saveSendingMessage/saveReceivedMessage for example. The caller site seems to be considering the returned domMessage to be a non-null one.

-----
After some thoughts, it seems the original notifyResult(rv) doesn't properly handle the aMessageRecord either. Hmm... I think your patch also fix that. Good!

The caller site shouldn't use the domMessage (null) if the returned result is not Cr.NS_OK. This could be another bug.

@@ +1413,5 @@
>      }
>      aMessage.deliveryIndex = [aMessage.delivery, timestamp];
>  
> +    return this.saveRecord(aMessage, threadParticipants,
> +                           this.notifyRilCallback.bind(this, aCallback));

Can you just input aCallback and call:

self.notifyRilCallback(aCallback, ...);

in the saveRecord()? which sounds more straight forward to me.

@@ +1470,5 @@
>        addresses = aMessage.receivers;
>      }
> +
> +    return this.saveRecord(aMessage, addresses,
> +                           this.notifyRilCallback.bind(this, aCallback));

Ditto.

Comment 36

[Gene Lian (I already quit Mozilla)]

Comment on attachment 799643 [details] [diff] [review]
part 4.a/5: use IndexedDBHelper

Review of attachment 799643 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +69,5 @@
>    lastMessageId: 0,
>  
>    init: function init(aDbName, aDbVersion, aGlobal) {
>      if (DEBUG) debug("init: '" + aDbName + "', " + aDbVersion);
>      IDB_GLOBAL = aGlobal;

Do you still need IDB_GLOBAL if you already have self.dbGlobal to use?

@@ +72,5 @@
>      if (DEBUG) debug("init: '" + aDbName + "', " + aDbVersion);
>      IDB_GLOBAL = aGlobal;
> +    this.initDBHelper(aDbName, aDbVersion,
> +                      [ MESSAGE_STORE_NAME, THREAD_STORE_NAME,
> +                        PARTICIPANT_STORE_NAME ], aGlobal);

You don't need spaces around "[]" which follows the style within this file. Also, I'd prefer:

this.initDBHelper(aDbName, aDbVersion,
                  [MESSAGE_STORE_NAME,
                   THREAD_STORE_NAME,
                   PARTICIPANT_STORE_NAME],
                  aGlobal);

or

let storeNames/stores = [MESSAGE_STORE_NAME, THREAD_STORE_NAME, PARTICIPANT_STORE_NAME];
this.initDBHelper(aDbName, aDbVersion, storeNames/stores, aGlobal);

Comment 37

[Gene Lian (I already quit Mozilla)]

Comment on attachment 799646 [details] [diff] [review]
part 4.b/5: remove ensureDB and refactor upgradeSchemaN

Review of attachment 799646 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +168,3 @@
>      }
>  
> +    aTransaction.abort();

It will never reach here. Right? Please dump an error debug message here.

Comment 38

[Gene Lian (I already quit Mozilla)]

Comment on attachment 799647 [details] [diff] [review]
part 4.c/5: remove newTxn and fix init()

Review of attachment 799647 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +76,5 @@
>                          PARTICIPANT_STORE_NAME ], aGlobal);
>  
>      let that = this;
> +    this.newTxn(READ_ONLY, MESSAGE_STORE_NAME,
> +                function ontxncallback(aTransaction, aMessageStore) {

We often use either onXXX or XXXCb, instead of onXXXCb, so txnCallback sounds better.

@@ +92,5 @@
>          }
>          that.lastMessageId = cursor.key || 0;
>          if (DEBUG) debug("Last assigned message ID was " + that.lastMessageId);
>        };
> +    }, null, function ontxnabort(aErrorName) {

Can you s/ontxnabort/txnFailureCb to follow the parameter name of IndexedDBHelper.newTxn(..., failureCb)?

Comment 39

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (national holidays until 9/23) from comment #38)
> Can you s/ontxnabort/txnFailureCb to follow the parameter name of
> IndexedDBHelper.newTxn(..., failureCb)?

After some thoughts, I think it's fine to keep the ontxnabort since the implementation actually uses .abort().

Comment 40

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (national holidays until 9/23) from comment #38)
> We often use either onXXX or XXXCb, instead of onXXXCb, so txnCallback
                                                             ^^^^^^^^^^^
> sounds better.

or just txncallback.

Comment 41

[Gene Lian (I already quit Mozilla)]

Comment on attachment 799649 [details] [diff] [review]
part 4.d/5: fix saveMessageRecord()

Review of attachment 799649 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +921,5 @@
>      let self = this;
> +    this.newTxn(READ_WRITE,
> +                [MESSAGE_STORE_NAME, PARTICIPANT_STORE_NAME, THREAD_STORE_NAME],
> +                function ontxncallback(aTransaction, aMessageStore,
> +                                       aParticipantStore, aThreadStore) {

Are you sure it's not:

function ontxncallback(aTransaction, stores)

where the stores is an array?

s/ontxncallback/txncallback

Comment 42

[Gene Lian (I already quit Mozilla)]

Per off-line discussion with Vicamo, we decided to postpone the reviews a bit.

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Looks like we need a test case for each function of the mmdb, not only part of them.  So moving only a few functions into a new jsm will not fit this requirement.  Should just keep this bug simple and land the helper merging part as soon as possible.

Comment 44

[BMO Automation]

Firefox OS is not being worked on