Closed Bug 968215 Opened 8 years ago Closed 8 years ago

SettingsService needs to have a callback when transaction are completed

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

x86
macOS
defect
Not set
normal

Tracking

(b2g-v2.0 fixed)

RESOLVED FIXED
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: sjochimek, Assigned: sjochimek)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 2 obsolete files)

We should be able to execute a callback when we are sure that transactions are completed.
Attached patch Patch gecko (obsolete) — Splinter Review
Attachment #8371503 - Flags: review?(bent.mozilla)
Comment on attachment 8371503 [details] [diff] [review]
Patch gecko

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

::: dom/interfaces/settings/nsISettingsService.idl
@@ +14,5 @@
> +[scriptable, uuid(f1b3d820-8e75-11e3-baa8-0800200c9a66)]
> +interface nsISettingsTransactionCompleteCallback : nsISupports
> +{
> +  void handle();
> +  void handleAbort(in DOMString aErrorMessage);

Hm. This won't really work. The indexedDB transaction.onabort() function is called with a DOM event argument (an "abort" event), not an error message string. In SettingsService you currently just have this:

  lock._transaction.onabort = aCallback.handleAbort;

That will end up calling the callback with the wrong kind of argument.

You could theoretically do this:

  transaction.onabort = function(event) {
    var message = event.target.error.message;
    handleAbort(message);
  };

Or you could just make this a function with no arguments and skip the message altogether. I doubt the message is going to be that useful anyway.
Attachment #8371503 - Flags: review?(bent.mozilla)
Assignee: nobody → sjochimek
Attached patch Gecko patch (obsolete) — Splinter Review
Attachment #8371503 - Attachment is obsolete: true
Attachment #8372167 - Flags: review?(bent.mozilla)
Comment on attachment 8372167 [details] [diff] [review]
Gecko patch

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

This looks great, thanks!

::: dom/settings/SettingsService.js
@@ +140,5 @@
>            lock._transaction = lock._settingsService._settingsDB._db.transaction(SETTINGSSTORE_NAME, "readwrite");
> +          if (aCallback) {
> +            lock._transaction.oncomplete = aCallback.handle;
> +            lock._transaction.onabort = function(event) {
> +              var message = '';

Nit: It looks like 'let' is preferred here instead of 'var', so please change it.
Attachment #8372167 - Flags: review?(bent.mozilla) → review+
Attached patch Gecko patchSplinter Review
thanks ben!
Attachment #8372167 - Attachment is obsolete: true
Attachment #8372191 - Flags: review+
Keywords: checkin-needed
Whiteboard: [systemsfe]
https://hg.mozilla.org/mozilla-central/rev/b53ca5954168
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
You need to log in before you can comment on or make changes to this bug.