ScriptPromise should check the constructor paraemter.

ScriptPromise should check the constructor paraemter.

ScriptPromise::ScriptPromise(v8::Handle<v8::Value>, v8::Isolate*) expects
that the first parameter is a Promise object, but no one checks that.
This CL fixes that.

Rename ScriptPromise::ScriptPromise(ScriptValue) to ScriptPromise::cast to
avoid the confusion caused by the diferrent behaviors with a non-Promise parameter.

BUG=347047
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168462

[yhirano, 2014-02-26 06:54:15]

PTAL?

abarth: bindings/v8
kinuko: modules/serviceworkers

[kinuko, 2014-02-26 07:53:21]

module/serviceworker lgtm

https://codereview.chromium.org/181173002/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/181173002/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromise.cpp:60: ScriptPromise ScriptPromise::cast(const
ScriptValue& value)
nit: can we match the order in .h and in .cpp? (though this diff is easier to
read)

[kinuko, 2014-02-26 08:33:02]

Btw I believe the BUG= line is wrong.

[yhirano, 2014-02-26 09:29:49]

On 2014/02/26 08:33:02, kinuko wrote:
> Btw I believe the BUG= line is wrong.

Thanks, fixed.

[yhirano, 2014-02-26 09:29:57]

https://codereview.chromium.org/181173002/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/181173002/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromise.cpp:60: ScriptPromise ScriptPromise::cast(const
ScriptValue& value)
On 2014/02/26 07:53:21, kinuko wrote:
> nit: can we match the order in .h and in .cpp? (though this diff is easier to
> read)

Done.

[yhirano, 2014-03-03 09:03:35]

abarth, PTAL?

[abarth-chromium, 2014-03-03 18:59:56]

LGTM

[abarth-chromium, 2014-03-03 18:59:59]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-03 19:00:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/181173002/40001

[commit-bot: I haz the power, 2014-03-03 19:00:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-03 19:00:40]

Failed to apply patch for Source/bindings/v8/ScriptPromiseTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/v8/ScriptPromiseTest.cpp
  Hunk #2 FAILED at 53.
  Hunk #3 succeeded at 79 (offset -2 lines).
  Hunk #4 succeeded at 100 (offset -2 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
Source/bindings/v8/ScriptPromiseTest.cpp.rej

Patch:       Source/bindings/v8/ScriptPromiseTest.cpp
Index: Source/bindings/v8/ScriptPromiseTest.cpp
diff --git a/Source/bindings/v8/ScriptPromiseTest.cpp
b/Source/bindings/v8/ScriptPromiseTest.cpp
index
07153f1a96d0b9be2679f14438845efc2cf1dc79..8020ed6f443d9485ac731bd46f6a8ea9dc8af749
100644
--- a/Source/bindings/v8/ScriptPromiseTest.cpp
+++ b/Source/bindings/v8/ScriptPromiseTest.cpp
@@ -43,6 +43,8 @@ namespace WebCore {
 
 namespace {
 
+void callback(const v8::FunctionCallbackInfo<v8::Value>& info) { }
+
 class ScriptPromiseTest : public testing::Test {
 public:
     ScriptPromiseTest()
@@ -51,19 +53,16 @@ public:
         , m_context(m_isolate, v8::Context::New(m_isolate))
         , m_contextScope(m_context.newLocal(m_isolate))
     {
-    }
-
-    void SetUp()
-    {
         v8::Handle<v8::Context> context(m_context.newLocal(m_isolate));
         V8PerContextDataHolder::install(context,
DOMWrapperWorld::current(m_isolate));
         m_perContextData = V8PerContextData::create(context);
         m_perContextData->init();
     }
 
-    void TearDown()
+    ~ScriptPromiseTest()
     {
-        m_perContextData.clear();
+        // FIXME: We put this statement here to clear an exception from the
isolate.
+        createClosure(callback, v8::Undefined(m_isolate), m_isolate);
     }
 
     V8PromiseCustom::PromiseState state(ScriptPromise promise)
@@ -79,10 +78,18 @@ protected:
     OwnPtr<V8PerContextData> m_perContextData;
 };
 
+TEST_F(ScriptPromiseTest, constructFromNonPromise)
+{
+    v8::TryCatch trycatch;
+    ScriptPromise promise(v8::Undefined(m_isolate), m_isolate);
+    ASSERT_TRUE(trycatch.HasCaught());
+    ASSERT_TRUE(promise.hasNoValue());
+}
+
 TEST_F(ScriptPromiseTest, castPromise)
 {
     ScriptPromise promise = ScriptPromise::createPending();
-    ScriptPromise newPromise(ScriptValue(promise.v8Value(), m_isolate));
+    ScriptPromise newPromise =
ScriptPromise::cast(ScriptValue(promise.v8Value(), m_isolate));
 
     ASSERT_FALSE(promise.hasNoValue());
     EXPECT_EQ(V8PromiseCustom::Pending, state(promise));
@@ -92,8 +99,8 @@ TEST_F(ScriptPromiseTest, castPromise)
 TEST_F(ScriptPromiseTest, castNonPromise)
 {
     ScriptValue value = ScriptValue(v8String(m_isolate, "hello"), m_isolate);
-    ScriptPromise promise1(ScriptValue(value.v8Value(), m_isolate));
-    ScriptPromise promise2(ScriptValue(value.v8Value(), m_isolate));
+    ScriptPromise promise1 = ScriptPromise::cast(ScriptValue(value.v8Value(),
m_isolate));
+    ScriptPromise promise2 = ScriptPromise::cast(ScriptValue(value.v8Value(),
m_isolate));
 
     ASSERT_FALSE(promise1.hasNoValue());
     ASSERT_FALSE(promise2.hasNoValue());

[kinuko, 2014-03-04 01:59:38]

Hirano-san, we have similar code to WaitUntilObserver.cpp in
Source/modules/serviceworkers/RespondWithObserver.cpp (which is just landed),
you probably need to update that too (in addition to resolving conflicts)--
thanks.

[yhirano, 2014-03-04 14:40:58]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-04 14:41:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/181173002/60001

[commit-bot: I haz the power, 2014-03-04 14:41:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 14:41:13]

Failed to apply patch for Source/bindings/v8/ScriptPromiseTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/v8/ScriptPromiseTest.cpp
  Hunk #2 FAILED at 53.
  Hunk #3 succeeded at 74 with fuzz 1 (offset -5 lines).
  Hunk #4 succeeded at 95 (offset -5 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
Source/bindings/v8/ScriptPromiseTest.cpp.rej

Patch:       Source/bindings/v8/ScriptPromiseTest.cpp
Index: Source/bindings/v8/ScriptPromiseTest.cpp
diff --git a/Source/bindings/v8/ScriptPromiseTest.cpp
b/Source/bindings/v8/ScriptPromiseTest.cpp
index
2eb97f3695a4bdd0e3f7b9b140d7017c99dce011..51e0cf025feda68237b0d206e77a474f5705c3a2
100644
--- a/Source/bindings/v8/ScriptPromiseTest.cpp
+++ b/Source/bindings/v8/ScriptPromiseTest.cpp
@@ -43,6 +43,8 @@ namespace WebCore {
 
 namespace {
 
+void callback(const v8::FunctionCallbackInfo<v8::Value>& info) { }
+
 class ScriptPromiseTest : public testing::Test {
 public:
     ScriptPromiseTest()
@@ -51,17 +53,14 @@ public:
         , m_context(m_isolate, v8::Context::New(m_isolate))
         , m_contextScope(m_context.newLocal(m_isolate))
     {
-    }
-
-    void SetUp()
-    {
         // FIXME: Create a new world and pass it to V8PerContextData.
         m_perContextData =
V8PerContextData::create(m_context.newLocal(m_isolate), 0);
     }
 
-    void TearDown()
+    ~ScriptPromiseTest()
     {
-        m_perContextData.clear();
+        // FIXME: We put this statement here to clear an exception from the
isolate.
+        createClosure(callback, v8::Undefined(m_isolate), m_isolate);
     }
 
     V8PromiseCustom::PromiseState state(ScriptPromise promise)
@@ -77,10 +76,18 @@ protected:
     OwnPtr<V8PerContextData> m_perContextData;
 };
 
+TEST_F(ScriptPromiseTest, constructFromNonPromise)
+{
+    v8::TryCatch trycatch;
+    ScriptPromise promise(v8::Undefined(m_isolate), m_isolate);
+    ASSERT_TRUE(trycatch.HasCaught());
+    ASSERT_TRUE(promise.hasNoValue());
+}
+
 TEST_F(ScriptPromiseTest, castPromise)
 {
     ScriptPromise promise = ScriptPromise::createPending();
-    ScriptPromise newPromise(ScriptValue(promise.v8Value(), m_isolate));
+    ScriptPromise newPromise =
ScriptPromise::cast(ScriptValue(promise.v8Value(), m_isolate));
 
     ASSERT_FALSE(promise.hasNoValue());
     EXPECT_EQ(V8PromiseCustom::Pending, state(promise));
@@ -90,8 +97,8 @@ TEST_F(ScriptPromiseTest, castPromise)
 TEST_F(ScriptPromiseTest, castNonPromise)
 {
     ScriptValue value = ScriptValue(v8String(m_isolate, "hello"), m_isolate);
-    ScriptPromise promise1(ScriptValue(value.v8Value(), m_isolate));
-    ScriptPromise promise2(ScriptValue(value.v8Value(), m_isolate));
+    ScriptPromise promise1 = ScriptPromise::cast(ScriptValue(value.v8Value(),
m_isolate));
+    ScriptPromise promise2 = ScriptPromise::cast(ScriptValue(value.v8Value(),
m_isolate));
 
     ASSERT_FALSE(promise1.hasNoValue());
     ASSERT_FALSE(promise2.hasNoValue());

[yhirano, 2014-03-04 14:41:56]

On 2014/03/04 01:59:38, kinuko wrote:
> Hirano-san, we have similar code to WaitUntilObserver.cpp in
> Source/modules/serviceworkers/RespondWithObserver.cpp (which is just landed),
> you probably need to update that too (in addition to resolving conflicts)--
> thanks.

Thanks, done.

[yhirano, 2014-03-04 15:46:41]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-04 15:46:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/181173002/80001

[yhirano, 2014-03-05 08:47:17]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2014-03-05 08:47:22]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2014-03-05 09:03:38]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2014-03-05 10:27:10]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-05 10:27:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/181173002/80001

[commit-bot: I haz the power, 2014-03-05 10:28:15]

Change committed as 168462