binding: Lets Dictionary::getPropertyNames, etc. rethrow an exception.

third_party/WebKit/Source/bindings/core/v8/V8StringResource.h

Index: third_party/WebKit/Source/bindings/core/v8/V8StringResource.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8StringResource.h b/third_party/WebKit/Source/bindings/core/v8/V8StringResource.h
index 9661f9c34ec7729322121ff589f585dd1fd49623..8888db3956d502145cf08b8ac972cbdeb988720d 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8StringResource.h
+++ b/third_party/WebKit/Source/bindings/core/v8/V8StringResource.h
@@ -193,26 +193,22 @@ class V8StringResource {
 
     // TODO(bashi): Pass an isolate to this function and remove
     // v8::Isolate::GetCurrent().
+    // TODO(bashi): Pass an exceptionState to this function and handle an

[bashi, 2016/11/24 07:25:53]

Why me? :)

[Yuki, 2016/11/24 10:19:32]

Done. X(

Removed TODO comments entirely because I made this version of prepare()
DEPRECATED.

+    // exception correctly.  If ToLocal() returned false, an exception is
+    // thrown.
     return m_v8Object->ToString(v8::Isolate::GetCurrent()->GetCurrentContext())

[haraken, 2016/11/24 07:22:30]

Not related to your CL but why don't you need to have a TryCatch block before
calling ToString?

[Yuki, 2016/11/24 10:19:32]

If you were talking about "the best coding practice", then we do need a TryCatch
block, or we should use ToLocalChecked().

The reason why this works so far (at least for most cases) is
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8A...
As soon as prepare() returned false, we return to V8, so letting an exception be
thrown is okay.

Probably, we have some cases that don't work well, but I'm not sure...

         .ToLocal(&m_v8Object);
   }
 
-  bool prepare(ExceptionState& exceptionState) {
+  bool prepare(v8::Isolate* isolate, ExceptionState& exceptionState) {
+    return prepareFast() || prepareSlow(isolate, exceptionState);
+  }
+
+  bool prepare(ExceptionState& exceptionState) {  // DEPRECATED
     if (prepareFast())

[peria, 2016/11/24 07:21:23]

Can't we make this like prepare(isoalte, state)?

return prepareFast() || prepareSlow(v8::Isolate::GetCurrent(), exceptionState);

[Yuki, 2016/11/24 10:19:33]

Done.

       return true;
 
-    // TODO(bashi): Pass an isolate to this function and remove
-    // v8::Isolate::GetCurrent().
-    v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    v8::TryCatch block(isolate);
-    // Handle the case where an exception is thrown as part of invoking toString
-    // on the object.
-    if (!m_v8Object->ToString(isolate->GetCurrentContext())
-             .ToLocal(&m_v8Object)) {
-      exceptionState.rethrowV8Exception(block.Exception());
-      return false;
-    }
-    return true;
+    return prepareSlow(v8::Isolate::GetCurrent(), exceptionState);
   }
 
   operator String() const { return toString<String>(); }
@@ -239,6 +235,15 @@ class V8StringResource {
     m_mode = DoNotExternalize;
     return false;
   }
+  bool prepareSlow(v8::Isolate* isolate, ExceptionState& exceptionState) {

[peria, 2016/11/24 07:21:23]

nit: put an empty line here

[Yuki, 2016/11/24 10:19:32]

Done.

+    v8::TryCatch tryCatch(isolate);
+    if (!m_v8Object->ToString(isolate->GetCurrentContext())
+             .ToLocal(&m_v8Object)) {
+      exceptionState.rethrowV8Exception(tryCatch.Exception());
+      return false;
+    }
+    return true;
+  }
 
   bool isValid() const;
   String fallbackString() const;