Closed Bug 793239 Opened 9 years ago Closed 8 years ago

SettingsLock should have a state property to indicate if it is still valid.

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp -

People

(Reporter: djf, Assigned: reuben)

References

Details

Attachments

(1 file, 2 obsolete files)

It would be great if the SettingsLock object had a state property with values "open" and "closed". Or just a closed boolean property.  Or some way to determine if a cached lock object is still usable.

Right now gaia's SettingsListener.js module assumes that it can do a setTimeout(, 0) and the lock will be valid until that callback fires. But there's a race condition.

I think we can work around the gaia bug with exception handling, so this is probably not a blocker.  But would be nice in the API.
blocking-basecamp: --- → ?
Currently, when I use a stale settings lock, it looks like the exception that is thrown is just a very large integer.  I suppose the API should also do something more JS friendly in this case.  A DOMError with InvalidState or something like that.
Based on comment #0, not a blocker.
blocking-basecamp: ? → -
Attached patch Patch (obsolete) — Splinter Review
Apparently it is *really* hard to what you asked in comment 1 from JS, so I didn't change the exception that is thrown.
Assignee: anygregor → reuben.bmo
Status: NEW → ASSIGNED
Attachment #702466 - Flags: review?(anygregor)
Comment on attachment 702466 [details] [diff] [review]
Patch

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

Great work!

::: dom/settings/tests/test_settings_basics.html
@@ +722,5 @@
> +      }
> +    }
> +    SimpleTest.executeSoon(cb);
> +  },
> +  function () {

Please move the previous test that deletes the database to the end.
You should also check that the lock is open in some other tests. So check that the lock is open when we create it and during a success callback.
Attachment #702466 - Flags: review?(anygregor) → review+
Attached patch Patch, v2 (obsolete) — Splinter Review
Alright, thanks! Carrying r=gwagner
Attachment #702466 - Attachment is obsolete: true
Attachment #702573 - Flags: review+
Keywords: checkin-needed
(In reply to :Ehsan Akhgari from comment #7)
> Backed out because of test failures

Thanks. I messed up when qref'ing that last set of changes :(
Will upload complete patch in a bit.
Attachment #702573 - Attachment is obsolete: true
Attachment #702631 - Flags: review+
Can you please run this through the try server?  Thanks!
The relevant test passes on Try: https://tbpl.mozilla.org/?tree=Try&rev=bcf82dfcb4fc
https://hg.mozilla.org/mozilla-central/rev/e00134f26069
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.