Last Comment Bug 676936 - Refactor Iterator/Generator/StopIteration initialization
: Refactor Iterator/Generator/StopIteration initialization
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks: 675520
  Show dependency treegraph
 
Reported: 2011-08-05 13:59 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-08-09 09:00 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial rearrangement of js_InitIterator, clarification of Iterator startup (3.41 KB, patch)
2011-08-05 13:59 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Review
Simplification of Generator startup (1.53 KB, patch)
2011-08-05 14:00 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Review
Simplification of StopIteration startup (1.17 KB, patch)
2011-08-05 14:01 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-05 13:59:54 PDT
Created attachment 551149 [details] [diff] [review]
Initial rearrangement of js_InitIterator, clarification of Iterator startup
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-05 14:00:28 PDT
Created attachment 551150 [details] [diff] [review]
Simplification of Generator startup
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-05 14:01:00 PDT
Created attachment 551153 [details] [diff] [review]
Simplification of StopIteration startup
Comment 3 Luke Wagner [:luke] 2011-08-08 10:31:00 PDT
Comment on attachment 551149 [details] [diff] [review]
Initial rearrangement of js_InitIterator, clarification of Iterator startup

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

::: js/src/jsiter.cpp
@@ +1453,5 @@
> +    GlobalObject *global = obj->asGlobal();
> +
> +    /*
> +     * Bail if Iterator has already been initialized.  We test for Iterator
> +     * rather than for StopIteration because if js_InitIteratorClasses recurs,

s/js_//
Comment 4 Luke Wagner [:luke] 2011-08-08 10:53:47 PDT
Comment on attachment 551153 [details] [diff] [review]
Simplification of StopIteration startup

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

::: js/src/jsiter.cpp
@@ +1445,4 @@
>  static JSObject *
>  InitStopIterationClass(JSContext *cx, GlobalObject *global)
>  {
> +    JSObject *proto = global->createBlankPrototype(cx, &js_StopIterationClass);

Here, I have the opposite comment as below: createBlankPrototype seems like it would be a free function, akin to NewNonFunction.

@@ +1452,5 @@
> +    /* This should use a non-JSProtoKey'd slot, but this is easier for now. */
> +    if (!DefineConstructorAndPrototype(cx, global, JSProto_StopIteration, proto, proto))
> +        return NULL;
> +
> +    MarkStandardClassInitializedNoProto(global, &js_StopIterationClass);

From IRL: some time in the future it would be nice if DefineConstructAndPrototype and MarkStandardClassInitializedNoProto were members of the GlobalObject class.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-08 16:35:40 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a399a694bfad
http://hg.mozilla.org/integration/mozilla-inbound/rev/aade388e6c62
http://hg.mozilla.org/integration/mozilla-inbound/rev/f25d125362a8

Re s/js_//: note that that function name remains as it was, so I think that's correct for now.

Re createBlankPrototype: we'll have to ask people.  No one else who's reviewed patches such as these has said anything, and it's not clear to me why homing the method in GlobalObject is not right.

Re IRL: yeah.  There is more work to do even after the patches I have in my tree have landed, and that can be part of it.

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