Closed Bug 502839 Opened 12 years ago Closed 12 years ago

Thread summaries fail when dealing with offline messages (e.g. news)

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: davida, Assigned: davida)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch WIP v1 (obsolete) — Splinter Review
Spunoff from bug 454829.

Attaching WIP, which for as-yet-unfigured-out reasons doesn't solve all thread summary issues.  In particular, I can get the system to fail to generate either kind of snippet (real or "not downloaded yet").

(This also includes a couple of cosmetic changes that I've run past bryan offline.)

(this requires the errUtils module from bug 500338)
Flags: blocking-thunderbird3+
Assignee: nobody → david.ascher
Whiteboard: [needs ui-review clarkbw]
mmm, #555; what a color! :)

We talked about this offline: I don't like the 'The message body hasn't been downloaded yet' string.  I think we could replace that with a (likely untranslated) '...'.  The current string is large and creates a lot of duplicated information when there is more than one of them in a row.

The best solution would be making news work, however not having the body will still be a reality in some cases.
Whiteboard: [needs ui-review clarkbw]
Whiteboard: [needs updated patch, reviews]
We're two days away from the firm code/string freeze and the WIP patch contains a new string. So can we please get some traction here? And is it really necessary to introduce a new string so late in the release cycle?
I've been on the road.  I'll refresh this patch today, w/ no new strings.  We'll use "..." as the string for b3, and come up with something better for b4.
Attached patch proposed patch (obsolete) — Splinter Review
This patches fixes the core issue, as well as a few other things:

1) If any exception was thrown by the streaming invoked in mimemsg.js, then the activestream listener was never removed, and subsequent streams of the same URI failed silently.  I remove the stream listener on exception, fixing that problem.

2) My original code had innerHTML places it should have had textContent.  That's a pretty serious security no-no.  There are still two current uses of innerHTML, but they're deriving data from an e4x object which (AFAICT) sanitizes all of its input data.

3) The selection summary code that was detecting "too big" selections was being short-circuited by some of the code in messageDisplay.js.  That code is removed here.  (that bit fixes bug 503484).

4) The aesthetics of the snippet have been modified, to use a) a slightly larger font, b) a slightly darker color, c) 300 chars, not 200.  Those changes have been ui-reviewed by clarkbw.

5) misc. code style cleanup & one doxygen correctness fix

Note:  In this version, the snippet body for messages that aren't available offline is just "...", because 1) anything looks goofy if it's repeated a lot of times, and 2) this doesn't require l10n, which is good given where we are in the string freezing process.  We may want to revisit this issue after we fix the issue for newsgroup messages, although I suspect most of those won't be marked for offline use anyway, so there's likely still work TBD even after that's done.

Throwing the review at asuth, because I touch some of his code, but if someone else wants to take it, I don't think he'll mind.
Attachment #387199 - Attachment is obsolete: true
Attachment #388422 - Flags: review?(bugmail)
Whiteboard: [needs updated patch, reviews] → [needs review]
Blocks: 503484
Comment on attachment 388422 [details] [diff] [review]
proposed patch

http://reviews.visophyte.org/r/388422/

r=asuth with the requested changes.  I am having build troubles so need to run
this with the unit tests still.


on file: mail/base/content/selectionsummaries.js line 229
>     const MAXCOUNT = 100;

Since you're touching this and also removed the awesome outer constant, please
either rename the constant to MAX_MESSAGES/MAX_THREADS/whatever or add a
comment that explains what the count actually cares about.


on file: mail/base/content/selectionsummaries.js line 287
>       msgNode.innerHTML = msgContents.toXMLString();

Please add a comment explaining why this use of innerHTML is safe.  After I,
the devil's advocate see {author}, {subject} and {countstring} above... how do
I know they are not full of eeevil HTML?  (Note: a certified devil's advocate
is okay with eeevil but I have yet to pass that test.)


on file: mail/base/content/selectionsummaries.js line 299
>                                                             aMsgHdr.folder, 300);

This number should also have a well-named and potentially well-commented
constant.


on file: mail/base/content/selectionsummaries.js line 304
>       } catch (e if e.result == Components.results.NS_ERROR_FAILURE) {

Please add a comment that explains why this is expected.


on file: mail/base/content/selectionsummaries.js line 305
>         // we don't have a great exception if it's an offline message

Add an XXX to this comment since I think this is a known cop-out and we should
make it easier to locate and fix said cop-out.


on file: mail/base/content/selectionsummaries.js line 538
>       msgNode.innerHTML = msgContents.toXMLString();

same comment complaint here about innerHTML.


on file: mail/base/content/selectionsummaries.js line 556
>       } catch (e if e.result == Components.results.NS_ERROR_FAILURE) {

Please add a comment that explains why this is expected.


on file: mail/base/content/selectionsummaries.js line 620
>     Component.utils.reportError(e);

Components with an s.


on file: mail/base/content/selectionsummaries.js line 641
>     Component.utils.reportError(e);

Components with an s.


on file: mailnews/db/gloda/modules/connotent.js line 90
>   let text = content.getContentSnippet();

Instead of no longer passing length, perhaps you could pass length+1 instead?

There are serious potential performance benefits to having the content
whittler be able to stop itself early, even if none of our whittlers are
really that clever yet.


on file: mailnews/db/gloda/modules/mimemsg.js line 198
>   } finally {
>     // If streamMessage throws an exception, we should make sure to clear the
>     // activeStreamListener, or any subsequent attempt at sreaming this URI
>     // will silently fail
>     if (activeStreamListeners[msgURI]) {
>       delete activeStreamListeners[msgURI];
>     }
>   }

I do this every time I go to use a finally; given that I suggested to use a
finally in this case, I think I've indirectly done it again here too.  You are
hoping that "finally" is akin to "catch (ex) { do stuff; throw ex; }", but it
is not.  I guess do that instead?
Attachment #388422 - Flags: review?(bugmail) → review+
Review comments addressed.  Thanks for the devil's advocacy, I escaped the input data.

Also, I changed the logic for counting messages to really count messages, not threads, fixing a correctness bug.
Attachment #388422 - Attachment is obsolete: true
Attachment #388631 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs review] → [need checkin]
http://hg.mozilla.org/comm-central/rev/490507630a60
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [need checkin]
Attached patch stupid mistakeSplinter Review
Attachment #388637 - Flags: review?(bugmail)
Why is it that with builds including this patch I can get more than a 100 summaries - which I thought was the limit that once reached I would get a warning message about only the first 100 being summarized.
I suspect you're trusting of the heading, which _should_ describe the number of selected messages.  In today's nightly, you should see some number of summaries (however many threads are covered by 100 messages), and a warning at the end of the page saying something like:


  These messages take up: 1.1 MB. (Note: 233 messages are selected, the first 100 are shown)

Or did you actually count the summaries and found more than 100?
(In reply to comment #11)
> I suspect you're trusting of the heading, which _should_ describe the number of
> selected messages.  In today's nightly, you should see some number of summaries
> (however many threads are covered by 100 messages), and a warning at the end of
> the page saying something like:
> 
> 
>   These messages take up: 1.1 MB. (Note: 233 messages are selected, the first
> 100 are shown)

Maybe it would make more sense to have that on top when you select a lot of messages and don't want to scroll down to see this.
 
> Or did you actually count the summaries and found more than 100?

No did not. - Your explanation seems correct. I just expected to be able to see the message.
(In reply to comment #12)

> Maybe it would make more sense to have that on top when you select a lot of
> messages and don't want to scroll down to see this.


Why?  It's a really minor point, and it's only there to be factually correct -- the main point of the selection is to afford actions on the set of selected messages, and people will want delete/archive etc. to apply to all of the messages they selected.  That we only show a 100 summaries doesn't feel that important to me at least.  I don't want to confuse people w/ "you selected 200 messages, we're showing you a hundred" at the top, because it'll confuse them about "but am I deleting 200, or 100?".

(I think you were only expecting that because you're running through the litmus test =)
You need to log in before you can comment on or make changes to this bug.