Closed
Bug 793239
Opened 13 years ago
Closed 12 years ago
SettingsLock should have a state property to indicate if it is still valid.
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: djf, Assigned: reuben)
References
Details
Attachments
(1 file, 2 obsolete files)
4.49 KB,
patch
|
reuben
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•13 years ago
|
blocking-basecamp: --- → ?
Reporter | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
Alright, thanks! Carrying r=gwagner
Attachment #702466 -
Attachment is obsolete: true
Attachment #702573 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 6•12 years ago
|
||
Keywords: checkin-needed
Comment 7•12 years ago
|
||
Backed out because of test failures:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9ca4e8812ed1
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=89942aa8ae6a
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #702573 -
Attachment is obsolete: true
Attachment #702631 -
Flags: review+
Comment 10•12 years ago
|
||
Can you please run this through the try server? Thanks!
Assignee | ||
Comment 11•12 years ago
|
||
The relevant test passes on Try: https://tbpl.mozilla.org/?tree=Try&rev=bcf82dfcb4fc
Comment 12•12 years ago
|
||
Comment 13•12 years ago
|
||
Status: ASSIGNED → 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.
Description
•