Closed Bug 827499 Opened 12 years ago Closed 12 years ago

SettingsService: Fix locking problem

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: gwagner, Assigned: gwagner)

Details

Attachments

(1 file)

      No description provided.
Attached patch patchSplinter Review
Assignee: nobody → anygregor
Attachment #698840 - Flags: review?(bent.mozilla)
blocking-basecamp: --- → ?
Not blocking, but please request approval for this and we'll approve the patch.
blocking-basecamp: ? → -
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
Attachment #698840 - Flags: approval-mozilla-b2g18?
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.
Attachment #698840 - Flags: review?(bent.mozilla) → review+
Attachment #698840 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/mozilla-central/rev/d82ffb79c40f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: