463047 - Use js 1.7 iterators for properties bag and ical components

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

Attachment: Use Iterators - v1

This patch makes wonderful use of the new js 1.7 iterators for both calPropertyBag and those components that have the getFirstXXX and getNextXXX functions. It also upgrades some code to use calUtils.jsm instead of the same function from calUtils.js.

It also includes a fix for building gdata with the new cal.loadScripts() function.

The file calAlarm.js was left untouched, because I have an upcoming patch that fixes a couple of other things in that file. The use of these iterators will follow there.

For easier QA, the following areas were touched:

* Item Attachments: Operations that do copying and make them immutable
* Item Attendees: Operations that do copying and make them immutable
* Item Relations: Operations that do copying and make them immutable
* Parsing ICS, also from Sunbird 0.2
* Items:
 - Operations that do copying and make them immutable
 - Exporting attachments,attendees,relations,categories,rrules,alarms
 - Importing items with unpromoted props (i.e TRANSP)
* Discovering the timezone on first start
* Looking at Emails with ICS attachments
* Parsing caldav and caldav freebusy requests
* Parsing wcap ICS
* Pasting from clipboard
  - The clipboard should now support pasting both snippets that start with
    BEGIN:VCALENDAR and also BEGIN:VEVENT

Comment 1

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

Attachment: Use Iterators - v2

After learning more about how iterators should work (i.e what the standard object iterator does), I needed to modify this patch a bit.

Comment 2

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

This bug should get some testing before it hits the trees. I've noticed a typo that didn't produce an error on first sight between v1 and v2 and there may be more to come.

Anyone who would like to test and can't build, please ping me for a linux build, I can arrange windows or mac on request.

Comment 3

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

Another one spotted...

pretend that the line about categories in calItemBase.js looks like this:

this.mCategories = [ catprop.value for (catprop in cal.icalPropIterator(icalcomp, "CATEGORIES")) ];

Comment 4

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

The gdata part was fixed differently in bug 462393, please ignore the chunks for gdata in this review.

Comment 5

[Daniel Boelzle [:dbo]]

Comment on attachment 346314 [details] [diff] [review]
Use Iterators - v2

Philipp, could you please debitrot this patch?

some comments reading it:

I am missing lots of Components.import statements for calUtils.jsm, e.g. in calItemBase.js. Import statements for calUtils.jsm ought to be cheap, because a module file is executed only once; exported symbols are shared among all importing scripts.
Why did you remove the import statement in calIcsParser.js?

>+        for each (let [name, value] in cal.icalParamIterator(icalatt)) {
>+            if (promotedProps[name]) {
>                 continue;
>-            this.setProperty(paramname, icalatt.getParameter(paramname));
>+            }
>+            this.setProperty(name, value);
>         }
if (!promotedProps...
IMO reads better instead of using continue

>+        this.mProperties = [ prop for (prop in cal.icalPropIterator(calComp))
>+                                  if (prop.propertyName != "VERSION" &&
>+                                      prop.propertyName != "PRODID") ];
I don't like this notation and prefer Array.filter().

>+        let prodId = calComp.getFirstProperty("PRODID");
>+        let isFromOldSunbird;
please init |isFromOldSunbird|

>+        for (let recprop in cal.icalPropIterator(icalcomp)) {
>+            let ritem = null;
>             if (recprop.propertyName == "RRULE" ||
>-                recprop.propertyName == "EXRULE")
>-            {
>+                recprop.propertyName == "EXRULE") {
>                 ritem = new CalRecurrenceRule();
>             } else if (recprop.propertyName == "RDATE" ||
>-                       recprop.propertyName == "EXDATE")
>-            {
>+                       recprop.propertyName == "EXDATE") {
>                 ritem = new CalRecurrenceDate();
>             } else {
>                 continue;
while you are here, please spend a switch-case


>diff --git a/calendar/base/src/calUtils.js b/calendar/base/src/calUtils.js
>--- a/calendar/base/src/calUtils.js
>+++ b/calendar/base/src/calUtils.js
>@@ -1414,9 +1414,7 @@
>         if (icalComp.componentType != "VCALENDAR") {
>             return func(icalComp);
>         }
>-        for (var subComp = icalComp.getFirstSubcomponent("ANY");
>-             subComp;
>-             subComp = icalComp.getNextSubcomponent("ANY")) {
>+        for (let subComp in cal.icalSubcompIterator(icalComp)) {
>             if (!calIterateIcalComponent(subComp, func)) {
>                 return false;
>             }
IMO it's getting problematic to refer to calUtils.jsm symbols in calUtils.js, because this would imply you need to import it in calUtils.js (while calUtils.jsm loads calUtils.js...).
Please move the whole function to calUtils.jsm and adopt all usages in the tree.
Wouldn't this function be a good candidate for iteration, too, e.g. cal.ical.deepComponentIterator?

>+    this.mKeys = [ key for (key in bag.mData) ];
please remove:      ^                          ^

I think it makes sense to span a sub-scope for all ical helpers, e.g. |cal.ical|. I really like to get rid of function prefixes.

>+    /**
>+     * Use to iterate through all subcomponents of a calIIcalComponent.
>+     *
>+     * @param aComponent        The component who's subcomponents to iterate.
>+     * @param aSubcomp          (optional) the specific subcomponent to
>+     *                            enumerate. If not given, "ANY" will be used.
>+     * @return                  An Iterator object to iterate the properties.
>+     */
>+    icalSubcompIterator: function cal_icalSubcompIterator(aComponent, aSubcomp) {
>+        return {
>+            __iterator__: function icalSubcompIterator(aWantKeys) {
>+                let subcompName = (aSubcomp || "ANY");
>+                for (let subcomp = aComponent.getFirstSubcomponent(subcompName);
>+                     subcomp;
>+                     subcomp = aComponent.getNextProperty(propertyName)) {
this looks odd, you are getting the next property

>+                    yield (aWantKeys ? subcomp : [subcomp, subcomp.serializeToICS()]);
The concept of wantKeys doesn't seem to match here. This serializes the ical component in most usages which is unnecessary.

>+                }
>+            }
>+        };
>+    },
This iterator does not iterator over sub-sub-components, e.g. I can imagine it doesn't get VALARM components of a VCALENDAR.
Either define/document the iterator more strictly or overwork it with options (e.g. incorporating the above mentioned deepIterator?).

>+    /**
>+     * Use to iterate through all properties of a calIIcalComponent.
>+     *
>+     * @param aComponent        The component to iterate.
>+     * @param aProperty         (optional) the specific property to enumerate.
>+     *                            If not given, "ANY" will be used.
>+     * @return                  An Iterator object to iterate the properties.
>+     */
>+    icalPropIterator: function cal_icalPropIterator(aComponent, aProperty) {
>+        return {
>+            __iterator__: function icalPropIterator(aWantKeys) {
>+                let propertyName = (aProperty || "ANY");
>+                for (let prop = aComponent.getFirstProperty(propertyName);
>+                     prop;
>+                     prop = aComponent.getNextProperty(propertyName)) {
>+                    yield (aWantKeys ? prop : [prop, prop.value]);
>+                }
>+            }
>+        };
>+    },
I am not sure about the keye vs. non-keys handling on ical properties. I think the notion of key doesn't fit here, because a component could have multiple (even identical) properties.
I think the iterator should only return the property objects one after another.

>+    /**
>+     * Use to iterate through all parameters of a calIIcalProperty.
>+     *
>+     * @param aProperty         The property to iterate.
>+     * @param aParam            (optional) The parameter to retrieve.
>+     * @return                  An Iterator object to iterate the properties.
>+     */
>+    icalParamIterator: function cal_icalParamIterator(aProperty, aParam) {

The second parameter seems to be unused in this patch. For what purpose did you add it?

>diff --git a/calendar/providers/wcap/calWcapUtils.js b/calendar/providers/wcap/calWcapUtils.js
>--- a/calendar/providers/wcap/calWcapUtils.js
>+++ b/calendar/providers/wcap/calWcapUtils.js
>@@ -203,19 +203,24 @@
> }
> 
> function forEachIcalComponent(icalRootComp, componentType, func, maxResults) {
>-    var itemCount = 0;
>+    let itemCount = 0;
>     // libical returns the vcalendar component if there is just
>     // one vcalendar. If there are multiple vcalendars, it returns
>     // an xroot component, with those vcalendar childs. We need to
>     // handle both.
>-    for (var calComp = (icalRootComp.componentType == "VCALENDAR"
>-                        ? icalRootComp : icalRootComp.getFirstSubcomponent("VCALENDAR"));
>-         calComp != null && (!maxResults || itemCount < maxResults);
>-         calComp = icalRootComp.getNextSubcomponent("VCALENDAR")) {
>+    let compToIterate = (icalRootComp.componentType == "VCALENDAR" ?
>+                         { __iterator__: function() { yield icalRootComp; } } :
>+                         cal.icalSubcompIterator(icalRootComp, "VCALENDAR"));
>+                         
>+    for (let calComp in compToIterate) {

This looks pretty complicated. I'd prefer a refurbished cal.ical.componentIterator with options :)

>diff --git a/calendar/resources/content/clipboard.js b/calendar/resources/content/clipboard.js
>--- a/calendar/resources/content/clipboard.js
>+++ b/calendar/resources/content/clipboard.js
>@@ -217,32 +217,29 @@
>     switch (flavour.value) {
>         case "text/calendar":
>         case "text/unicode":
>-            // Moving this test up before processing
>             var destCal = getSelectedCalendar();
>             if (!destCal) {
>                 return;
>             }
>             var calComp = getIcsService().parseICS(data, null);
>-            var subComp = calComp.getFirstSubcomponent("ANY");
>-            while (subComp) {
>+            let compToIterate = (calComp.componentType == "VCALENDAR" ?
>+                                 cal.icalSubcompIterator(calComp) :
>+                                 { __iterator__: function() { yield calComp; } });
>+            for (let subComp in compToIterate) {
dto.

Comment 6

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

(In reply to comment #5)
> (From update of attachment 346314 [details] [diff] [review])
> Philipp, could you please debitrot this patch?
> 
> some comments reading it:
> 
> I am missing lots of Components.import statements for calUtils.jsm, e.g. in
> calItemBase.js. Import statements for calUtils.jsm ought to be cheap, because a
> module file is executed only once; exported symbols are shared among all
> importing scripts.

I'm a bit unsure how to proceed here. Imagine the following scenario:

/* File: calFooUtils.jsm. Does NOT need calUtils.jsm itself. */
var cal = {
  myFooHelper: function() { ... }

};
var EXPORTED_SYMBOLS = ["cal"];


/* Now, any random file that uses functions from calUtils and calFooUtils: */

Components.utils.import("resource://calendar/modules/calUtils.jsm");
Components.utils.import("resource://calendar/modules/calFooUtils.jsm");

Whats the contents of |cal| now? Is |cal| automatically expanded to contain both calFooUtils' stuff and calUtils' stuff? I would rather expect that calFooUtils' |cal| overwrites the one from calUtils.

I've seen the following code around, maybe its something we should rather do:

var cal = {};
Components.utils.import("resource://calendar/modules/calUtils.jsm", cal);
Components.utils.import("resource://calendar/modules/calFooUtils.jsm", cal);
/* Of course, the jsm's directly export the methods */




> >+        this.mProperties = [ prop for (prop in cal.icalPropIterator(calComp))
> >+                                  if (prop.propertyName != "VERSION" &&
> >+                                      prop.propertyName != "PRODID") ];
> I don't like this notation and prefer Array.filter().
As discussed, it won't work with Array.filter().


> >+        for (let recprop in cal.icalPropIterator(icalcomp)) {
> while you are here, please spend a switch-case
This became tricky since I needed to |continue| the outer loop. I've solved it by using a label for the for() loop and then |continue iterateProperty;| inside the switch.
s odd, you are getting the next property
> 
> >+                    yield (aWantKeys ? subcomp : [subcomp, subcomp.serializeToICS()]);
> The concept of wantKeys doesn't seem to match here. This serializes the ical
> component in most usages which is unnecessary.
In most usages you can just do:

for (let comp in cal.iterators.iterateSubcomponent) {}

This causes wantKeys to be true and onlt the subcomponent is returned. In the (admittedly unused) case of the caller wanting the serialized component, he can use for each(). I did this for consistancy to the other iterators.



> This iterator does not iterator over sub-sub-components, e.g. I can imagine it
> doesn't get VALARM components of a VCALENDAR.
> Either define/document the iterator more strictly or overwork it with options
> (e.g. incorporating the above mentioned deepIterator?).
I defined it to be more strict, I think you usually don't want to go through all sub-sub-components. I'll think about the deep iterator.


> I am not sure about the keye vs. non-keys handling on ical properties. I think
> the notion of key doesn't fit here, because a component could have multiple
> (even identical) properties.
> I think the iterator should only return the property objects one after another.
See comment above, if only the prop is needed then for() can be used, if (as it often happens) the prop and its value is needed, then for each() can be used.


Patch will follow as soon as I have fixed all other comments.

Comment 7

[Daniel Boelzle [:dbo]]

(In reply to comment #6)
> > module file is executed only once; exported symbols are shared among all
> > importing scripts.
> 
> I'm a bit unsure how to proceed here. Imagine the following scenario:
> 
> /* File: calFooUtils.jsm. Does NOT need calUtils.jsm itself. */
> var cal = {
>   myFooHelper: function() { ... }
> 
> };
> var EXPORTED_SYMBOLS = ["cal"];

The script actually *needs* calUtils.jsm, because it needs to hook into the |cal| object which is defined (and shared) by calUtils.jsm, so you need to import calUtils.jsm and must not redefine |cal|, but rather only add to it. As mentioned, calUtils.jsm is executed only once and the exported symbols are shared among importing code. Our helper modules (adding to |cal|) all need to export |cal| though (only). calItipUtils.jsm already works like this.

> /* Now, any random file that uses functions from calUtils and calFooUtils: */
> 
> Components.utils.import("resource://calendar/modules/calUtils.jsm");
> Components.utils.import("resource://calendar/modules/calFooUtils.jsm");
> 
> Whats the contents of |cal| now? Is |cal| automatically expanded to contain
> both calFooUtils' stuff and calUtils' stuff? I would rather expect that
> calFooUtils' |cal| overwrites the one from calUtils.
> 
> I've seen the following code around, maybe its something we should rather do:
> 
> var cal = {};
> Components.utils.import("resource://calendar/modules/calUtils.jsm", cal);
> Components.utils.import("resource://calendar/modules/calFooUtils.jsm", cal);
> /* Of course, the jsm's directly export the methods */
I don't think this is solid.

> > >+        for (let recprop in cal.icalPropIterator(icalcomp)) {
> > while you are here, please spend a switch-case
> This became tricky since I needed to |continue| the outer loop. I've solved it
> by using a label for the for() loop and then |continue iterateProperty;| inside
> the switch.
So it was actually a bug (because continue refers to the enclosing loop)? Please don't use labels, but conditions.

> > >+                    yield (aWantKeys ? subcomp : [subcomp, subcomp.serializeToICS()]);
> > The concept of wantKeys doesn't seem to match here. This serializes the ical
> > component in most usages which is unnecessary.
> In most usages you can just do:
> 
> for (let comp in cal.iterators.iterateSubcomponent) {}
> 
> This causes wantKeys to be true and onlt the subcomponent is returned. In the
> (admittedly unused) case of the caller wanting the serialized component, he can
> use for each(). I did this for consistancy to the other iterators.
I don't this is what's been intended by key/value: the ical component isn't a key nor is it's serialized ics string a value. If users need to iterate serialized ics, they ought to explicitly call serializeToICS().

> > I am not sure about the keye vs. non-keys handling on ical properties. I think
> > the notion of key doesn't fit here, because a component could have multiple
> > (even identical) properties.
> > I think the iterator should only return the property objects one after another.
> See comment above, if only the prop is needed then for() can be used, if (as it
> often happens) the prop and its value is needed, then for each() can be used.
As mentioned I don't think we should make a difference and prefer
  |for each (let prop in cal.ical.propertyIterator(icalComp))|.
  |for (let prop in cal.ical.propertyIterator(icalComp))| would work, too.

Comment 8

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

(In reply to comment #7)
> (In reply to comment #6)
> > > module file is executed only once; exported symbols are shared among all
> > > importing scripts.
I've done some simple tests and it doesn't seem to overwrite |cal| in any way, regardless of if the module file uses calUtils or not, so I'm fine with going the way you suggested.

I still think the use of |var cal = {}; Cu.import()| is more flexible since you can import the needed functions just where you want them, i.e importing into the global namespace in components where namespace conflicts are less likely and importing into a namespace like |cal| in chrome files where the conflicts are common. This is also more similar to the concept of C++ namespaces (using namespace cal vs. cal::foo()). I guess this is a different matter though.


> > > >+        for (let recprop in cal.icalPropIterator(icalcomp)) {
> > > while you are here, please spend a switch-case
> > This became tricky since I needed to |continue| the outer loop. I've solved it
> > by using a label for the for() loop and then |continue iterateProperty;| inside
> > the switch.
> So it was actually a bug (because continue refers to the enclosing loop)?
> Please don't use labels, but conditions.
No, there was no bug previously. The outer loop was intended to be continued, and that actually happened the way it was before. Introducing a switch/case inside the for() loop adds another loop that can be break/continued. I see no obvious way to use the for() and switch() together without using labels.


> I don't this is what's been intended by key/value: the ical component isn't a
> key nor is it's serialized ics string a value. If users need to iterate
> serialized ics, they ought to explicitly call serializeToICS().

I admit the naming key/value might be a bit confusing, this is taken from the object iterator where using for() or Iterator(obj, true) causes the iterator to return only the object's key, and using for each() or Iterator(obj, false) causes it to return an array [key, value]. I'm fine with renaming the parameter, but I would like to keep the concept of returning an array with something that resembles the thing being iterated as a whole and a shortcut to its value when the argument is false and just the iterated item when the argument is true.

> As mentioned I don't think we should make a difference and prefer
>   |for each (let prop in cal.ical.propertyIterator(icalComp))|.
>   |for (let prop in cal.ical.propertyIterator(icalComp))| would work, too.

Just think away the iterators for a minute, don't you agree that for () and for each () in other code locations do two different things? Why should they be doing the same thing here?

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
> (In reply to comment #7)
> I still think the use of |var cal = {}; Cu.import()| is more flexible since you
> can import the needed functions just where you want them, i.e importing into
> the global namespace in components where namespace conflicts are less likely
> and importing into a namespace like |cal| in chrome files where the conflicts
> are common. This is also more similar to the concept of C++ namespaces (using
> namespace cal vs. cal::foo()). I guess this is a different matter though.

I like that idea, but yet don't see a large benefit:
- we need to keep care to export every symbol separately (minor effort though)
- further nesting of scopes can get complicated and needs to be added to client code, e.g. |cal.ui.alarms|.
- for code brevity you could even use e.g. |const itip = cal.itip| at the top of your file like we do for |Components|. Sticking to the comparison with C++, this is kind of a namespace alias.

Admittedly you cannot import into global scope, but I (personally) discourage to do that. Referring to external symbols in a "qualified" way has the large benefit that code readers know where it comes from. I try to use unqualified symbols for local code only.
What I like most on the way I took is that there's a single unique path to every symbol; IMO a benefit when maintaining files over years. This requires careful thoughts about scoping symbols though. My experience is largely based on coding C++: Adding (multiple) |using namespace| clauses in C++ needs to be carefully considered, because you could run into lookup clashes you have to solve by explicitly qualifying symbols. This could always hit you even if you don't change your code, because included headers may change over time.

> > > > >+        for (let recprop in cal.icalPropIterator(icalcomp)) {
> > > > while you are here, please spend a switch-case
> > > This became tricky since I needed to |continue| the outer loop. I've solved it
> > > by using a label for the for() loop and then |continue iterateProperty;| inside
> > > the switch.
> > So it was actually a bug (because continue refers to the enclosing loop)?
> > Please don't use labels, but conditions.
> No, there was no bug previously. The outer loop was intended to be continued,
> and that actually happened the way it was before. Introducing a switch/case
> inside the for() loop adds another loop that can be break/continued. I see no
> obvious way to use the for() and switch() together without using labels.
I don't understand that yet, maybe you could just add the code and I comment on that.

> > I don't this is what's been intended by key/value: the ical component isn't a
> > key nor is it's serialized ics string a value. If users need to iterate
> > serialized ics, they ought to explicitly call serializeToICS().
> 
> I admit the naming key/value might be a bit confusing, this is taken from the
> object iterator where using for() or Iterator(obj, true) causes the iterator to
> return only the object's key, and using for each() or Iterator(obj, false)
> causes it to return an array [key, value]. I'm fine with renaming the
> parameter, but I would like to keep the concept of returning an array with
> something that resembles the thing being iterated as a whole and a shortcut to
> its value when the argument is false and just the iterated item when the
> argument is true.

A value would be ok, but there's no concept of a |key|, because the same property could occur multiple times.

> > As mentioned I don't think we should make a difference and prefer
> >   |for each (let prop in cal.ical.propertyIterator(icalComp))|.
> >   |for (let prop in cal.ical.propertyIterator(icalComp))| would work, too.
> 
> Just think away the iterators for a minute, don't you agree that for () and for
> each () in other code locations do two different things? Why should they be
> doing the same thing here?

They shouldn't. If it's possible, we should prevent the latter use of |for-without-each|. I think a |for-without-each| simply doesn't match for key-less iterations.
Another example: even though you would use a

  |for (let prop in cal.ical.propertyIterator(icalComp))|

to iterate the "keys", you can't do a lookup on the iterator object with that key.

BTW: mail code has introduced fixIterator to iterate xpcom arrays:

<http://mxr.mozilla.org/comm-central/source/mailnews/base/util/iteratorUtils.jsm#46>

and doesn't follow a key/value semantic either. Though it seems there's already mixed usage of |for| vs. |for each|:

<http://mxr.mozilla.org/comm-central/search?string=fixIterator&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central>. sigh :/

Comment 10

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

Attachment: Use Iterators - v3

(In reply to comment #9)
> A value would be ok, but there's no concept of a |key|, because the same
> property could occur multiple times.
Sure, the term "key" is a bit misguided here. I can rename the parameter aDontWantValue if you like ;-)

> to iterate the "keys", you can't do a lookup on the iterator object with that
> key.
You can't do lookups on iterators anyway, so I think its ok.

> and doesn't follow a key/value semantic either. Though it seems there's already
> mixed usage of |for| vs. |for each|:
I guess if you really want me to get rid of the way its working now and only use for each(), I can throw if used in a for() block. Although I'd still prefer the way it is now.


I decided to get rid of the iterating function in calUtils.js. Please check what I've done, it seems to me the old code there was a bit redundant (esp. in the itip code).

Comment 11

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

Attachment: Use Iterators - v4

Takes care of phone comments

Comment 12

[Daniel Boelzle [:dbo]]

Comment on attachment 348201 [details] [diff] [review]
Use Iterators - v4

I know it's convenient to attach patches right from mq, but please spend some more context, if possible qdiff -U 8.

>+cal.itemIterator = function cal_itemIterator(items) {
>+    return {
>+        __iterator__: function itemIterator(aWantKeys) {
>+            cal.ASSERT(aWantKeys, "Please use for() on the item iterator", true);
I think we should stay graceful here, and don't abort the iteration.

>+    calendarComponentIterator: function cal_ical_calendarComponentIterator(aComponent, aCompType) {

>+        let compType = (aCompType || "ANY");
I try to avoid similar variable names in the same body to avoid confusion. In this case I'd prefer to just correct the (optional) aCompType parameter:
if (!aCompType) {
    aCompType = "ANY";
}

Please spend a single higher level |if (aComponent) {| instead of checking each time
>+        if (aComponent && aComponent.componentType == "VCALENDAR") {
>+            return cal.ical.subcompentIterator(aComponent, compType);
>+        } else if (aComponent && aComponent.componentType == "XROOT") {
>+            function calVCALENDARIterator(aWantKeys) {
>+                for (let calComp in cal.ical.subcomponentIterator(aComponent), "VCALENDAR") {
The latter "VCALENDAR" doesn't fit in; I assume it should be passed to subcomponentIterator.

>+                    for (let itemComp in cal.ical.subcomponentIterator(calComp, compType)) {
>+                        yield (aWantKeys ? itemComp : [itemComp, itemComp.serializeToICS()]);
I remember we didn't want to allow to return tuples, but wanted to ASSERT(aWantKeys).

>+                    }
>+                }
>+            };
>+            return { __iterator__: calVCALENDARIterator };
>+        } else if (aComponent && (compType == "ANY" || compType == aComponent.componentType)) {
>+            return {
>+                __iterator__: function singleItemIterator(aWantKeys) {
>+                    yield aComponent;
>+                }
>+            }
>+        } else {
>+            return Iterator({});
>+        }
>+    },

>+    subcomponentIterator: function cal_ical_subcomponentIterator(aComponent, aSubcomp) {
>+        return {
>+            __iterator__: function icalSubcompIterator(aWantKeys) {
>+                cal.ASSERT(aWantKeys, "Please use for() on the subcomponent iterator", true);
be graceful here, too

>+    propertyIterator: function cal_ical_propertyIterator(aComponent, aProperty) {
>+        return {
>+            __iterator__: function icalPropertyIterator(aWantKeys) {
>+                cal.ASSERT(aWantKeys, "Please use for() on the property iterator", true);
dto.

>+    /**
>+     * Use to iterate through all parameters of a calIIcalProperty.
Please add some more doc w.r.t. for/for-each returning tuples of [name, value]...

>+     *
>+     * @param aProperty         The property to iterate.
>+     * @return                  An iterator object to iterate the properties.
>+     */
>+    paramIterator: function cal_ical_paramIterator(aProperty) {
"parameterIterator" for consistency? Up to you.

>diff --git a/calendar/base/modules/calItipUtils.jsm b/calendar/base/modules/calItipUtils.jsm
>-                    cal.calIterateIcalComponent(
>-                        item.icalComponent,
>-                        function(subComp) {
>-                            for (let prop = subComp.getFirstProperty("ANY");
>-                                 prop;
>-                                 prop = subComp.getNextProperty("ANY")) {
>-                                if (majorProps[prop.propertyName]) {
>-                                    propStrings.push(item.recurrenceId + "#" + prop.icalString);
>-                                }
>-                            }
>-                        },
>-                        cal.isEvent(item) ? "VEVENT" : "VTODO");
>+
>+                    for (let prop in cal.ical.propertyIterator(item.icalComponent)) {
>+                        if (prop.propertyName in majorProps) {
>+                            propStrings.push(item.recurrenceId + "#" + prop.icalString);
>+                        }
>+                    }
>                 }
>             }
>             addProps(aItem);
This (and the following code in the file) could now even be shorter using the cal.itemIterator, resolving local function hasMajorProps().

>diff --git a/calendar/base/src/calAttachment.js b/calendar/base/src/calAttachment.js
>+        for each (let [key,value] in this.mProperties) {
add space                    ^

>diff --git a/calendar/base/src/calAttendee.js b/calendar/base/src/calAttendee.js
>+        for each (let [key,value] in this.mProperties) {
dto.

>diff --git a/calendar/base/src/calIcsParser.js b/calendar/base/src/calIcsParser.js
>+        this.mProperties = [ prop for (prop in cal.ical.propertyIterator(calComp))
>+                                  if (prop.propertyName != "VERSION" &&
>+                                      prop.propertyName != "PRODID") ];

I don't yet see why we need to filter out VERSION and PRODID at all, because we're tagging every exported ics using calSetProdidVersion(). VERSION and PRODID may be useful to know for providers when importing ics.

>diff --git a/calendar/base/src/calItemBase.js b/calendar/base/src/calItemBase.js
>+        for each (let [propKey, propValue] in this.mProperties) {
>+            if (propValue instanceof Components.interfaces.calIDateTime) {
>+                if (propValue.isMutable) {
>+                    propValue.makeImmutable();
>+                }
use && for brevity?

>-            var att = new CalAttendee();
>+        for (let attprop in cal.ical.propertyIterator(icalcomp, "ATTENDEE")) {
>+            let att = new CalAttendee();
I don't see a reason why we shouldn't directly create |calAttendee|, since it's ever been in the same js scope. May save us some cycles.

>+        for (let attprop in cal.ical.propertyIterator(icalcomp, "ATTACH")) {
>+            let att = cal.createAttachment();
dto.

>+        for (let relprop in cal.ical.propertyIterator(icalcomp, "RELATED-TO")) {
>+            let rel = cal.createRelation();
dto.

>-            var org = new CalAttendee();
>+            let org = new CalAttendee();
dto.

>+        iterateProperty: for (let recprop in cal.ical.propertyIterator(icalcomp)) {
>+            let ritem = null;
>+            switch (recprop.propertyName) {
>+                case "RRULE":
>+                case "EXRULE":
>+                    ritem = new CalRecurrenceRule();
>+                    break;
>+                case "RDATE":
>+                case "EXDATE":
>+                    ritem = new CalRecurrenceDate();
>+                    break;
>+                default:
>+                    continue iterateProperty;
>+
^^ remove line feed

I yet don't see a reason to continue to label instead of using a plain |continue|. Could you please explain?

>+            } else {
>+                // Really, really old Sunbird/Calendar versions didn't give us a
>+                // trigger.
>+                Components.utils.reportError("No trigger property for alarm on item: " + this.id);
please use cal.ERROR()

>     importUnpromotedProperties: function (icalcomp, promoted) {
>-        for (var prop = icalcomp.getFirstProperty("ANY");
>-             prop;
>-             prop = icalcomp.getNextProperty("ANY")) {
>+        for (let prop in cal.ical.propertyIterator(icalcomp)) {
>             if (!promoted[prop.propertyName]) {
>                 this.setProperty(prop.propertyName, prop.value);
>-                var param = prop.getFirstParameterName();
>-                while (param) {
>+                for each (let [paramName, paramValue] in cal.ical.paramIterator(prop)) {
>                     if (!(prop.propertyName in this.mPropertyParams)) {
>                         this.mPropertyParams[prop.propertyName] = {};
>                     }
>-                    this.mPropertyParams[prop.propertyName][param] = prop.getParameter(param);
>-                    param = prop.getNextParameterName();
>+                    this.mPropertyParams[prop.propertyName][paramName] = paramValue;
>                 }
>             }
>         }

Reading that loop, I think we should define and use |let propName = prop.propertyName;| once at the beginning of the loop.

>diff --git a/calendar/base/src/calRelation.js b/calendar/base/src/calRelation.js
>+        for each (let [key,value] in this.mProperties) {
                             ^
>diff --git a/calendar/base/src/calTimezoneService.js b/calendar/base/src/calTimezoneService.js
>diff --git a/calendar/base/src/calUtils.js b/calendar/base/src/calUtils.js
>--- a/calendar/base/src/calUtils.js
>+++ b/calendar/base/src/calUtils.js
>@@ -984,6 +984,7 @@
>     }
> 
>     NS_ASSERT(aCondition, aMessage);
>+
I've removed NS_ASSERT anyway in a concurring patch. Please remove this hunk to not break it further.

> // implementation part of calPropertyBag
> function calPropertyBagEnumerator(bag) {
>     this.mIndex = 0;
>     this.mBag = bag;
>-    var keys = [];
>-    for (var key in bag.mData) {
>-        keys.push(key);
>-    }
>-    this.mKeys = keys;
>+    this.mKeys = [ key for (key in bag.mData) ];
closer braces, please

>diff --git a/calendar/providers/caldav/calDavCalendar.js b/calendar/providers/caldav/calDavCalendar.js

>+Components.utils.import("resource://calendar/modules/calUtils.jsm");
Although this import might not be needed (because it's imported by the module function already), but I think it's sensible to properly document what's been imported.

>diff --git a/calendar/providers/wcap/calWcapSession.js b/calendar/providers/wcap/calWcapSession.js
>-                forEachIcalComponent(
>-                    data, "VTIMEZONE",
>-                    function eachComp(subComp) {
>-                        try {
>-                            var tzid = subComp.getFirstProperty("TZID").value;
>-                            this_.m_serverTimezones[tzid] = new calWcapTimezone(this_, tzid, subComp);
>-                        } catch (exc) { // ignore but errors:
>-                            logError(exc, this_);
>-                        }
>-                    });
>+                for (let subComp in cal.ical.calendarItemIterator(data, "VTIMEZONE")) {
You are referring to |calendarItemIterator| instead of |calendarComponentIterator|; same applies in calDavCalendar.js, calWcapCalendarItems.js.

r=dbo

Comment 13

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

(In reply to comment #12)
> I try to avoid similar variable names in the same body to avoid confusion. In
> this case I'd prefer to just correct the (optional) aCompType parameter:
> if (!aCompType) {
>     aCompType = "ANY";
> }
I'd rather use a similar name and keep the argument as it is, although in js it might not be that bad, I don't want to be changing arguments.

> I remember we didn't want to allow to return tuples, but wanted to
> ASSERT(aWantKeys).
Sorry, slipped in.

> I don't yet see why we need to filter out VERSION and PRODID at all, because
> we're tagging every exported ics using calSetProdidVersion(). VERSION and
> PRODID may be useful to know for providers when importing ics.
I'll leave this to a different bug.

> I yet don't see a reason to continue to label instead of using a plain
> |continue|. Could you please explain?

Looks like my tests had typos, seems to work without the label now.


> I've removed NS_ASSERT anyway in a concurring patch. Please remove this hunk to
> not break it further.
Did the exact change you did in the other bug.


> >+    this.mKeys = [ key for (key in bag.mData) ];
> closer braces, please
I find it harder to read when making the braces closer. I'd like to keep it this way.


> >+Components.utils.import("resource://calendar/modules/calUtils.jsm");
> Although this import might not be needed (because it's imported by the module
> function already), but I think it's sensible to properly document what's been
> imported.
Document includes? I think this is too much :-)

Comment 14

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

Attachment: Use Iterators - v5 (patch as checked in)

Comment 15

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/9b7fe8e84e72>

-> FIXED

Comment 16

[Daniel Boelzle [:dbo]]

Attachment: regression fix

r=me, pushed: <http://hg.mozilla.org/comm-central/rev/75dcf3c6be22>

Comment 17

[[e-mail]]

Comment on attachment 348639 [details] [diff] [review]
Use Iterators - v5 (patch as checked in)

>+        return {
>+            __iterator__: function
Are you expecting these iterators to be used more than once? Otherwise you can just call the function, which returns a one-off iterator object.

Comment 18

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".