827499 - SettingsService: Fix locking problem

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Johnny Stenback (:jst)]

Not blocking, but please request approval for this and we'll approve the patch.

Comment 3

[Gregor Wagner [:gwagner]]

Comment on attachment 698840 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 821814
User impact if declined: random functionality breaks
Testing completed: yes
Risk to taking this patch (and alternatives if risky): minor
String or UUID changes made by this patch: none

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 698840 [details] [diff] [review]
patch

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

::: dom/settings/SettingsService.js
@@ +56,5 @@
>          case "set":
>            let value = info.value;
>            let message = info.message;
> +          if(typeof(value) == 'object') {
> +            if (DEBUG) debug("object name:" + name + ", val: " + JSON.stringify(value));

Nit: Let's combine those ifs

@@ +72,3 @@
>              }
> +            let obj = { settingName: name, defaultValue: defaultValue, userValue: value };
> +            let setReq = store.put({ settingName: name, defaultValue: defaultValue, userValue: value });

Nit: no need for 'obj'

@@ +83,5 @@
>                  value: value,
>                  message: message
>                }));
>                lock._open = false;
> +              if (!lock._requests.isEmpty()) {

Nit: no need to check empty

@@ +105,5 @@
>          case "get":
> +          let getReq = store.mozGetAll(name);
> +          getReq.onsuccess = function(event) {
> +            if (DEBUG) debug("Request successful. Record count:" + event.target.result.length);
> +            if (DEBUG) debug("result: " + JSON.stringify(event.target.result));

Nit: combine into one if check

@@ +130,5 @@
>            break;
>        }
>      }
>      if (!lock._requests.isEmpty())
>        throw Components.results.NS_ERROR_ABORT;

Need to remove this now.

Comment 5

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d82ffb79c40f

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d82ffb79c40f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/88e81a47ec02