[builtins] move String.prototype[@@iterator] to C++ builtin

src/bootstrapper.cc

Index: src/bootstrapper.cc
diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
index 14fe8b0a5b67b9c058cf437de1d44c6931c3d555..816e30a417e841d83f75510bde113d2996feb791 100644
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@@ -1406,6 +1406,18 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
                           Builtins::kStringPrototypeTrimRight, 0, false);
     SimpleInstallFunction(prototype, "valueOf",
                           Builtins::kStringPrototypeValueOf, 0, true);
+
+    Handle<JSFunction> iterator = CreateFunction(
+        isolate, factory->NewStringFromAsciiChecked("[Symbol.iterator]"),
+        JS_OBJECT_TYPE, JSObject::kHeaderSize, MaybeHandle<JSObject>(),
+        Builtins::kStringCreateIterator, true);
+    iterator->shared()->set_internal_formal_parameter_count(0);
+    iterator->shared()->set_length(0);
+    iterator->shared()->set_language_mode(STRICT);

[Benedikt Meurer, 2016/09/15 17:34:33]

Why do you need to set the language mode on a C++ builtin?

[caitp, 2016/09/15 19:02:39]

I guess the other builtins just use set_native(true), so I've done that. A
better fix would be to add a version of these helpers which accept symbol
properties, but that seemed like more effort

[Benedikt Meurer, 2016/09/16 03:16:37]

Yes, we really need a version that can deal with names in general, but Yang
and/or Andreas expressed some concerns regarding the number of helpers...

+    iterator->map()->set_is_constructor(false);

[Benedikt Meurer, 2016/09/15 17:34:33]

This mutates the map inplace, which is not what you want. You have to use the
appropriate map instead.

+
+    JSObject::AddProperty(prototype, factory->iterator_symbol(), iterator,
+                          static_cast<PropertyAttributes>(DONT_ENUM));
   }
 
   {
@@ -3382,6 +3394,33 @@ bool Genesis::InstallNatives(GlobalContextType context_type) {
     }
   }
 
+  {  // StringIterator
+    PrototypeIterator iter(native_context()->generator_object_prototype_map());

[Benedikt Meurer, 2016/09/15 17:34:33]

Can we make this more robust? I.e. have the iterator prototype as field in the
native context or the bootstrapper?

[caitp, 2016/09/15 19:02:39]

Ok.

I assume it wasn't done originally to avoid growing the context more.

+    iter.Advance();  // Advance to the prototype of generator_object_prototype.
+    Handle<JSObject> iterator_prototype =
+        Handle<JSObject>(iter.GetCurrent<JSObject>());
+
+    Handle<JSObject> string_iterator_prototype =
+        factory()->NewJSObject(isolate()->object_function(), TENURED);
+    JSObject::ForceSetPrototype(string_iterator_prototype, iterator_prototype);

[Benedikt Meurer, 2016/09/15 17:34:33]

Not sure about this ForceSetPrototype. We seem to just use
JSObject::SetPrototype elsewehere in this file.

[caitp, 2016/09/15 19:02:39]

was copy/pasted from one of the Map or Set iterator setup code. I'm not sure
this is actually the right place for this block to live at all, it seems out of
place.

Will change it to SetPrototype and see if that breaks anything, anyway.
(actually, after uploading the patch, I may have forgotten to change it to
SetPrototype, but I'll wait for the tryjob to see if there are other failures
before changing it)

+
+    JSObject::AddProperty(
+        string_iterator_prototype, factory()->to_string_tag_symbol(),
+        factory()->NewStringFromAsciiChecked("String Iterator"),
+        static_cast<PropertyAttributes>(DONT_ENUM | READ_ONLY));
+
+    InstallFunction(string_iterator_prototype, "next", JS_OBJECT_TYPE,
+                    JSObject::kHeaderSize, MaybeHandle<JSObject>(),
+                    Builtins::kStringIteratorNext);
+
+    Handle<JSFunction> string_iterator_function = CreateFunction(
+        isolate(), factory()->NewStringFromAsciiChecked("StringIterator"),
+        JS_STRING_ITERATOR_TYPE, JSStringIterator::kSize,
+        string_iterator_prototype, Builtins::kIllegal);
+    native_context()->set_string_iterator_map(
+        string_iterator_function->initial_map());
+  }
+
   return true;
 }