Last Comment Bug 746450 - remove console noise for external attachments that cannot be found
: remove console noise for external attachments that cannot be found
Status: RESOLVED FIXED
[good first bug]
:
Product: MailNews Core
Classification: Components
Component: Attachments (show other bugs)
: unspecified
: All All
: -- trivial (vote)
: Thunderbird 15.0
Assigned To: Magnus Melin
:
:
Mentors:
Depends on:
Blocks: 654933
  Show dependency treegraph
 
Reported: 2012-04-17 20:33 PDT by alta88
Modified: 2012-05-10 01:56 PDT (History)
2 users (show)
mkmelin+mozilla: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (4.05 KB, patch)
2012-04-21 03:39 PDT, Magnus Melin
squibblyflabbetydoo: review+
Details | Diff | Splinter Review

Description alta88 2012-04-17 20:33:28 PDT
Error: Couldn't open external attachment!
Source file: chrome://messenger/content/msgHdrViewOverlay.js
Line: 610

this is an invalid throw; nntp or feed attachments, or mail attachments that have been moved trigger this, wrongly.

plus, it doesn't seem this, even if valid, would help a user in any way.
Comment 1 Jim Porter (:squib) 2012-04-17 21:14:08 PDT
It's not actually throwing, just reporting to the console. I'm not opposed to removing it, but it's not hurting anything. Patches welcome.
Comment 2 alta88 2012-04-18 07:16:26 PDT
the console pollution is, in fact, a detriment and distraction to your fellow developers and alarming to any user who happens upon it.  first patch?  it seems better manners for the author of this 'work' to take the 30 seconds and clean up after himself.
Comment 3 alta88 2012-04-18 08:07:01 PDT
forgot to add ;)
Comment 4 Magnus Melin 2012-04-21 03:39:22 PDT
Created attachment 617205 [details] [diff] [review]
proposed fix

FTR, normal nntp attachments seemed fine to me.
Comment 5 Magnus Melin 2012-04-21 03:40:41 PDT
Ugh, and without the trailing "live"
Comment 6 alta88 2012-04-21 11:10:13 PDT
if an email attachment has been detached, and then moved/deleted, it's incorrect to raise an error simply because the original file is not there.  since it cannot be determined if this is a real error or user action, it seems the whole premise is not right.
Comment 7 Jim Porter (:squib) 2012-04-21 16:29:40 PDT
I think the message would be useful for people who didn't intend to delete their external attachment. How about downgrading it to a warning, and also indicating the name of the attachment we failed on?
Comment 8 alta88 2012-04-22 07:56:21 PDT
the number of people who did that and went to look in Tb console is next to 0.

the proper way to indicate this to a user is to check the existence of an external attachment, file:// protocol, on local disk at attachment list creation time and set an attribute for styling if it's not found (X icon or strikethrough eg), and/or a tooltiptext stating the original detached location no longer exists.  but not send it to console.
Comment 9 Magnus Melin 2012-04-22 11:16:53 PDT
Yeah i agree we should have a proper ui for those cases.
So the current patch would only log to the console if we can't open for unknown reasons. I'm not sure what those would be, but i didn't dare to loose the try/catch completely.
Comment 10 Jim Porter (:squib) 2012-05-08 19:12:11 PDT
Comment on attachment 617205 [details] [diff] [review]
proposed fix

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

I've thought about this, and I think this method is ok. I'm not sure we'd ever hit the catch block though, so I'd be ok with not reporting the error too.
Comment 11 Magnus Melin 2012-05-09 12:11:36 PDT
http://hg.mozilla.org/comm-central/rev/bb5a9e8521a2 ->FIXED

Note You need to log in before you can comment on or make changes to this bug.