Closed Bug 975944 Opened 6 years ago Closed 6 years ago

Log.jsm's appender objects have wrong ._name

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Don't overwrite this._name (obsolete) — Splinter Review
Every appender in log.jsm sets this._name to something correct, then calls Appender() - which resets this._name to "Appender".  As a result every appender has a name of "Appender".

This patch just changes Appender() to only set this._name if it is not already defined.  Another alternative would be to change every appender to this._name is set only after Appender has been called.
Attachment #8380454 - Flags: review?(bmcbride)
Comment on attachment 8380454 [details] [diff] [review]
Don't overwrite this._name

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

I'd rather have the other approach of calling the super constructor first (generally more common anyway) - as that also solves it for cases such as BoundedFileAppender. Otherwise you'd need to add that check to every constructor, just to be safe :\
Attachment #8380454 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] from comment #1)
> I'd rather have the other approach of calling the super constructor first
> (generally more common anyway)

Fairy nuff - how's this?
Assignee: nobody → mhammond
Attachment #8380454 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8380566 - Flags: review?(bmcbride)
Comment on attachment 8380566 [details] [diff] [review]
0008-Bug-975944-fix-log-appender-s-._name-property.-r-unf.patch

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

r+ with a fixup

::: toolkit/modules/Log.jsm
@@ +588,5 @@
>  
>    // This is a promise exposed for testing/debugging the logger itself.
>    this._lastWritePromise = null;
>    Appender.call(this, formatter);
> +  this._name = "FileAppender";

Should move the Appender.call() line to the top, rather than the this._name line to the bottom. Ditto in BoundedFileAppender.
Attachment #8380566 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/3e84bd445574
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.