I think this one is covered by bug 1318776 too and I am still analyzing if it is correct. Do you know why the view is null and why it is expected condition?
Comment on attachment 8836379 [details] [diff] [review] Avoid null reference of this.view.dbView Sorry, I got there first ;-)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1318776
I think we need two changes here.
Attachment #8838517 - Flags: review?(acelists)
For the record, tests showing these warnings: folder-display/test-deletion-from-virtual-folders.js (for the this.view.displayedFolder case) folder-display/test-message-pane-visibility.js (for the this.view.dbView case)
I have looked at the patches and what the tests do. For the this.view.dbView I propose an alternate solution here. Using treeBox.view returns the correct number of rows in this particular test (and does not visibly regress others, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7336e0359c5dc4480516ff0ee2ca4ef016cfd8b5). I don't know why treeBox.view is not null and this.view.dbView is in this case. But I think when the function ensureRowsIsVisible and also ensureRowRangeIsVisible calculate everything based on attributes of treeBox, why should we take rowCount from a different object (this.view.dbView).
Attachment #8839820 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8838517 [details] [diff] [review] 1338798-js-type-errors.patch Review of attachment 8838517 [details] [diff] [review]: ----------------------------------------------------------------- I proposed a different solution for the second hunk of the patch. For the first part with this.view.displayedFolder I still don't know why it happens. The test switches from an empty search folder to the smart (unified) folder mode. It could be there is a time window where some of the view is not yet set up when the tree is rebuilt. But I don't know how bad an empty this.view.displayedFolder condition is. There is a similar check in canArchiveSelectedMessages() where it checks this.view.isSingleFolder together with this.view.displayedFolder as in the proposed patch. But if you add the check, the block is skipped and the following code is run (this.navigate(nsMsgNavigationType.lastMessage, /* select */ false)) or this.ensureRowIsVisible(0)). And I don't know if that is the right thing to do (it changes behaviour). Maybe if we are in some temporary state of disorder, we should just exit the function not run the code breaking the stored view position (remembered last message).
Attachment #8838517 - Flags: review?(acelists) → review?(mkmelin+mozilla)
Comment on attachment 8839820 [details] [diff] [review] alternate solution for this.view.dbView Review of attachment 8839820 [details] [diff] [review]: ----------------------------------------------------------------- Yes, all the operations later are on treeBox, so this seems like a good approach. r=mkmelin
Attachment #8839820 - Flags: review?(mkmelin+mozilla) → review+
Magnus, the first hunk here is not superseded by the other patch. So this needs looking at too. I have now left only the relevant part after you accepted the other patch.
Comment on attachment 8841364 [details] [diff] [review] 1338798-js-type-errors.patch v2 Review of attachment 8841364 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/folderDisplay.js @@ +1164,3 @@ > // use the displayed folder; nsMsgDBView goes to the effort to save the > // state to the viewFolder, so this is the correct course of action. > let lastLoadedMessageKey = this.view.displayedFolder.lastMessageLoaded; Hard to see, but here is |this.view.displayedFolder| and if it's null, there is no joy to be had ;-(
Comment on attachment 8841364 [details] [diff] [review] 1338798-js-type-errors.patch v2 Review of attachment 8841364 [details] [diff] [review]: ----------------------------------------------------------------- Great, thx! r=mkmelin
Attachment #8841364 - Flags: review?(mkmelin+mozilla) → review+
Assignee: ishikawa → acelists
Status: NEW → RESOLVED
Closed: 3 years ago → 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
You need to log in before you can comment on or make changes to this bug.