Last Comment Bug 725907 - Change for-of loop to work in terms of .iterator() and .next()
: Change for-of loop to work in terms of .iterator() and .next()
Status: RESOLVED FIXED
[DocArea=JS]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on: 749010 771027 907077
Blocks: 775781 725909 768692
  Show dependency treegraph
 
Reported: 2012-02-09 18:03 PST by Jason Orendorff [:jorendorff]
Modified: 2014-12-09 08:36 PST (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject (37.39 KB, patch)
2012-04-20 13:00 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 2 - Make for-of loops just call .iterator(), v1 (44.17 KB, patch)
2012-04-20 13:23 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 3 - Add .next() method to iterator objects and make for-of call it, v1 (49.51 KB, patch)
2012-04-20 13:41 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 4 - make findReferences accept string arguments and add some gc tests, v1 (3.43 KB, patch)
2012-04-20 13:42 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 5 - make ctypes arrays iterable, v1 (1.49 KB, patch)
2012-04-20 13:43 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject, v2 (36.68 KB, patch)
2012-06-13 11:29 PDT, Jason Orendorff [:jorendorff]
jorendorff: review+
Details | Diff | Splinter Review
Part 2 - Make for-of loops just call .iterator(), WIP 2 (42.40 KB, patch)
2012-06-13 11:33 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 3 - Add .next() method to iterator objects and make for-of call it, WIP 2 (57.50 KB, patch)
2012-06-13 11:34 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 4 - make findReferences accept string arguments and add some gc tests, WIP 2 (3.51 KB, patch)
2012-06-13 11:35 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 5 - make ctypes arrays iterable, WIP 2 (1.49 KB, patch)
2012-06-13 11:41 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject, v3 (37.23 KB, patch)
2012-06-15 13:22 PDT, Jason Orendorff [:jorendorff]
jorendorff: review+
Details | Diff | Splinter Review
Part 2 - Make for-of loops just call .iterator(), v3 (42.40 KB, patch)
2012-06-15 13:23 PDT, Jason Orendorff [:jorendorff]
bhackett1024: review+
Details | Diff | Splinter Review
Part 2a - Implement .iterator() for arraylike DOM objects (7.29 KB, patch)
2012-06-15 14:09 PDT, Jason Orendorff [:jorendorff]
bzbarsky: review+
Details | Diff | Splinter Review
Part 3 - Add .next() method to iterator objects and make for-of call it, v3 (40.36 KB, patch)
2012-06-15 14:09 PDT, Jason Orendorff [:jorendorff]
bhackett1024: review+
Details | Diff | Splinter Review
Part 4 - make findReferences accept string arguments and add some gc tests, v3 (3.51 KB, patch)
2012-06-15 14:12 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 5 - make ctypes arrays iterable, v3 (1.49 KB, patch)
2012-06-15 14:13 PDT, Jason Orendorff [:jorendorff]
bhackett1024: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-02-09 18:03:54 PST
Cc-ing some randomly chosen DOM and WebIDL people in hopes of getting free advice.

The for-of loop (added in bug 699565) is currently fairly magical. The iterator objects are not exposed to JS code. It is impossible for plain old Objects to be for-of-iterable; the only way for a user-defined object to be iterable is if it's a Proxy.

I've talked this over very briefly with dherman and samth, and I think we can make this simpler and at the same time a lot more user-friendly.

I propose that the for-of loop:

    for (V of EXPR)
        STMT

work like this:

    {
        let %iter = (EXPR).iterator();
        for (;;) {
            let %val;
            try {
                %val = %iter.next();
            } catch (exc) {
                if (exc instanceof StopIteration)
                    break;
                else
                    throw exc;
            }

            V = %val;
            STMT
        }
    }

where %val and %iter are different from all other identifiers.

With this change, it's easy to make an object iterable. Give it an iterator() method. We would define Array.prototype.iterator and so on--and also for the DOM.
Comment 1 Cameron McCormack (:heycam) 2012-02-10 17:17:01 PST
Less magic sounds better to me.  We could easily have Web IDL require all objects with indexed properties (like NodeList) to have an iterator() function on their prototype.  Sounds like a no-brainer in terms of usefulness.  (On Window too?  Maybe...)
Comment 2 Jason Orendorff [:jorendorff] 2012-04-10 13:27:29 PDT
Implementation notes.

JSOP_ITER will change to call .iterator(). The iternext/itermore opcodes already call .next as a fallback; it'll just become the normal behavior in the for-of case. This will be slow, for now. Type inference could help a lot.

Array.prototype.iterator will be added. It'll return an Iterator object with proto Iterator.prototype. The native function Iterator.prototype.next will be changed to be slightly generic, so that it'll work with Array iterators, generator iterators, and other types (like Map and Set iterators; see bug 725909), though not with arbitrary objects.

Proxy::iteratorNext and ProxyHandler::iteratorNext go away; next is an ordinary method call now.
Comment 3 Jason Orendorff [:jorendorff] 2012-04-20 13:00:44 PDT
Created attachment 617074 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject

This patch doesn't affect user-visible behavior. Just a little C++ refactoring and moving stuff around.

I considered calling this "Enumerator" but PropertyIteratorObject is precisely what it is. Note that this kind of object is sometimes exposed to script (if you call Iterator({a:1, b:2}) you get a property iterator object; ES6 will probably change the API but still expose such iterators somehow) and sometimes not (a for-in loop makes an iterator of the same Class, but it lives in a stack slot, has getProto() == NULL, and doesn't escape).
Comment 4 Jason Orendorff [:jorendorff] 2012-04-20 13:03:37 PDT
I just realized there's a pretty big bug in part 2 of this work. I will have to come back to it in 2 weeks since all next week is dedicated to debugger.
Comment 5 Jason Orendorff [:jorendorff] 2012-04-20 13:23:37 PDT
Created attachment 617076 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), v1

Just for fun. The bug is that I didn't add an iterator method to the DOM classes that need one. bz already pointed out how to do it, I just forgot, and haven't submitted this stack to the try server yet.
Comment 6 Jason Orendorff [:jorendorff] 2012-04-20 13:41:26 PDT
Created attachment 617080 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, v1

This is the main part, though it's not much bigger than part 1 or part 2...

The following changes are happening in this patch:

  - Up to now, for-of iteration was achieved by js_IteratorNext having
    magical knowledge of certain kinds of iterator object. With this patch,
    those special cases are removed and js_IteratorNext just calls iter.next().
    Simple sane semantics.

    Well, except for PropertyIteratorObjects, which are still special-cased.
    But that is strictly for speed (for-in is still the common case here
    and the only thing that shows up on benchmarks).

  - All iterators inherit from Iterator.prototype, which already has a .next()
    method. This method just calls js_IteratorNext. But now that would be
    circular, so I changed it to call a class hook:
      iterobj->getClass()->ext.iteratorNext
    If you try to do Iterator.prototype.next.call(obj) on an object that
    has a NULL iteratorNext class hook, it'll throw a TypeError at you.
    Not that you ever would.

  - PropertyIteratorObjects and ElementIteratorObjects each have an
    iteratorNext hook that gets the next property/element.

  - Wrapper::iteratorNext was really buggy. The replacement does the obvious
    thing and just delegates to the wrapped object's iteratorNext class hook.
    This also adds a "next" trap for scripted proxies.

Some of these design decisions are pretty subtle. Things may change later as ES6 coalesces. This is a snapshot of the current design. Certainly having for-of treat everything as a plain old object and call plain old .iterator() and .next() methods is a simplicity win for users; I don't expect TC39 to back away from that.

This will also help me make Maps and Sets iterable, which I'm eager to finish (bug 725909).
Comment 7 Jason Orendorff [:jorendorff] 2012-04-20 13:42:54 PDT
Created attachment 617081 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v1
Comment 8 Jason Orendorff [:jorendorff] 2012-04-20 13:43:44 PDT
Created attachment 617082 [details] [diff] [review]
Part 5 - make ctypes arrays iterable, v1
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-27 18:52:51 PDT
Comment on attachment 617074 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject

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

::: js/src/jsiter.cpp
@@ +486,3 @@
>      }
>  
> +    return (PropertyIteratorObject *) NewBuiltinClassInstance(cx, &PropertyIteratorObject::class_);

Use &asPropertyIterator(), please, like above.

::: js/src/jsiter.h
@@ +100,5 @@
>  
>      void mark(JSTracer *trc);
>  };
>  
> +class PropertyIteratorObject : public JSObject {

{ on new line per style guide.

::: js/src/jsobjinlines.h
@@ +49,5 @@
>  #include "jscntxt.h"
>  #include "jsdate.h"
>  #include "jsfun.h"
>  #include "jsgcmark.h"
> +#include "jsinterp.h"

Why this addition?
Comment 10 Jason Orendorff [:jorendorff] 2012-06-01 00:53:35 PDT
I need to add a few lines of code here to finish this bug:

https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dombindingsgen.py?rev=7082192622e6#717
Comment 11 Jason Orendorff [:jorendorff] 2012-06-13 11:29:03 PDT
Created attachment 632779 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject, v2

Unbitrotted, carrying forward review.
Comment 12 Jason Orendorff [:jorendorff] 2012-06-13 11:32:17 PDT
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #9)
> Use &asPropertyIterator(), please, like above.

Good idea. Done.

> > +class PropertyIteratorObject : public JSObject {
> { on new line per style guide.

OK.

> > +#include "jsinterp.h"
> Why this addition?

I dunno. It works fine without it, so I removed it again.
Comment 13 Jason Orendorff [:jorendorff] 2012-06-13 11:33:48 PDT
Created attachment 632783 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), WIP 2

What comment 10 said still applies to this patch. It needs more work (and tests) in order not to break for-of on arraylike DOM objects.
Comment 14 Jason Orendorff [:jorendorff] 2012-06-13 11:34:42 PDT
Created attachment 632784 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, WIP 2

Unbitrotted, but this is the wrong approach and I'm going to reimplement it almost from scratch.
Comment 15 Jason Orendorff [:jorendorff] 2012-06-13 11:35:17 PDT
Created attachment 632785 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, WIP 2

Unbitrotted.
Comment 16 Jason Orendorff [:jorendorff] 2012-06-13 11:41:30 PDT
Created attachment 632792 [details] [diff] [review]
Part 5 - make ctypes arrays iterable, WIP 2

Same as v1 actually, I think.
Comment 17 Jason Orendorff [:jorendorff] 2012-06-15 13:22:12 PDT
Created attachment 633646 [details] [diff] [review]
Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject, v3

Carrying r+ forward again.
Comment 18 Jason Orendorff [:jorendorff] 2012-06-15 13:23:03 PDT
Created attachment 633647 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), v3
Comment 19 Jason Orendorff [:jorendorff] 2012-06-15 14:09:09 PDT
Created attachment 633666 [details] [diff] [review]
Part 2a - Implement .iterator() for arraylike DOM objects

Things to look out for:

- Are there any arraylike DOM objects that don't get the new bindings, or don't have an IndexGetter?

- Are there any other DOM objects that should be iterable?

- Are there cases where one prototype object for an iterable interface inherits from another, so that we're pointlessly shadowing that .iterator method with another .iterator?
Comment 20 Jason Orendorff [:jorendorff] 2012-06-15 14:09:53 PDT
Created attachment 633667 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, v3
Comment 21 Jason Orendorff [:jorendorff] 2012-06-15 14:12:21 PDT
Created attachment 633668 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v3

Jim's not around, but that's OK. This part can wait a week or three.
Comment 22 Jason Orendorff [:jorendorff] 2012-06-15 14:13:42 PDT
Created attachment 633671 [details] [diff] [review]
Part 5 - make ctypes arrays iterable, v3
Comment 23 Jason Orendorff [:jorendorff] 2012-06-15 14:27:55 PDT
dev-doc-needed: Note that what it currently says here is already wrong:
https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of

It says that for-of loops get property values. That isn't right. 'for (x in obj)' iterates over the names of obj's properties. But 'for (x of obj)' will call obj.iterator() and loop over the values produced by that iterator's .next() method. This is almost exactly how Python's for-in loop works, only instead of a .iterator() method, Python has a .__iter__() method.

When this bug lands, users will be able to write their own iterators in order to make the for-of loop do whatever they want. For example:

  var obj = {iterator: function () { yield 1; yield 2; yield 3; }};
  for (var x of obj)
      print(x);

This prints 1, then 2, then 3.

Also, any object that inherits from Array.prototype is automatically iterable, just because Array.prototype has a .iterator method on it.

  var obj = Object.create(Array.prototype);
  obj.length = 3;
  obj[0] = 'A';
  obj[1] = 'B';
  obj[2] = 'C';

  for (var x of obj)
      print(x);

This prints A, then B, then C.
Comment 24 Brian Hackett (:bhackett) 2012-06-18 08:40:47 PDT
Comment on attachment 633647 [details] [diff] [review]
Part 2 - Make for-of loops just call .iterator(), v3

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

::: js/src/jsatom.tbl
@@ +53,5 @@
>  DEFINE_ATOM(index, "index")
>  DEFINE_ATOM(input, "input")
>  DEFINE_ATOM(toISOString, "toISOString")
> +DEFINE_ATOM(iterator, "iterator")
> +DEFINE_ATOM(iterator_, "__iterator__")

I think that iterator_ should have a more verbose/ugly name to distinguish it from the 'iterator' proper, maybe iteratorIntrinsic?
Comment 25 Brian Hackett (:bhackett) 2012-06-18 09:10:23 PDT
Comment on attachment 633667 [details] [diff] [review]
Part 3 - Add .next() method to iterator objects and make for-of call it, v3

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

::: js/src/jsiter.cpp
@@ +841,5 @@
> +    GlobalObject *global = GetCurrentGlobal(cx);
> +    RootedObject proto(cx, global->getOrCreateElementIteratorPrototype(cx));
> +    if (!proto)
> +        return NULL;
> +    RootedObject iterobj(cx, NewObjectWithGivenProto(cx, &class_, proto, global));

The root on iterobj itself isn't necessary, as setReservedSlot cannot trigger a GC.

@@ +872,5 @@
> +        obj = ValueToObject(cx, target);
> +        if (!obj)
> +            return false;
> +        if (!js_GetLengthProperty(cx, obj, &length))
> +            goto error;

The semantics here look weird and this at least needs a comment.  What are the conditions when it is OK to just propagate failures vs. necessary to close the iterator down?  Also, the error label should be renamed to closeIterator or something.

::: js/src/jsiter.h
@@ +97,5 @@
> + *      to any other object to create iterators over that object's elements.
> + *
> + *    - The next() method of an Array iterator is non-reentrant. Trying to reenter,
> + *      e.g. by using it on an object with a length getter that calls it.next() on
> + *      the same iterator, causes a TypeError.

How do we ensure that next() is non-reentrant?  ElementIteratorObject::next doesn't seem to do any guarding.  Either way, a comment in the implementation about this would be good.
Comment 26 Jason Orendorff [:jorendorff] 2012-06-19 18:05:15 PDT
(In reply to Brian Hackett (:bhackett) from comment #25)
> The root on iterobj itself isn't necessary, as setReservedSlot cannot
> trigger a GC.

Removed.

You found two bugs in ElementIteratorObject::next():

> The semantics here look weird and this at least needs a comment. [...]

Bug 1. Easily fixed. I changed it so that any time we return false, we do it via 'goto close;'.

> How do we ensure that next() is non-reentrant?

Bug 2. This one's a bit more work; I'll fix (and test) in a separate patch.

For the time being, re-entering will not cause anything horrible to happen, it'll just behave contrary to spec in some corner cases (and honestly there is no spec, the comment is sort of a guess).

BTW, both bugs have the same cause: I turned the old iteratorNext() code into a script-visible .next() method without re-reading it closely. And I had, perhaps foolishly, optimized iteartorNext() with the knowledge that iterator objects were not script-visible. Oops.

Thanks.
Comment 27 Brian Hackett (:bhackett) 2012-06-19 19:54:28 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #26)
> For the time being, re-entering will not cause anything horrible to happen,
> it'll just behave contrary to spec in some corner cases (and honestly there
> is no spec, the comment is sort of a guess).

Hmm, if the spec doesn't insist on this, it might be better to just leave this as is and fix the later comment to not discuss reentrancy.  The behavior looks fine to me in the reentrant case, some implementation details exposed for sure but oh well.
Comment 28 Jason Orendorff [:jorendorff] 2012-06-20 09:11:41 PDT
(In reply to Brian Hackett (:bhackett) from comment #27)
> Hmm, if the spec doesn't insist on this, it might be better to just leave
> this as is and fix the later comment to not discuss reentrancy.  The
> behavior looks fine to me in the reentrant case, some implementation details
> exposed for sure but oh well.

Yup. I agree and I went through and re-spec'd it that way
http://wiki.ecmascript.org/doku.php?id=harmony:iterators#arrayiteratorprototype_.next
and changed the comment.
Comment 29 Boris Zbarsky [:bz] 2012-06-20 23:01:19 PDT
Comment on attachment 633666 [details] [diff] [review]
Part 2a - Implement .iterator() for arraylike DOM objects

r=me.  Sorry for the lag....
Comment 32 Jason Orendorff [:jorendorff] 2012-07-04 09:11:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44eca03418c

One more to go. Please keep open still.
Comment 33 Jim Blandy :jimb 2012-07-04 16:19:39 PDT
Comment on attachment 633668 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v3

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

I think you'll also need to include strings in the array of roots maintained at the top of HeapReverser::traverseEdge. HeapReverser::nodeToValue needs to be fixed as well.

I'm also surprised ReferenceFinder::representable didn't need to be changed. Until it is, I think findReferences will fail to notice ropes referring to their left and right halves.
Comment 34 Jim Blandy :jimb 2012-07-04 16:22:40 PDT
Comment on attachment 633668 [details] [diff] [review]
Part 4 - make findReferences accept string arguments and add some gc tests, v3

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

I think you'll also need to include strings in the array of roots maintained at the top of HeapReverser::traverseEdge. HeapReverser::nodeToValue needs to be fixed as well.

I'm also surprised ReferenceFinder::representable didn't need to be changed. Until it is, I think findReferences will fail to notice ropes referring to their left and right halves.

::: js/src/jit-test/tests/for-of/string-iterator-gc.js
@@ +1,4 @@
> +// String iterators keep the underlying string alive.
> +
> +load(libdir + "referencesVia.js");
> +var song = Array(17).join("na") + "batman!";

You've *got* use use the NaN trick here!!!
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-07-04 16:24:55 PDT
https://hg.mozilla.org/mozilla-central/rev/f44eca03418c
Comment 36 :Ehsan Akhgari 2012-07-04 16:53:04 PDT
This bug, or something which was merged to mozilla-central in the same push, caused a permanent orange on Linux32 mochitest-chrome for builds without frame pointers (example log: <https://tbpl.mozilla.org/php/getParsedLog.php?id=13227357&tree=Profiling&full=1>).  This is important because framepointers are not enabled on Aurora, so this permaorange will appear there on the next uplift.  I backed out this patch as part of the rest of the js and xpconnect patches in the same merge push.  Please test this patch locally (and push to the try server if needed by taking out the --enable-profiling command from the Linux32 mozconfig) and reland when you make sure that it's not the cause behind the perma-orange.  I apologize in advance for the inconvenience in case this patch is not at fault.

Backout changeset:
https://hg.mozilla.org/mozilla-central/rev/b9c98f0d0fde
https://hg.mozilla.org/mozilla-central/rev/0d2b03dff288
https://hg.mozilla.org/mozilla-central/rev/aadf6091245b
https://hg.mozilla.org/mozilla-central/rev/86cf7f8a124a
https://hg.mozilla.org/mozilla-central/rev/015cbcaf8552
Comment 37 :Ehsan Akhgari 2012-07-04 17:17:52 PDT
In order to test whether this patch is at fault, push it to try with --enable-profiling removed from browser/config/mozconfigs/linux32/nightly, and if you get a green Linux Moth run, you're good to go!

And in turn you can reproduce the crash on try with your patch, I'd suggest you keep the possibility of having hit a compiler bug in mind.  :-)
Comment 38 :Ehsan Akhgari 2012-07-04 19:23:11 PDT
Also, note that these backouts indeed made the Linux32 Moth runs on the profiling branch green again, so we can be fairly certain that one of the backed out patches was at fault.
Comment 39 Jason Orendorff [:jorendorff] 2012-07-19 03:55:03 PDT
I have been trying to land this for weeks now.

The backouts in comment 36 were due to the craziness in bug 749010, but my latest Try Server runs also show some orange.
  https://tbpl.mozilla.org/?tree=Try&rev=db99191b1353
On OSX only, the browser doesn't start. No clue why, no info in the log. On my machine (also a mac), a debug browser build runs fine.
Comment 40 Luke Wagner [:luke] 2012-07-19 08:40:28 PDT
Try again?
Comment 42 Jim Blandy :jimb 2012-10-05 17:46:05 PDT
Can this bug be closed, then?
Comment 43 :Ms2ger (⌚ UTC+1/+2) 2013-08-21 06:06:27 PDT
(In reply to Jim Blandy :jimb from comment #42)
> Can this bug be closed, then?
Comment 44 Jason Orendorff [:jorendorff] 2013-08-22 16:01:03 PDT
Yes. This is done.

Note You need to log in before you can comment on or make changes to this bug.