1115954 - Use fixIterator instead of enumerator in timezone prefs

Status: RESOLVED WONTFIX

(Calendar :: Preferences, task)

Description

[Richard Marti (:Paenglab)]

+++ This bug was initially created as a clone of Bug #432675 +++

Comment on attachment 8541879 [details] [diff] [review] [diff] [review]
moveTZ.patch

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

::: mailnews/base/util/iteratorUtils.jsm
@@ +118,5 @@
>      return { __iterator__: iter };
>    }
>  
> +  // How about nsIStringEnumerator or nsIUTF8StringEnumerator?
> +  if (aEnum instanceof Ci.nsIStringEnumerator || aEnum instanceof Ci.nsIUTF8StringEnumerator) {

I'd suggest adding a comment why it needs to happen after the former block. I've dug in one step deeper and have come to this conclusion:

// This needs to happen after nsISimpleEnumerator because the default C++ enumerator implementation offers both for empty enumerators.

While it may work in the Lightning case because our string enumerator is js and only implements nsIUTF8StringEnumerator, I'm not yet sure its the right approach: what happens if its the default implementation [1] is used for something that is actually a string enumerator? Will the code think its a nsISimpleEnumerator and just return the nsISupports instances instead of strings?

I haven't had time to find a such case to test with, I think we should do that before pushing this code. Maybe its easier to just revert the iteratorUtils changes and iterate manually, then open a new bug to add the code to iteratorUtils.

r- if you'd like to do everything in this bug, r+ with the old iteration code if you prefer.


[1] http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsEnumeratorUtils.cpp#16e reset."

Comment 1

[Richard Marti (:Paenglab)]

Attachment: fixIterator.patch

This patch has to be applied on top of bug 432675.

Philipp, I tried also to remove 'aEnum instanceof Ci.nsIStringEnumerator' from 'if (aEnum instanceof Ci.nsIStringEnumerator || aEnum instanceof Ci.nsIUTF8StringEnumerator)' and it still worked. Could this be a solution?

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

I'm surprised that it works when removing nsIStringEnumerator, because the empty enumerator doesn't even implement that.

What about a special case? Untested, but maybe this is enough:

// Check if its a string enumerator. Some string enumerators are also simple enumerators, so 
// only assume its really a string enumerator if no interface was passed.
if ((aEnum instanceof Ci.nsIStringEnumerator || aEnum instanceof Ci.nsIUTF8StringEnumerator)) &&
    (!(aEnum instanceof Ci.nsISimpleEnumerator) || !aIface) {
    // TODO Return object for string enumerator
}

if (aEnum instanceof Ci.nsISimpleEnumerator) {
  // TODO return object for simple enumerator
}

Another thing we could try is this (also untested):

if (aEnum instanceof Ci.nsIStringEnumerator || aEnum instanceof Ci.nsIUTF8StringEnumerator) {
    let iter = function() {
        while (aEnum.hasMore()) {
            let next = aEnum.getNext();
            if (typeof next == "string") {
                yield next;
            } else if (next instanceof Ci.nsISupportsPrimitive) {
                yield next.QueryInterface(aIface || Ci.nsISupportsCString).data;
            } else {
                yield next.QueryInterface(aIface);
            }
        }
    };
    return { __iterator__: iter };
}


I think we are definitely at the stage where we should add a test for this. To do so, I'd suggest taking one enumerator of each type (combined iterator from nsICategoryManager, custom js-only string enumerator, c++ only string enumerator like from nsIMsgDbHdr::propertyEnumerator).

Also, a mailnews peer needs to review this change.

Comment 3

[:aceman]

It looks like bug 474219 is doing a very similar change.

Comment 4

[Martin Schröder [:mschroeder]]

Like bug 474219 this should be resolved as WONTFIX now.