iteratorUtils.js in ABSearchDialog.js not found

RESOLVED FIXED in Thunderbird 53.0

Status

--
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 53.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.46 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
In bug 857230 I mistyped iteratorUtils.js instead of iteratorUtils.jsm

This now throws errors in ABSearchDialog.js:

NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import] 1 ABSearchDialog.js:6
	<anonymous> chrome://messenger/content/ABSearchDialog.js:6:1

ReferenceError: reference to undefined property "undefined" 1 ABSearchDialog.js:57:3

TypeError: Components.classes[searchSessionContractID] is undefined 1 ABSearchDialog.js:57:3
	searchOnLoad chrome://messenger/content/ABSearchDialog.js:57:3
	onload chrome://messenger/content/ABSearchDialog.xul:1:1
(Assignee)

Comment 1

2 years ago
Created attachment 8814679 [details] [diff] [review]
patch
Attachment #8814679 - Flags: review?(philip.chee)
Attachment #8814679 - Flags: review?(jorgk)

Comment 2

2 years ago
Comment on attachment 8814679 [details] [diff] [review]
patch

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

::: mail/base/content/ABSearchDialog.js
@@ -180,5 @@
>      var searchUri = currentAbURI + "?(";
>  
> -    for (let searchTerm of fixIterator(gSearchSession.searchTerms, nsIMsgSearchTerm)) {
> -      // get the "and" / "or" value from the first term
> -      if (i == 0) {

You didn't spot that 'i' when you introduced the fixIterator, right? How about you just scrap the fixIterator and go back to the original loop controlled by 'i'? Pulling it out before the loop and controlled with a searchTerms.length > 0 appears mildly unelegant ;-)
(Assignee)

Comment 4

2 years ago
Pulling it out of the loop seems semantically better per the comment, we want to set it once and be done with it, no need to check i==0 for all the array members.

But yes, I can revert it back :)
(Assignee)

Comment 5

2 years ago
Created attachment 8814698 [details] [diff] [review]
patch v2

The beauty of the fixIterator is that it hides away what type of array we are handling. Now we hardcoded calls to .length and queryElementAt() so if it ever stops being a nsIArray we need to change the attributes/methods again.
Had it been fixIterator before bug 857230 we wouldn't even need to touch this loop when switching from nsISupportArray.

That seems to be also the point of implementing .Count() and GetElementAt() on a nsIArray so that the methods are named the same as on nsISupportsArray. The comment in nsIArrayExtensions says so.

But I don't think we have a general plan whether to migrate to fixIterator or away from it, so it doesn't matter much.
Attachment #8814679 - Attachment is obsolete: true
Attachment #8814679 - Flags: review?(philip.chee)
Attachment #8814679 - Flags: review?(jorgk)
Attachment #8814698 - Flags: review?(philip.chee)
Attachment #8814698 - Flags: review?(jorgk)

Comment 6

2 years ago
Comment on attachment 8814698 [details] [diff] [review]
patch v2

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

Can we land this now since something is broken right now.
Attachment #8814698 - Flags: review?(jorgk) → review+

Comment 7

2 years ago
https://hg.mozilla.org/comm-central/rev/1c4780bb98f563027ac46fbe74c25c770c590733
Landed to fix the regression. Since this was merely returning to the pre bug 857230 state, I think we can live without a further review.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0

Updated

2 years ago
Attachment #8814698 - Flags: review?(philip.chee)

Comment 8

2 years ago
Danke!
You need to log in before you can comment on or make changes to this bug.