Closed Bug 1318974 Opened 4 years ago Closed 4 years ago

New messages sometimes not scrolled into view


(Chat Core :: General, defect)

Not set


(thunderbird_esr4551+ fixed, thunderbird51 fixed, thunderbird52 fixed, thunderbird53 fixed)

Instantbird 53
Tracking Status
thunderbird_esr45 51+ fixed
thunderbird51 --- fixed
thunderbird52 --- fixed
thunderbird53 --- fixed


(Reporter: florian, Assigned: florian)




(1 file)

This seems to be a regression from bug 1315479, but it actually just uncovered an existing bug. If we interrupt the displayPendingMessages loop before getNextPendingMessage returned null, the _pendingMessages array isn't cleaned up and getPendingMessagesCount returns a bogus value at the next call.
Blocks: 1315479
Attached patch FixSplinter Review
Attachment #8812626 - Flags: review?(aleth)
Comment on attachment 8812626 [details] [diff] [review]

Review of attachment 8812626 [details] [diff] [review]:

So that means this bug happens whenever we run out of messages to display just as we run out of time for the current cycle.

I suppose the original idea behind this was to be more generous about letting the array keep growing before scrolling to the end.

The real problem here seems to be that there's two counters, this._nextPendingMessageIndex and this._pendingMessagesDisplayed, one being internal to the stuff that can be overridden.
Attachment #8812626 - Flags: review?(aleth) → review+
Bug 1318974 - Cleanup the _pendingMessages array even if getNextPendingMessage never returns null, r=aleth. a=aleth CLOSED TREE
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 53
We should probably uplift this!
(In reply to aleth [:aleth] from comment #4)
> We should probably uplift this!

We can for the sake of correctness, but we backed bug 1315479 out of 52, so the bug is likely invisible there. I think it hardly ever manifests itself when loading old messages, and we are very unlikely to have more than 40 new incoming messages to display at once.
Comment on attachment 8812626 [details] [diff] [review]

[Approval Request Comment]
User impact if declined: Fixes a relatively rare but annoying edge case
Testing completed (on c-c, etc.): yes
Attachment #8812626 - Flags: approval-comm-esr45?
Attachment #8812626 - Flags: approval-comm-beta?
Attachment #8812626 - Flags: approval-comm-aurora?
Attachment #8812626 - Flags: approval-comm-beta?
Attachment #8812626 - Flags: approval-comm-beta+
Attachment #8812626 - Flags: approval-comm-aurora?
Attachment #8812626 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.