Abstract out Modulator from V8PerContextData

third_party/WebKit/Source/core/dom/Modulator.cpp

Index: third_party/WebKit/Source/core/dom/Modulator.cpp
diff --git a/third_party/WebKit/Source/core/dom/Modulator.cpp b/third_party/WebKit/Source/core/dom/Modulator.cpp
index c5a8aed9e1f46cc004a82536dfcb740228c8ef02..be9a44da5034e08abfa8d8e4d84565483b7a88d7 100644
--- a/third_party/WebKit/Source/core/dom/Modulator.cpp
+++ b/third_party/WebKit/Source/core/dom/Modulator.cpp
@@ -8,16 +8,41 @@
 
 namespace blink {
 
-Modulator* Modulator::from(LocalFrame* frame) {
+namespace {
+const char* kPerContextDataKey = "Modulator";

[dcheng, 2017/04/05 21:13:29]

Drive-by:
const char kPerContextDataKey[] = "Modulator";

or

const char* const kPerContextDataKey = "Modulator";

In general, the former is preferred. The reason for either of these over:
  const char* kPerContextDataKey = "Modulator";

is that the "constant" is still assignable here.

[adithyas, 2017/04/05 21:45:36]

Fixed, nice catch :)

+
+V8PerContextData* getPerContextData(LocalFrame* frame) {

[haraken, 2017/04/05 02:08:10]

I'm not sure if it makes sense to let Modulator take a LocalFrame* parameter
(and assume that we're in the main world). From the binding perspective,
Modulator should be 1:1 with v8::Context, so Modulator should take a
ScriptState* parameter.

I'd prefer changing the call site of Modulator to pass in
ScriptState::forMainWorld(frame). Modulator should not worry about LocalFrame.

[kouhei (in TOK), 2017/04/05 02:09:05]

Ah. This completely makes sense to me.

[adithyas, 2017/04/05 21:45:36]

Yeah, ScriptState sounds appropriate, and it removes the need for 2 "from"
methods. I'll change this in a follow up.

   ScriptState* scriptState = ScriptState::forMainWorld(frame);
   if (!scriptState)
     return nullptr;
-  // TODO(kouhei): setModulator in V8PerContextData when we land ModulatorImpl.
-  return scriptState->perContextData()->modulator();
+  return scriptState->perContextData();
+}
+}  // namespace
+
+Modulator* Modulator::from(LocalFrame* frame) {
+  return from(getPerContextData(frame));
+}
+
+Modulator* Modulator::from(V8PerContextData* perContextData) {

[haraken, 2017/04/05 02:08:10]

Ditto. It would be nicer to pass in a ScriptState* parameter.

You can address these issues in a follow-up CL.

[jbroman, 2017/04/05 21:07:00]

+1; ScriptState seems nicer here (and it's cheap to get V8PCD from a ScriptState
anyhow).

+  if (!perContextData)
+    return nullptr;
+  return static_cast<Modulator*>(perContextData->getData(kPerContextDataKey));
 }
 
 Modulator::~Modulator() {}
 
+void Modulator::setModulator(LocalFrame* frame, Modulator* modulator) {
+  V8PerContextData* perContextData = getPerContextData(frame);
+  DCHECK(perContextData);
+  perContextData->addData(kPerContextDataKey, modulator);
+}
+
+void Modulator::clearModulator(LocalFrame* frame) {
+  V8PerContextData* perContextData = getPerContextData(frame);
+  DCHECK(perContextData);
+  perContextData->clearData(kPerContextDataKey);
+}
+
 KURL Modulator::resolveModuleSpecifier(const String& moduleRequest,
                                        const KURL& baseURL) {
   // Step 1. Apply the URL parser to specifier. If the result is not failure,