Abstract out Modulator from V8PerContextData

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/dom/Modulator.h"
 
 #include "bindings/core/v8/ScriptState.h"
 
 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,
   // return the result.
   KURL url(KURL(), moduleRequest);
   if (url.isValid())
     return url;
 
   // Step 2. If specifier does not start with the character U+002F SOLIDUS (/),
   // the two-character sequence U+002E FULL STOP, U+002F SOLIDUS (./), or the
   // three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F SOLIDUS
   // (../), return failure and abort these steps.
   if (!moduleRequest.startsWith("/") && !moduleRequest.startsWith("./") &&
       !moduleRequest.startsWith("../"))
     return KURL();
 
   // Step 3. Return the result of applying the URL parser to specifier with
   // script's base URL as the base URL.
   DCHECK(baseURL.isValid());
   KURL absoluteURL(baseURL, moduleRequest);
   if (absoluteURL.isValid())
     return absoluteURL;
 
   return KURL();
 }
 
 }  // namespace blink