Open Bug 474219 Opened 12 years ago Updated 4 years ago

Use iteratorUtils.jsm in calendar

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: Fallen, Assigned: aryx, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

43.30 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
2.40 KB, patch
neil
: review+
Details | Diff | Splinter Review
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
This patch makes use of the iteratorUtils that are going to come to Sunbird in bug 474213.

mvl, maybe you could take a look at this one?
Attachment #357555 - Flags: review?(mvl)
Comment on attachment 357555 [details] [diff] [review]
Fix - v1

All usages of fixIterator look good to me. However, I don't think we should import its symbols into the |cal| namespace (may duplicate |cal| declarations lead to warnings BTW?).
I'd either like to use them as they come (imported into global space like in mail/mailnews code which I prefer, because usage looks similar) or import them into |cal|, but then part of calIteratorUtils.jsm.

>diff --git a/calendar/base/content/dialogs/calendar-event-dialog.js b/calendar/base/content/dialogs/calendar-event-dialog.js
>             // ... and add the attachment.
>-            var attachment = createAttachment();
>+            let  attachment = cal.createAttachment();
fix whitespace, too

>diff --git a/calendar/providers/gdata/components/calGoogleUtils.js b/calendar/providers/gdata/components/calGoogleUtils.js
>-        var enumerator = aItem.propertyEnumerator;
>-        while (enumerator.hasMoreElements()) {
>-            var prop = enumerator.getNext().QueryInterface(Components.interfaces.nsIProperty);
>+        var enumerator = ;
leftover?

>-    let enumerator = aAlarm.propertyEnumerator;
>+    let enumerator = ;
dto?

>diff --git a/calendar/providers/ics/calICSCalendar.js b/calendar/providers/ics/calICSCalendar.js
>         function purgeOldBackups() {
>             // Enumerate files in the backupdir for expiry of old backups
>-            var dirEnum = backupDir.directoryEntries;
>-            var files = [];
>-            while (dirEnum.hasMoreElements()) {
>-                var file = dirEnum.getNext().QueryInterface(CI.nsIFile);
>-                if (file.isFile()) {
>-                    files.push({name: file.leafName, lastmodified: file.lastModifiedTime});
>-                }
>-            }
>+            let files = [ { name: file.leafName, lastmodified: file.lastModifiedTime }
>+                          for (file in cal.fixIterator(dirEnum, CI.nsIFile))
>+                          if (file.isFile()) ];
tested?

>diff --git a/calendar/providers/storage/calStorageCalendar.js b/calendar/providers/storage/calStorageCalendar.js
>                 var props = "";
>                 var propEnum = att.propertyEnumerator;
>-                while (propEnum && propEnum.hasMoreElements()) {
>-                    var prop = propEnum.getNext().QueryInterface(Components.interfaces.nsIProperty);
>+                for (let prop in cal.fixIterator(propEnum, Components.interfaces.nsIProperty)) {
>                     if (props.length) {
>                         props += ",";
>                     }

let props = [encodeURIComponent(prop.name) + ":" + encodeURIComponent(prop.value)
             for (let prop in fixIterator(propEnum,
                                          Components.interfaces.nsIProperty))].join(",");
?

I occasionally see tests of enumerator against null. Should fixIterator() balance such cases out and return {}?

More context in patch files would be nice.

r=dbo with discussion about importing resolved
Attachment #357555 - Flags: review?(mvl) → review+
This needs to be postponed to after our milestone for 3.0b2
Whiteboard: [after our 3.0b2 miletone]
Whiteboard: [after our 3.0b2 miletone] → [after our 3.0b2 milestone]
Bug 474213 was just fixed, this needs to be postponed to after Thunderbird 3.0b4.
Whiteboard: [after our 3.0b2 milestone] → [after tb3.0b4]
Attached patch Fix - v2 (obsolete) β€” β€” Splinter Review
As this patch has had some bitrot, I'd love to see an extra set of eyes on this and especially some more testing.
Attachment #357555 - Attachment is obsolete: true
Attachment #405710 - Flags: review?(ssitter)
Whiteboard: [after tb3.0b4] → [needs review]
Attachment #405710 - Flags: review?(ssitter) → review?
Comment on attachment 405710 [details] [diff] [review]
Fix - v2

Archaeopteryx, this is along the lines of what you were doing with Services.jsm and mailServices.jsm. Would you like to possibly debitrot this patch and review it?
Attachment #405710 - Flags: review? → review?(archaeopteryx)
Attached patch switch to iteratorUtils.jsm, v3 (obsolete) β€” β€” Splinter Review
Mostly the same code as in the old patch, also replaced some if (string.substr(0, count) == "term") with string.startsWith("term")

By the way, how do I have to alter the build config on try to
a) build Lightning, and
b) run Lightning tests?

It didn't work the old way, see https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=33cbfff8de11
Assignee: philipp → archaeopteryx
Attachment #405710 - Attachment is obsolete: true
Attachment #405710 - Flags: review?(archaeopteryx)
Attachment #687367 - Flags: review?(philipp)
(In reply to Archaeopteryx [:aryx] from comment #6)
> By the way, how do I have to alter the build config on try to
> a) build Lightning, and
Unless I broke something, all you need to do is add a mozconfig-extra that contains ac_add_options --enable-calendar. I don't know what the newest way to add mozconfig options to all platforms is, you might want to ask someone in #maildev. If in doubt, you can always edit all the mozconfigs in mail/configs/mozconfig to include that option. The way you did it looks fine too.


> b) run Lightning tests?
I'm working on hooking these up, I don't think they are run automatically. You could try to remove the skip-if statement here: http://mxr.mozilla.org/comm-central/source/mail/test/xpcshell.ini#24


> 
> It didn't work the old way, see
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=33cbfff8de11

This looks like m-c updated clang again, should be working when Mark syncs the manifests again.
Comment on attachment 687367 [details] [diff] [review]
switch to iteratorUtils.jsm, v3

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

r=philipp with these changes and a test run. I haven't gotten around to actually trying the patch so I'd appreciate if you could take care of that.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +1736,5 @@
>          return;
>      }
>  
>      // Create the attachment
> +    for (let file in fixIterator(files, Components.interfaces.nsILocalFile)) {

Does fixIterator swallow null values?

::: calendar/timezones/update.js
@@ +14,5 @@
>   *
>   * args: <path-to-zones.tab-file> <moz-root> <version>
>   */
>  
> ++Components.utils.import("resource://gre/modules/iteratorUtils.jsm");

Looks like there is an extra + at the beginning here.
Attachment #687367 - Flags: review?(philipp) → review+
Whiteboard: [needs review]
iteratorUtils.jsm is not in //gre/modules/ but ///modules/ .
Also, where youa re touching nsILocalFile, you can probably replace it with nsIFile, if the reviewer agrees.
Archaeopteryx, whats the status on this one? Can you fix the nits and push it? I'm fine with switching to nsIFile if you like.
Flags: needinfo?(archaeopteryx)
Attached patch patch, v4 (obsolete) β€” β€” Splinter Review
This is the updated patch. I ran tests and saw your comment about fixIterator and null later: It chokes on null as parameter, so I added an additional test to iteratorUtils.jsm. What should be done next? Ask someone (Neil) for review?
Attachment #687367 - Attachment is obsolete: true
Flags: needinfo?(archaeopteryx) → needinfo?(philipp)
What about my comment 9?
nsILocalFile: We should do this in a separate patch (in my opinion).

resource:///modules/: will update the patch
Attached patch patch, v5 (obsolete) β€” β€” Splinter Review
Attachment #8385283 - Attachment is obsolete: true
Yes, a mailnews reviewer for the mailnews part, I can do the rest. Thanks for updating the patch!
Flags: needinfo?(philipp)
This patch makes iteratorUtils.jsm check for null as argument.
Attachment #8385321 - Attachment is obsolete: true
Attachment #8393001 - Flags: review?(neil)
Attached patch calendar part, v6 (obsolete) β€” β€” Splinter Review
Attachment #8393002 - Flags: review?(philipp)
Comment on attachment 8393002 [details] [diff] [review]
calendar part, v6

Calendar part looks good. Since this patch has been in limbo for a while I'm fine with keeping it as is, but changed could should prefer the for..of construct if possible. Its up to you if you want to change it. Note I haven't actually tested if for..of works with fixIterator.
Attachment #8393002 - Flags: review?(philipp) → review+
Comment on attachment 8393001 [details] [diff] [review]
iteratorUtils.jsm, check for null as argument, v1

I thought for (x in null) would throw, but apparently only for (x of null) does, so it looks like you're good.
Attachment #8393001 - Flags: review?(neil) → review+
Archaeopteryx, what about those patches having r+? Are they ready to land on comm-central?
Flags: needinfo?(archaeopteryx)
The extension of iteratorUtils.jsm is needed to support calPropertyBagEnumerator.
Attachment #8423008 - Flags: review?(neil)
Flags: needinfo?(archaeopteryx)
Attached patch calendar part, v7 β€” β€” Splinter Review
This passes mozmill and xpcshell tests locally.

Successful Try run for the mailnews part: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=0664c67316c3
Attachment #8393001 - Attachment is obsolete: true
Attachment #8393002 - Attachment is obsolete: true
Attachment #8423010 - Flags: review?(philipp)
(In reply to Archaeopteryx from comment #21)
> The extension of iteratorUtils.jsm is needed to support
> calPropertyBagEnumerator.

OK, but why the null-check?
(In reply to neil@parkwaycc.co.uk from comment #23)
> OK, but why the null-check?
Because we don't check for null when calling iteratorUtils.jsm functions (which would be a mess in |for ... in fixIterator ...| constructs). The former nsISimpleEnumerator check has it because the functions returns null for null at its bottom. If you want, I can change it to a null check and early return on the top.
Attached patch iteratorUtils.jsm, v4 (obsolete) β€” β€” Splinter Review
aceman asked me why I added the null check, but I don't remember anymore. I likely catched some fallout of other issues at that time which got fixed later. So I removed the null-check (calendar xpcshell and mozmill tests passed locally).
Attachment #8423008 - Attachment is obsolete: true
Attachment #8423008 - Flags: review?(neil)
Attachment #8423431 - Flags: review?(neil)
Attached patch iteratorUtils.jsm, v5 β€” β€” Splinter Review
Improved the comments.
Attachment #8423431 - Attachment is obsolete: true
Attachment #8423431 - Flags: review?(neil)
Comment on attachment 8424329 [details] [diff] [review]
iteratorUtils.jsm, v5

[On a side note, anyone know why we return { __iterator__: iter }; instead of return iter(); ? ]
Attachment #8424329 - Flags: review?(neil) → review+
[On another side note, [x for (x of iterator)] works for SpiderMonkey and ES6 iterators but throws a type error for __iterator__: iterators, while [x for (x in iterator)] works for SpiderMonkey and __iterator__: iterators, and hangs for ES6 iterators...]
(In reply to neil@parkwaycc.co.uk from comment #27)
> [On a side note, anyone know why we return { __iterator__: iter }; instead
> of return iter(); ? ]

Maybe something to do with the fact we use this in "for (x in fixIterator)" loops instead of "for (x of fixIterator)". That may be better, but would need to convert all the callers. I think we discussed this in another bug. But I am just guessing now.
Comment on attachment 8423010 [details] [diff] [review]
calendar part, v7

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

r=philipp pending the for..of vs for..in discussion.

::: calendar/base/src/calTimezoneService.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +Components.utils.import("resource:///modules/iteratorUtils.jsm");

I'd suggest mixing this import between the gre imports and adding a blank line between calendar and gre imports.
Attachment #8423010 - Flags: review?(philipp) → review+
Aryx, what is needed to finish this?
Fallen, do you really want to open that 'for..of' question in this bug? The rest of c-c uses for..in for now as nobody was able to decide whether to rework the iterator utils. I think it should be a bug of its own that converts whole c-c (you probably can't make calendar have the new syntax while the rest is on the old one).
Flags: needinfo?(philipp)
I'm fine with using for..in now and switching to for..of later on if that will speed up this bug.
Flags: needinfo?(philipp) → needinfo?(archaeopteryx)
(In reply to :aceman from comment #31)
> Aryx, what is needed to finish this?
1. Working build environment on Windows with MSVC 2013, please.
2. No other calendar tests failing and hiding the fails from my changes.
3. The remaining issue are with iterators using nsIProperty, if I remember correct.
Flags: needinfo?(archaeopteryx)
Bug 1123117 enabled use 'for..of' in addition to 'for..of' with fixIterator. So please guys decide which one to use and the patch can hopefully progress.

(In reply to Archaeopteryx [:aryx] from comment #33)
> (In reply to :aceman from comment #31)
> > Aryx, what is needed to finish this?
> 1. Working build environment on Windows with MSVC 2013, please.
> 2. No other calendar tests failing and hiding the fails from my changes.
The try server seems green currently, maybe it is right time now to test the patch there?

> 3. The remaining issue are with iterators using nsIProperty, if I remember
What is the problem here? Which comment talks about this?
Flags: needinfo?(archaeopteryx)
Comment on attachment 8424329 [details] [diff] [review]
iteratorUtils.jsm, v5

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

::: mailnews/base/util/iteratorUtils.jsm
@@ +111,5 @@
>      return { __iterator__: iter };
>    }
>  
> +  // How about nsISimpleEnumerator or similar? This one is nice and simple.
> +  if (aEnum.hasMoreElements && aEnum.getNext) {

In JS strict mode, won't this warn about undefined property .hasMoreelement and .getNext on objects that do not have it? I'd better like 
if (("hasMoreElements" in aEnum) && ("getNext" in aEnum))
if that works.
I think for..of is working now with fixIterator. Aryx, can you try to finish this?
Comment on attachment 8423010 [details] [diff] [review]
calendar part, v7

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

Please use for..of now at all places now.

Aryx, can you finish this? The try servers are in good enough shape now.

::: calendar/providers/ics/calICSCalendar.js
@@ +679,5 @@
>              // Enumerate files in the backupdir for expiry of old backups
>              var dirEnum = backupDir.directoryEntries;
> +            let files = [ { name: file.leafName, lastmodified: file.lastModifiedTime }
> +                          for (file in fixIterator(dirEnum, CI.nsIFile))
> +                          if (file.isFile()) ];

I'm not sure this syntax still works. Better would be to make it a normal loop.
Comment on attachment 8424329 [details] [diff] [review]
iteratorUtils.jsm, v5

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

::: mailnews/base/util/iteratorUtils.jsm
@@ +120,5 @@
>      return { __iterator__: iter };
>    }
>  
> +  // How about nsIUTF8StringEnumerator? This one is nice and simple.
> +  if (aEnum instanceof Ci.nsIUTF8StringEnumerator) {

I think there are some concerns about the ordering of these checks in bug 1115954.

@@ +125,5 @@
> +    let iter = function () {
> +      while (aEnum.hasMore())
> +        yield aEnum.getNext();
> +    }
> +    return { __iterator__: iter };

This syntax has changed in current iteratorUtils.js
Aryx, do you think you could look into this again?
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.