Rename NS_WARN_IF_FALSE as NS_WARNING_ASSERTION

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

The new name makes the sense of the condition much clearer. E.g. compare:

  NS_WARN_IF_FALSE(!rv.Failed());

with:

  NS_WARNING_ASSERTION(!rv.Failed());

The new name also makes it clearer that it only has effect in debug builds,
because that's standard for assertions.
I'd love to use NS_WARN_ASSERTION or NS_WARNING_ASSERT or even NS_WARN_ASSERT
to reduce the number of line breaks required... but NS_WARNING_ASSERTION really
feels like the best name for this.
Attachment #8787054 - Flags: review?(erahm)
Comment on attachment 8787054 [details] [diff] [review]
Rename NS_WARN_IF_FALSE as NS_WARNING_ASSERTION

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

r=me

We're definitely going to need to send out a message to dev-platform for this as well as update any MDN docs.

::: dom/ipc/Blob.cpp
@@ +1604,5 @@
>      nsCOMPtr<nsIThread> ioTarget;
>      mIOTarget.swap(ioTarget);
>  
> +    NS_WARNING_ASSERTION(NS_SUCCEEDED(stream->Close()),
> +                         "Failed to close stream!");

Follow up: stream->Close() in debug only assert.
Attachment #8787054 - Flags: review?(erahm) → review+
> > +    NS_WARNING_ASSERTION(NS_SUCCEEDED(stream->Close()),
> > +                         "Failed to close stream!");
> 
> Follow up: stream->Close() in debug only assert.

Oh snap! Nice catch. That adds weight to the "clearer that it only has effect in debug builds" argument from comment 0. In fact, bug 994190 did exactly this:

> -    nsresult rv = stream->Close();
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    NS_WARN_IF_FALSE(NS_SUCCEEDED(stream->Close()), "Failed to close stream!");

<sadface>

I filed bug 1300007 to fix it.
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93657f8dedaa
Rename NS_WARN_IF_FALSE as NS_WARNING_ASSERTION. r=erahm.
https://hg.mozilla.org/mozilla-central/rev/93657f8dedaa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1272010
You need to log in before you can comment on or make changes to this bug.