De-Element ScriptLoader

third_party/WebKit/Source/core/dom/ScriptLoaderClient.h

Index: third_party/WebKit/Source/core/dom/ScriptLoaderClient.h
diff --git a/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h b/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h
index 3e9fd7ed9ac79467943a03de6331b91203eb7faa..9f71e0b2555ebd1e8eb223b78f4c864f52090fa4 100644
--- a/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h
+++ b/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h
@@ -22,30 +22,65 @@
 #define ScriptLoaderClient_h
 
 #include "core/CoreExport.h"
+#include "platform/heap/Handle.h"
+#include "platform/heap/Heap.h"
 #include "wtf/text/AtomicString.h"
+#include "wtf/text/TextPosition.h"
 #include "wtf/text/WTFString.h"
 
 namespace blink {
+class Document;
+class Element;
+class HTMLScriptElementOrSVGScriptElement;
+class ScriptLoader;
 
-class CORE_EXPORT ScriptLoaderClient {
+class CORE_EXPORT ScriptLoaderClient : public GarbageCollectedMixin {

[Nate Chapin, 2017/02/28 21:39:29]

I'm not thrilled with how large this interface ended up. :/

  public:
   virtual ~ScriptLoaderClient() {}
 
-  virtual void dispatchLoadEvent() = 0;
+  static ScriptLoaderClient* fromElementIfPossible(Element*);
 
-  virtual String sourceAttributeValue() const = 0;
-  virtual String charsetAttributeValue() const = 0;
-  virtual String typeAttributeValue() const = 0;
-  virtual String languageAttributeValue() const = 0;
-  virtual String forAttributeValue() const = 0;
-  virtual String eventAttributeValue() const = 0;
-  virtual bool asyncAttributeValue() const = 0;
-  virtual bool deferAttributeValue() const = 0;
-  virtual bool hasSourceAttribute() const = 0;
+  virtual void dispatchLoadEvent() {}
+  virtual void dispatchErrorEvent() {}
+
+  virtual String sourceAttributeValue() const { return String(); }

[Nate Chapin, 2017/02/28 21:39:29]

I don't know how good/bad it is to have the default implementations here. At the
bare minimum, I don't think document() can do anything sane in the base class.

[hiroshige, 2017/03/01 00:58:14]

I think making these all methods all abstract is better, because, due to the
large number of virtual functions, it is relatively easy to forget to override
all of them (and I think we don't expect the default implementation works well
in the production, except for in tests).

[Nate Chapin, 2017/03/01 21:34:59]

I had been thinking that there are some methods where the default implementation
is benign:
* async/defer, which simply don't exist for SVGScriptElement
* If we do have a non-Element ScriptLoaderClient someday, anything that looks at
attributes can probably safely use the default?

But yeah, I don't know how to balance that with the risk of forgetting to
override something important.

[hiroshige, 2017/03/06 22:54:03]

I think it's better to make these abstract, because
This is not trivial and the reason for using (not overriding) the default
implementation in SVGScriptElement is not so clear from the default definition
here.
(Or to be explicitly state this, we might have to add a comment like
"SVGScriptElement doesn't have async/defer" here, which looks layering
violation.)

Implementing these as returning String() in SVGScriptElement (possibly with
comments saying that SVGScriptElement doesn't have these) is clearer.
Even in that case it's safer to enforce the new subclass to implement the
overrides (mostly returning String()/false), because we can explicitly check
which attributes/methods are meaningful for the new subclass or not.
(Probably adding a new ScriptLoaderClient subclass will require substantial work
(not just adding a new subclass but also modifications to other parts of code
and spec), so making the things rigid and explicit will be more helpful than
making the things easier.)

[kouhei (in TOK), 2017/03/15 03:31:40]

+1 on making this abstract

[Nate Chapin, 2017/03/16 20:15:09]

Done.

+  virtual String charsetAttributeValue() const { return String(); }
+  virtual String typeAttributeValue() const { return String(); }

[hiroshige, 2017/03/01 00:58:14]

Can we split these methods into subgroups for readability, at least separate
attributeValue() getters from the others? Sorting attribute getters might also
be good.

[Nate Chapin, 2017/03/01 21:34:59]

Done.

+  virtual String languageAttributeValue() const { return String(); }
+  virtual String forAttributeValue() const { return String(); }
+  virtual String eventAttributeValue() const { return String(); }
+  virtual String crossOriginAttributeValue() const { return String(); }
+  virtual String integrityAttributeValue() const { return String(); }
+  virtual String textFromChildren() { return String(); }
+  virtual String textContent() const { return String(); }
+  virtual bool asyncAttributeValue() const { return false; }
+  virtual bool deferAttributeValue() const { return false; }
+  virtual bool hasSourceAttribute() const { return false; }
+  virtual bool isConnected() const { return false; }
+  virtual bool hasChildren() const { return false; }
+  virtual bool isNonceableElement() const { return false; }
+  virtual bool allowInlineScriptForCSP(const AtomicString& nonce,

[hiroshige, 2017/03/01 00:58:14]

nit: Can this be a const method?

[Nate Chapin, 2017/03/01 21:34:59]

It calls a CSP function that takes a non-const Element*, so not without doing a
fairly large change to ContentSecurityPolicy to make all its Element* usage
const.

[hiroshige, 2017/03/06 22:54:03]

Oh, I see. (and I've just found the ContentSecurityPolicy function eventually
causes event dispatching on the element? and thus probably this cannot be const)

+                                       const WTF::OrdinalNumber&,
+                                       const String& scriptContent) = 0;
+  virtual AtomicString initiatorName() const { return AtomicString(); }
+  virtual Document& document() const = 0;
+  virtual void setScriptElementForBinding(
+      HTMLScriptElementOrSVGScriptElement&) {}
+
+  ScriptLoader* loader() const { return m_loader.get(); }
 
   AtomicString nonce() const { return m_nonce; }
   void setNonce(const String& nonce) { m_nonce = AtomicString(nonce); }
 
+  DECLARE_VIRTUAL_TRACE();
+
+ protected:
+  void initializeScriptLoader(bool parserInserted,
+                              bool alreadyStarted,
+                              bool createdDuringDocumentWrite);
+
+  Member<ScriptLoader> m_loader;
+
  private:
   AtomicString m_nonce;
 };