Closed Bug 1492735 Opened 6 years ago Closed 6 years ago

Port Bug 1486147 Support JS iterator protocol for nsIStringEnumerator

Categories

(MailNews Core :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

Details

Attachments

(4 files, 4 obsolete files)

Busted!
Attached patch 1492735-string-enum2.patch - WIP (obsolete) — Splinter Review
Fixed some compile errors, but not all, nsMsgHdr.cpp not compiling.
Assignee: nobody → jorgk
Assignee: jorgk → nobody
Version: 24 → Trunk
This seems to compile. Probably heaps of test failures now?

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=04c1c016a705c0ebf3bd3f0bba13f9a31db397c6
Attachment #9010541 - Attachment is obsolete: true
Looking at the M-C patches, the nsIStringEnumerator also needs to be added to
NS_IMPL_ISUPPORTS(xxx, nsIUTF8StringEnumerator, nsIStringEnumerator)
Assignee: nobody → jorgk
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9ebd15259379
Port Bug 1486147: Adapt inheritance of string enumerators in mailnews/ and add QIs(). rs=bustage-fix
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Attachment #9010543 - Attachment is obsolete: true
Attachment #9010549 - Flags: review?(acelists)
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2bfc93d3de04
Port Bug 1486147: Provide Symbol.iterator for JS objects implementing nsIStringEnumerator; rs=bustage-fix DONTBUILD
Comment on attachment 9010549 [details] [diff] [review]
1492735-string-enum2.patch (v3) [landed in comment #7]

Kris, could you please look at these.
Attachment #9010549 - Flags: feedback?(kmaglione+bmo)
Attachment #9010573 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 9010549 [details] [diff] [review]
1492735-string-enum2.patch (v3) [landed in comment #7]

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

Thanks

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +82,5 @@
>  #include "nsArrayEnumerator.h"
>  #include "nsAutoSyncManager.h"
>  #include "nsIMsgFilterCustomAction.h"
>  #include "nsMsgReadStateTxn.h"
> +#include "nsStringEnumerator.h"

OK, just because nsStringEnumerator.h includes nsIStringEnumerator.h, as you add reference to nsIStringEnumerator .
Attachment #9010549 - Flags: review?(acelists) → review+
Attachment #9010549 - Flags: feedback?(kmaglione+bmo) → feedback+
Comment on attachment 9010573 [details] [diff] [review]
1492735-string-enum-iterator-1.diff [landed in comment #8]

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

::: mailnews/mime/src/mimeJSComponents.js
@@ +18,5 @@
>  }
>  StringEnumerator.prototype = {
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsIUTF8StringEnumerator]),
> +  [Symbol.iterator]: function () {
> +    return this;

This will only work if you implement a next() method.
Attachment #9010573 - Flags: feedback?(kmaglione+bmo)
Would anyone be bothered if I just changed msgIStructuredHeaders.headerNames from nsIUTF8StringEnumerator to nsIJSEnumerator (ie. a normal JS iterator)? We only use it in tests, but I guess there might be extensions out there…
Flags: needinfo?(jorgk)
No opinion, sorry.
Flags: needinfo?(jorgk) → needinfo?(acelists)
And what would be the benefit? They seem to be iterable in for..of already.
For extensions, we could mention the change at https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_63
Flags: needinfo?(acelists)
We wouldn't need this implementation of nsIUTF8StringEnumerator hanging around any more.
Same as before, but with headerNames changed from nsIUTF8StringEnumerator to nsIJSEnumerator.
Attachment #9010573 - Attachment is obsolete: true
Attachment #9010573 - Flags: review?(philipp)
Attachment #9011334 - Flags: review?(philipp)
Comment on attachment 9011334 [details] [diff] [review]
1492735-string-enum-iterator-2.diff [mailnews part landed in comment #23]

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

This looks ok to me for the calendar bits. I also checked the other changes, I'll leave it up to you to determine if my review is enough.
Attachment #9011334 - Flags: review?(philipp) → review+
Comment on attachment 9011334 [details] [diff] [review]
1492735-string-enum-iterator-2.diff [mailnews part landed in comment #23]

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

The rest also looks good. Always nice to be able to get rid of unneeded code.
Attachment #9011334 - Flags: review+
(In reply to Geoff Lankow (:darktrojan) from comment #16)
> We wouldn't need this implementation of nsIUTF8StringEnumerator hanging
> around any more.

Which implementation? We still use it in other files.

And what is the difference of nsIJSEnumerator and nsISimpleEnumerator?

Please make sure that the nsIJSEnumerator works with fixIterator from iteratorUtils.jsm.
(In reply to :aceman from comment #20)
> (In reply to Geoff Lankow (:darktrojan) from comment #16)
> > We wouldn't need this implementation of nsIUTF8StringEnumerator hanging
> > around any more.
> 
> Which implementation? We still use it in other files.

The one from mimeJSComponents.js I'm proposing we get rid of. We don't use that in other files.

> And what is the difference of nsIJSEnumerator and nsISimpleEnumerator?
> 
> Please make sure that the nsIJSEnumerator works with fixIterator from
> iteratorUtils.jsm.

nsIJSEnumerator is just the XPCOM term for an ordinary JS iterator, which fixIterator is compatible with.
Keywords: checkin-needed
Attachment #9010549 - Attachment description: 1492735-string-enum2.patch (v3) → 1492735-string-enum2.patch (v3) [landed in comment #7]
Comment on attachment 9010573 [details] [diff] [review]
1492735-string-enum-iterator-1.diff [landed in comment #8]

Calendar part was approved for attachment 9011334 [details] [diff] [review] since it's the same code. No need to back it out and land it again. Will fix mailnews/ part.
Attachment #9010573 - Attachment description: 1492735-string-enum-iterator-1.diff → 1492735-string-enum-iterator-1.diff [landed in comment #8]
Attachment #9010573 - Flags: review+
Attachment #9010573 - Attachment is obsolete: false
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3bc0fb310b8a
Port Bug 1486147: Replace use of nsIUTF8StringEnumerator in msgIStructuredHeaders. r=mkmelin
Keywords: checkin-needed
Attachment #9011334 - Attachment description: 1492735-string-enum-iterator-2.diff → 1492735-string-enum-iterator-2.diff [mailnews part landed in comment #23]
Backout:
https://hg.mozilla.org/comm-central/rev/e386db4f34bd5db42dcad676cb465b010b165299

Sorry, I had to back this out due to four test failures:
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_offlineDraftDataloss.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_autoReply.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMessageLater2.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_saveImapDraft.js | xpcshell return code: -11

I confirmed locally that the backout fixes the tests. It also got landed with the incorrect author. I'll fix that next time.
Status: RESOLVED → REOPENED
Flags: needinfo?(geoff)
Resolution: FIXED → ---
This is the patch that got landed in comment #23 and backed out in comment #24.
Attachment #9011334 - Attachment is obsolete: true
With just one extra word I could've fixed it properly in the first patch. I is smart.
Assignee: jorgk → geoff
Flags: needinfo?(geoff)
Comment on attachment 9012425 [details] [diff] [review]
1492735-string-enum-iterator-3.diff

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

::: mailnews/compose/test/unit/test_nsIMsgCompFields.js
@@ -10,5 @@
>  
>  function check_headers(enumerator, container) {
>    let checkValues = new Set(container.map(header => header.toLowerCase()));
> -  while (enumerator.hasMore()) {
> -    let value = enumerator.getNext().toLowerCase();

the .toLowerCase() went missing here. Is it not needed?
Oops, no, that's me screwing up again.
Attachment #9012425 - Attachment is obsolete: true
Comment on attachment 9012550 [details] [diff] [review]
1492735-string-enum-iterator-4.diff

OK, let's do it ;-)
Attachment #9012550 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/20b96eb01d6e
Port Bug 1486147: Provide Symbol.iterator for JS objects implementing nsIStringEnumerator, part 2. r=mkmelin,jorgk
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: