bindings: Use V8 MaybeLocal<> APIs in Dictionary class

Source/bindings/core/v8/Dictionary.h

Index: Source/bindings/core/v8/Dictionary.h
diff --git a/Source/bindings/core/v8/Dictionary.h b/Source/bindings/core/v8/Dictionary.h
index 5860f34414e00be8276216d35edc9ca8d684d6e0..69c577bb779166ca87caf9a15993cf99806f0849 100644
--- a/Source/bindings/core/v8/Dictionary.h
+++ b/Source/bindings/core/v8/Dictionary.h
@@ -29,6 +29,7 @@
 #include "bindings/core/v8/ExceptionMessages.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/Nullable.h"
+#include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/ScriptValue.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8BindingMacros.h"
@@ -44,12 +45,12 @@ namespace blink {
 // Dictionary class provides ways to retrieve property values as C++ objects
 // from a V8 object. Instances of this class must not outlive V8's handle scope
 // because they hold a V8 value without putting it on persistent handles.
-class Dictionary {
+class Dictionary final : public ScriptState::Observer {

[haraken, 2015/03/03 05:12:15]

ScriptState::Observer is just intended to be used for some inspector-specific
reason, and it's highly discouraged (because the lifetime of ScriptState is
already complicated and we don't want to introduce something more complicated
like observers).

I don't think you need the observer. See my comment below.

[bashi, 2015/03/03 05:56:28]

I see.

     ALLOW_ONLY_INLINE_ALLOCATION();
 public:
     Dictionary();
     Dictionary(const v8::Handle<v8::Value>& options, v8::Isolate*, ExceptionState&);
-    ~Dictionary();
+    virtual ~Dictionary();
 
     Dictionary& operator=(const Dictionary&);
 
@@ -120,11 +121,21 @@ public:
     bool hasProperty(const String&) const;
 
     v8::Isolate* isolate() const { return m_isolate; }
+    // Used by DictionaryHelper. This context could be an empty handle and
+    // should be used only for V8 APIs which return MaybeLocal<T>.
+    v8::Local<v8::Context> context() const { return m_context; }
 
     bool getKey(const String& key, v8::Local<v8::Value>&) const;
 
 private:
+    // ScriptState::Observer implementation
+    virtual void willDisposeScriptState(ScriptState*) override;
+
+    void subscribeScriptStateDisposal();
+    void unsubscribeScriptStateDisposal();
+
     v8::Handle<v8::Value> m_options;
+    v8::Local<v8::Context> m_context;

[bashi, 2015/03/03 04:25:05]

Should we hold ScriptState instead of v8::Local<v8::Context> ?

[haraken, 2015/03/03 05:12:15]

A big YES. If a DOM object needs to keep some script state (including Context),
the DOM object should hold RefPtr<ScriptState>. It is scary to hold a Context
and I've replaced almost all of them with RefPtr<ScriptState> before :)

However, in this particular case, Dictionary is guaranteed to be used on stack,
which means that we're guaranteed to be in a correct ScriptState already when
Dictionary is used. Thus what we should do here is:

- Guarantee that Dictionary is used only on stack. I think this is already
guaranteed in practice but we have no way to verify the fact.

- Guarantee that we're always in a correct ScriptState. I hope this is already
guaranteed. (As discussed offline, this is hard to verify.)

- Just use m_isolate->GetCurrentContext() in Dictionary methods.

[bashi, 2015/03/03 05:56:28]

Yeah, relying on RefPtr looks much better, and will follow your suggestion. But
I have one concern: Won't we want to stop using GetCurrentContext() in the near
future? If we want to stop using it, I guess that we would need this kind of
weak reference (or make Dictionary take ScriptState each time get() called).

One of the reasons why I choose Dictionary as the first instance was to make
sure that this kind of our assumption is reasonable for the time being:)

[haraken, 2015/03/03 06:12:14]

I'm sorry for not being clear.

Using GetCurrentContext() is completely fine (Note: We want to remove
Isolate::GetCurrent(), but that's a totally different issue than
Isolate::GetCurrentContext()). What we must make sure is that we are always in a
correct context by entering a correct ScriptState::Scope. Being in a correct
context is a mandatory requirement to make the system work correctly. And once
it's guaranteed (I hope it's already guaranteed but it's very hard to guarantee
the fact), GetCurrentContext() is guaranteed to return a correct context
whenever it's called, so using GetCurrentContext() is totally fine.

In short, what we should do is not to avoid GetCurrentContext but to guarantee
that we're always in a correct context (by entering a correct
ScriptState::Scope).

     v8::Isolate* m_isolate;
     ExceptionState* m_exceptionState;
 };