820301 - Assert that ensurePersistentDataLoaded is not called on PrivateDownloadsData

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

Internal minor enhancement, follow-up to bug 801232 comment 133, to make the
meaning of the function clearer.

Comment 1

[(no longer active)]

Is this what you mean by assert <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/debug.js#32>?

Comment 2

[:Paolo Amadini]

(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?

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[:Paolo Amadini]

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.

Comment 6

[Marco Bonardo [:mak]]

(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

Comment 7

[(no longer active)]

Attachment: Patch (v2)

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d51c189670fc

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d51c189670fc