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

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

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 169 matching lines @@
 
   V8StringResource(const String& string)
       : m_mode(Externalize), m_string(string) {}
 
   void operator=(v8::Local<v8::Value> object) { m_v8Object = object; }
 
   void operator=(const String& string) { setString(string); }
 
   void operator=(std::nullptr_t) { setString(String()); }
 
   bool prepare() {

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

This should be deprecated as well, right?

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

Done.

     if (prepareFast())
       return true;
 
     // 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>(); }
   operator AtomicString() const { return toString<AtomicString>(); }
 
  private:
   bool prepareFast() {
     if (m_v8Object.IsEmpty())
       return true;
 
     if (!isValid()) {
       setString(fallbackString());
       return true;
     }
 
     if (LIKELY(m_v8Object->IsString()))
       return true;
 
     if (LIKELY(m_v8Object->IsInt32())) {
       setString(int32ToWebCoreString(m_v8Object.As<v8::Int32>()->Value()));
       return true;
     }
 
     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;
 
   void setString(const String& string) {
     m_string = string;
     m_v8Object.Clear();  // To signal that String is ready.
   }
 
   template <class StringType>
@@ skipping 50 matching lines @@
 
 template <>
 inline String
 V8StringResource<TreatNullAndUndefinedAsNullString>::fallbackString() const {
   return String();
 }
 
 }  // namespace blink
 
 #endif  // V8StringResource_h