1318974 - New messages sometimes not scrolled into view

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Florian Quèze [:florian]]

Attachment: Fix

Comment 2

[aleth [:aleth]]

Comment on attachment 8812626 [details] [diff] [review]
Fix

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.

Comment 3

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/d4e762ef561de865b6e95befb61beafaf914866d
Bug 1318974 - Cleanup the _pendingMessages array even if getNextPendingMessage never returns null, r=aleth. a=aleth CLOSED TREE

Comment 4

[aleth [:aleth]]

We should probably uplift this!

Comment 5

[Florian Quèze [:florian]]

(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 6

[aleth [:aleth]]

Comment on attachment 8812626 [details] [diff] [review]
Fix

[Approval Request Comment]
User impact if declined: Fixes a relatively rare but annoying edge case
Testing completed (on c-c, etc.): yes

Comment 7

[Jorg K (CEST = GMT+2)]

C-A (TB 52): https://hg.mozilla.org/releases/comm-aurora/rev/49c1ec6b281d11d8c7417405340f85631aa47039
C-B (TB 51): https://hg.mozilla.org/releases/comm-beta/rev/5af3facabea75a661b9e3a76c67e0717cd302ff8

Comment 8

[Kent James (:rkent)]

Comment on attachment 8812626 [details] [diff] [review]
Fix

https://hg.mozilla.org/releases/comm-esr45/rev/bdbed6658861