Closed
Bug 955273
Opened 10 years ago
Closed 10 years ago
Errors in handlers lose their location information
Categories
(Chat Core :: IRC, enhancement)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.4
People
(Reporter: aleth, Assigned: aleth)
Details
Attachments
(1 file, 1 obsolete file)
4.25 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1840 at 2012-11-28 17:14:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Assignee | ||
Comment 1•10 years ago
|
||
*** Original post on bio 1840 as attmnt 2142 at 2012-11-28 17:14:00 UTC *** Adds an optional parameter to scriptError() to which a caught error can be passed. In this case, the location information from this error is used instead of that of the exception handler. The original error message is appended to the new one.
Attachment #8353904 -
Flags: review?(florian)
Attachment #8353904 -
Flags: feedback?(clokep)
Comment 2•10 years ago
|
||
*** Original post on bio 1840 at 2012-11-28 17:18:23 UTC *** I guess my first question is whether the text we're adding really adds more information or if we could just show the original error. (I.e. does it need to be an extra parameter or can we just pass it into this.error?)
Assignee | ||
Comment 3•10 years ago
|
||
*** Original post on bio 1840 at 2012-11-28 17:22:50 UTC *** (In reply to comment #1) > I guess my first question is whether the text we're adding really adds more > information or if we could just show the original error. (I.e. does it need to > be an extra parameter or can we just pass it into this.error?) This patch is intended as an improvement over just adding "+ e" to the end of the new error string (look at the IRC use case in the patch) As we have to pass e as a parameter for the location information anyway, we don't have to add "+ e" to the string anymore by hand either.
Comment 4•10 years ago
|
||
*** Original post on bio 1840 at 2012-11-28 22:48:06 UTC *** (In reply to comment #1) > I guess my first question is whether the text we're adding really adds more > information or if we could just show the original error. (I.e. does it need to > be an extra parameter or can we just pass it into this.error?) Are you asking if we should add a check to detect when the value passed as aMessage parameter is actually already an nsIScriptError instance, and then use that instead of creating a new one? If that's your question, then I think it could be useful, but I don't think it solves the problem aleth is trying to solve here.
Comment 5•10 years ago
|
||
Comment on attachment 8353904 [details] [diff] [review] Patch *** Original change on bio 1840 attmnt 2142 at 2012-11-28 22:54:06 UTC *** >- scriptError.init(aMessage, caller.filename, sourceLine, caller.lineNumber, >- null, flag, "component javascript"); >+ if (!aOriginalError) { >+ scriptError.init(aMessage, caller.filename, sourceLine, caller.lineNumber, >+ null, flag, "component javascript"); >+ } >+ else { >+ aMessage += "\n" + (aOriginalError.message || aOriginalError); >+ scriptError.init(aMessage, aOriginalError.fileName || caller.filename, >+ sourceLine, aOriginalError.lineNumber || caller.lineNumber, >+ null, flag, "component javascript"); >+ } This triggers my usual "code duplication!" warning ;). I think I would prefer it written this way: let fileName = caller.filename; let lineNumber = caller.lineNumber; if (aOriginalError) { aMessage += "\n" + (aOriginalError.message || aOriginalError); if (aOriginalError.fileName) fileName = aOriginalError.fileName; if (aOriginalError.lineNumber) lineNumber = aOriginalError.lineNumber } let scriptError = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError); scriptError.init(...)
Attachment #8353904 -
Flags: review?(florian) → review-
Comment 6•10 years ago
|
||
*** Original post on bio 1840 at 2012-11-28 22:56:36 UTC *** Comment on attachment 8353904 [details] [diff] [review] (bio-attmnt 2142) Patch >-function scriptError(aModule, aLevel, aMessage) { >+function scriptError(aModule, aLevel, aMessage, aOriginalError) { Mind adding a comment explaining what the parameters are?
Comment 7•10 years ago
|
||
Comment on attachment 8353904 [details] [diff] [review] Patch *** Original change on bio 1840 attmnt 2142 at 2012-11-29 11:45:16 UTC *** (In reply to comment #3) > (In reply to comment #1) > > I guess my first question is whether the text we're adding really adds more > > information or if we could just show the original error. (I.e. does it need to > > be an extra parameter or can we just pass it into this.error?) > > Are you asking if we should add a check to detect when the value passed as > aMessage parameter is actually already an nsIScriptError instance, and then use > that instead of creating a new one? > > If that's your question, then I think it could be useful, but I don't think it > solves the problem aleth is trying to solve here. Yes, I was trying to understand what aleth was solving, he explained it online and I'm fine with these changes. (Plus florian's nits. :))
Attachment #8353904 -
Flags: feedback?(clokep) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
*** Original post on bio 1840 as attmnt 2143 at 2012-11-29 15:24:00 UTC *** Feel free to amend the comment I added if you see a way to improve it.
Attachment #8353905 -
Flags: review?(florian)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8353904 [details] [diff] [review] Patch *** Original change on bio 1840 attmnt 2142 at 2012-11-29 15:24:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353904 -
Attachment is obsolete: true
Comment 10•10 years ago
|
||
Comment on attachment 8353905 [details] [diff] [review] Patch *** Original change on bio 1840 attmnt 2143 at 2012-12-14 23:26:33 UTC *** I tweaked the comment a bit, mostly to make it look like a doxygen comment.
Attachment #8353905 -
Flags: review?(florian) → review+
Comment 11•10 years ago
|
||
*** Original post on bio 1840 at 2012-12-14 23:32:54 UTC *** Fixed in http://hg.instantbird.org/instantbird/rev/c440a7324cd1 Thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
You need to log in
before you can comment on or make changes to this bug.
Description
•