Closed
Bug 1233628
Opened 9 years ago
Closed 9 years ago
handling {} for clear-origin-data in PushService.jsm
Categories
(Core :: DOM: Push Subscriptions, defect)
Core
DOM: Push Subscriptions
Tracking
()
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
firefox46 | --- | affected |
People
(Reporter: allstars.chh, Assigned: allstars.chh)
References
Details
Attachments
(1 file)
3.19 KB,
patch
|
lina
:
feedback+
|
Details | Diff | Splinter Review |
See Honza's comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1233136#c0
"PushService [1], bug 1170115, that seems to do the right thing, except that empty aData string is simply ignored (the code doesn't delete anything)"
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8699852 -
Flags: review?(kcambridge)
Comment 3•9 years ago
|
||
Comment on attachment 8699852 [details] [diff] [review]
Patch
Review of attachment 8699852 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the patch! r=me with the two issues fixed.
::: dom/push/PushService.jsm
@@ -295,1 @@
> let pattern = JSON.parse(data);
This doesn't seem quite right. `JSON.parse("")` will throw a syntax error. How about `data ? JSON.parse(data) : {}`?
::: dom/push/test/xpcshell/test_clear_origin_data.js
@@ +8,5 @@
> const userAgentID = 'bd744428-f125-436a-b6d0-dd0c9845837f';
>
> let clearForPattern = Task.async(function* (testRecords, pattern) {
> let patternString = JSON.stringify(pattern);
> + yield PushService.observe(null, 'clear-origin-data', patternString);
Please change `observe` to return the promise. Otherwise, this might race.
Attachment #8699852 -
Flags: review?(kcambridge) → feedback+
Assignee | ||
Comment 4•9 years ago
|
||
Oh your're right, I made a mistake, I thought JSON.stringify("{}") will become "" but it should be "{}", so the original code should work already, I'll go back to bug 1233136 to confirm with Honza again.
Thanks
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•