From 30c38af903e3fc55f4ada0b99bb2598a3aef63e7 Mon Sep 17 00:00:00 2001
From: Elliott Sprehn <esprehn@chromium.org>
Date: Mon, 26 Jan 2015 11:38:04 -0800
Subject: [PATCH] module.exports should default to an empty object.

Per the spec in modules.md the exports property should default to an
empty object. We lazy allocate it so that modules that just replace it
don't create the empty object and then throw it away.

R=abarth@chromium.org

Review URL: https://codereview.chromium.org/872043003
---
 engine/bindings/core/v8/ScriptController.cpp |  2 +-
 engine/bindings/core/v8/ScriptValue.h        |  5 +++++
 engine/bindings/templates/attributes.cpp     |  3 +++
 engine/core/app/AbstractModule.cpp           |  2 +-
 engine/core/app/Module.cpp                   | 10 ++++++++++
 engine/core/app/Module.h                     |  6 +++---
 engine/core/app/Module.idl                   |  2 +-
 examples/city-list/city-list.sky             |  5 -----
 tests/modules/import-without-export.sky      |  6 ++++--
 tests/modules/modules-expected.txt           |  7 ++++---
 tests/modules/modules.sky                    |  7 +++++++
 11 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/engine/bindings/core/v8/ScriptController.cpp b/engine/bindings/core/v8/ScriptController.cpp
index bd14d09324b..ffc6aeefd5c 100644
--- a/engine/bindings/core/v8/ScriptController.cpp
+++ b/engine/bindings/core/v8/ScriptController.cpp
@@ -295,7 +295,7 @@ void ScriptController::executeModuleScript(AbstractModule& module, const String&
                     scriptModule.formalDependencies.append(name);
                     v8::Handle<v8::Value> actual;
                     if (Module* childModule = child->module())
-                        actual = childModule->exports().v8Value();
+                        actual = childModule->exports(scriptState).v8Value();
                     if (actual.IsEmpty())
                         actual = v8::Undefined(m_isolate);
                     scriptModule.resolvedDependencies.append(actual);
diff --git a/engine/bindings/core/v8/ScriptValue.h b/engine/bindings/core/v8/ScriptValue.h
index 9102432ada3..41996d4d753 100644
--- a/engine/bindings/core/v8/ScriptValue.h
+++ b/engine/bindings/core/v8/ScriptValue.h
@@ -60,6 +60,11 @@ public:
         ASSERT(isEmpty() || m_scriptState);
     }
 
+    static ScriptValue createEmptyObject(ScriptState* scriptState)
+    {
+        return ScriptValue(scriptState, v8::Object::New(scriptState->isolate()));
+    }
+
     ScriptState* scriptState() const
     {
         return m_scriptState.get();
diff --git a/engine/bindings/templates/attributes.cpp b/engine/bindings/templates/attributes.cpp
index 1d21116419a..80e31b88f48 100644
--- a/engine/bindings/templates/attributes.cpp
+++ b/engine/bindings/templates/attributes.cpp
@@ -249,6 +249,9 @@ v8::Local<v8::Value> v8Value, const v8::PropertyCallbackInfo<void>& info
           attribute.is_setter_call_with_execution_context %}
     ExecutionContext* executionContext = currentExecutionContext(info.GetIsolate());
     {% endif %}
+    {% if attribute.is_call_with_script_state %}
+    ScriptState* scriptState = ScriptState::current(info.GetIsolate());
+    {% endif %}
     {# Set #}
     {{attribute.cpp_setter}};
     {# Post-set #}
diff --git a/engine/core/app/AbstractModule.cpp b/engine/core/app/AbstractModule.cpp
index a505f15e3cf..fccb59bf221 100644
--- a/engine/core/app/AbstractModule.cpp
+++ b/engine/core/app/AbstractModule.cpp
@@ -41,7 +41,7 @@ void AbstractModule::OnModuleLoadComplete(ModuleLoader* loader,
                                           Module* module) {
   RefPtr<ScriptPromiseResolver> resolver = loaders_.take(loader);
   ScriptState::Scope scope(resolver->scriptState());
-  resolver->resolve(module->exports().v8Value());
+  resolver->resolve(module->exports(resolver->scriptState()).v8Value());
 }
 
 } // namespace blink
diff --git a/engine/core/app/Module.cpp b/engine/core/app/Module.cpp
index ed1f97223cc..02dc428899d 100644
--- a/engine/core/app/Module.cpp
+++ b/engine/core/app/Module.cpp
@@ -29,4 +29,14 @@ Application* Module::GetApplication() {
   return application();
 }
 
+void Module::setExports(ScriptState*, const ScriptValue& exports) {
+  exports_ = exports;
+}
+
+const ScriptValue& Module::exports(ScriptState* scriptState) const {
+  if (exports_.isEmpty())
+    exports_ = ScriptValue::createEmptyObject(scriptState);
+  return exports_;
+}
+
 } // namespace blink
diff --git a/engine/core/app/Module.h b/engine/core/app/Module.h
index 29ff7c5ef8b..525b62734f9 100644
--- a/engine/core/app/Module.h
+++ b/engine/core/app/Module.h
@@ -24,8 +24,8 @@ public:
 
   Application* application() const { return application_.get(); }
 
-  void setExports(const ScriptValue& exports) { exports_ = exports; }
-  const ScriptValue& exports() const { return exports_; }
+  void setExports(ScriptState*, const ScriptValue& exports);
+  const ScriptValue& exports(ScriptState*) const;
 
 private:
   Module(ExecutionContext* context,
@@ -37,7 +37,7 @@ private:
   Application* GetApplication() override;
 
   RefPtr<Application> application_;
-  ScriptValue exports_;
+  mutable ScriptValue exports_;
 };
 
 } // namespace blink
diff --git a/engine/core/app/Module.idl b/engine/core/app/Module.idl
index 4d67150f288..276b42a4320 100644
--- a/engine/core/app/Module.idl
+++ b/engine/core/app/Module.idl
@@ -7,5 +7,5 @@
     Constructor(Application application, Document document, DOMString url),
 ] interface Module : AbstractModule {
     readonly attribute Application application;
-    attribute any exports;
+    [CallWith=ScriptState] attribute any exports;
 };
diff --git a/examples/city-list/city-list.sky b/examples/city-list/city-list.sky
index 5d57d0b57a9..a29a3e81173 100644
--- a/examples/city-list/city-list.sky
+++ b/examples/city-list/city-list.sky
@@ -7,11 +7,6 @@
 <import src="city-data-service.sky" as="CityDataService" />
 <import src="city-sequence.sky" as="CitySequence" />
 
-<script>
-// TODO(esprehn): exports should start as the empty object.
-module.exports = {};
-</script>
-
 <sky-element name="state-header">
 <template>
   <style>
diff --git a/tests/modules/import-without-export.sky b/tests/modules/import-without-export.sky
index 19415ffae34..a3eaa1ffe4c 100644
--- a/tests/modules/import-without-export.sky
+++ b/tests/modules/import-without-export.sky
@@ -3,7 +3,9 @@
 <import src="../resources/dump-as-text.sky" />
 <div id="result">FAIL - Script did not execute</div>
 <script>
-document.getElementById("result").textContent =
-    hello === undefined ? "PASS" : "FAIL";
+var result = "FAIL";
+if (typeof hello === "object" && Object.keys(hello).length === 0)
+  result = "PASS";
+document.getElementById("result").textContent = result;
 </script>
 </html>
diff --git a/tests/modules/modules-expected.txt b/tests/modules/modules-expected.txt
index c7bf9331b91..5bfda93db7e 100644
--- a/tests/modules/modules-expected.txt
+++ b/tests/modules/modules-expected.txt
@@ -1,5 +1,6 @@
-Running 1 tests
+Running 2 tests
 ok 1 Module should be constructable
-1 tests
-1 pass
+ok 2 Module should have an empty object by default for exports
+2 tests
+2 pass
 0 fail
diff --git a/tests/modules/modules.sky b/tests/modules/modules.sky
index 1e7bf94a46a..baccb20e383 100644
--- a/tests/modules/modules.sky
+++ b/tests/modules/modules.sky
@@ -13,6 +13,13 @@ describe('Module', function() {
     assert.equal(module.document, doc2);
     assert.equal(module.url, "http://www.example.com/module");
   });
+
+  it('should have an empty object by default for exports', function() {
+    var app = new Application(new Document(), "http://www.example.com/app");
+    var module = new Module(app, new Document(), "http://www.example.com/module");
+    assert.isObject(module.exports);
+    assert.lengthOf(Object.keys(module.exports), 0);
+  });
 });
 </script>
 </html>