Closed Bug 820301 Opened 10 years ago Closed 10 years ago

Assert that ensurePersistentDataLoaded is not called on PrivateDownloadsData

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: Paolo, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 1 obsolete file)

Internal minor enhancement, follow-up to bug 801232 comment 133, to make the
meaning of the function clearer.
(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> Is this what you mean by assert
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/debug.js#32>?

That, or a simple if false then Cu.reportError and return.

Marco, what do you think about using the JavaScript assertions module in the
Downloads Panel code?
Flags: needinfo?(mak77)
NS_assert is bad cause it opens a dialog directly in the face of the user, and since it doesn't distinguish between us (devs) and the final user it can easily become an issue. Indeed we used it in Places for some time, but due to some hard to debug bugs they started appearing to the final users, and were quite harmful for UX.

I think a simple Cu.reportError works fine for us.
Flags: needinfo?(mak77)
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #698111 - Flags: review?(mak77)
You should also remove the conditionals that check the object type later.

I also expected an early return if the assertion failed, but I'll leave this
style choice to Marco since we shouldn't reach that code path in any case.
(In reply to Paolo Amadini [:paolo] from comment #5)
> I also expected an early return if the assertion failed, but I'll leave this
> style choice to Marco since we shouldn't reach that code path in any case.

I agree with early return
Attached patch Patch (v2)Splinter Review
Attachment #698111 - Attachment is obsolete: true
Attachment #698111 - Flags: review?(mak77)
Attachment #698490 - Flags: review?(mak77)
Attachment #698490 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/d51c189670fc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.