Move setting of custom element default style outside of constructor

Previously custom element default style is passed to the constructor,
adding a CSSStyleSheet parameter to multiple parts of code. This change
removes that and instead sets the default style after the definition
has been built, in CustomElementRegistry::DefineInternal

Bug: 824684
Change-Id: Ic544561830cced77e946ae43491e6ee989511117
Reviewed-on: https://chromium-review.googlesource.com/1256263Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595821}

third_party/blink/renderer/bindings/core/v8/script_custom_element_definition.cc

@@ -79,12 +79,11 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::Create(
     V8Function* disconnected_callback,
     V8Function* adopted_callback,
     V8Function* attribute_changed_callback,
-    HashSet<AtomicString>&& observed_attributes,
-    CSSStyleSheet* default_style_sheet) {
+    HashSet<AtomicString>&& observed_attributes) {
   ScriptCustomElementDefinition* definition = new ScriptCustomElementDefinition(
       script_state, descriptor, constructor, connected_callback,
       disconnected_callback, adopted_callback, attribute_changed_callback,
-      std::move(observed_attributes), default_style_sheet);
+      std::move(observed_attributes));
 
   // Tag the JavaScript constructor object with its ID.
   v8::Local<v8::Value> id_value =
@@ -106,11 +105,8 @@ ScriptCustomElementDefinition::ScriptCustomElementDefinition(
     V8Function* disconnected_callback,
     V8Function* adopted_callback,
     V8Function* attribute_changed_callback,
-    HashSet<AtomicString>&& observed_attributes,
-    CSSStyleSheet* default_style_sheet)
-    : CustomElementDefinition(descriptor,
-                              default_style_sheet,
-                              std::move(observed_attributes)),
+    HashSet<AtomicString>&& observed_attributes)
+    : CustomElementDefinition(descriptor, std::move(observed_attributes)),
       script_state_(script_state),
       constructor_(constructor),
       connected_callback_(connected_callback),

third_party/blink/renderer/bindings/core/v8/script_custom_element_definition.h

@@ -40,8 +40,7 @@ class CORE_EXPORT ScriptCustomElementDefinition final
       V8Function* disconnected_callback,
       V8Function* adopted_callback,
       V8Function* attribute_changed_callback,
-      HashSet<AtomicString>&& observed_attributes,
-      CSSStyleSheet*);
+      HashSet<AtomicString>&& observed_attributes);
 
   ~ScriptCustomElementDefinition() override = default;
 
@@ -74,8 +73,7 @@ class CORE_EXPORT ScriptCustomElementDefinition final
                                 V8Function* disconnected_callback,
                                 V8Function* adopted_callback,
                                 V8Function* attribute_changed_callback,
-                                HashSet<AtomicString>&& observed_attributes,
-                                CSSStyleSheet*);
+                                HashSet<AtomicString>&& observed_attributes);
 
   // Implementations of |CustomElementDefinition|
   ScriptValue GetConstructorForScript() final;

third_party/blink/renderer/bindings/core/v8/script_custom_element_definition_builder.cc

@@ -11,7 +11,6 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_custom_element_constructor.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_function.h"
-#include "third_party/blink/renderer/core/css/css_style_sheet.h"
 #include "third_party/blink/renderer/platform/bindings/callback_method_retriever.h"
 #include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
@@ -23,13 +22,11 @@ namespace blink {
 ScriptCustomElementDefinitionBuilder::ScriptCustomElementDefinitionBuilder(
     ScriptState* script_state,
     CustomElementRegistry* registry,
-    CSSStyleSheet* default_style_sheet,
     V8CustomElementConstructor* constructor,
     ExceptionState& exception_state)
     : script_state_(script_state),
       exception_state_(exception_state),
       registry_(registry),
-      default_style_sheet_(default_style_sheet),
       constructor_(constructor) {}
 
 bool ScriptCustomElementDefinitionBuilder::CheckConstructorIntrinsics() {
@@ -135,8 +132,7 @@ CustomElementDefinition* ScriptCustomElementDefinitionBuilder::Build(
   return ScriptCustomElementDefinition::Create(
       script_state_, registry_, descriptor, id, constructor_,
       connected_callback_, disconnected_callback_, adopted_callback_,
-      attribute_changed_callback_, std::move(observed_attributes_),
-      default_style_sheet_);
+      attribute_changed_callback_, std::move(observed_attributes_));
 }
 
 }  // namespace blink

third_party/blink/renderer/bindings/core/v8/script_custom_element_definition_builder.h

@@ -19,7 +19,6 @@
 
 namespace blink {
 
-class CSSStyleSheet;
 class CustomElementRegistry;
 class ExceptionState;
 class ScriptState;
@@ -34,7 +33,6 @@ class CORE_EXPORT ScriptCustomElementDefinitionBuilder
  public:
   ScriptCustomElementDefinitionBuilder(ScriptState*,
                                        CustomElementRegistry*,
-                                       CSSStyleSheet*,
                                        V8CustomElementConstructor* constructor,
                                        ExceptionState&);
   ~ScriptCustomElementDefinitionBuilder() = default;
@@ -49,7 +47,6 @@ class CORE_EXPORT ScriptCustomElementDefinitionBuilder
   Member<ScriptState> script_state_;
   ExceptionState& exception_state_;
   Member<CustomElementRegistry> registry_;
-  const Member<CSSStyleSheet> default_style_sheet_;
   const Member<V8CustomElementConstructor> constructor_;
   // These v8::Local handles on stack make the function objects alive until we
   // finish building the CustomElementDefinition and wrapper-tracing on it gets

third_party/blink/renderer/core/html/custom/custom_element_definition.cc

@@ -30,19 +30,11 @@ CustomElementDefinition::CustomElementDefinition(
 
 CustomElementDefinition::CustomElementDefinition(
     const CustomElementDescriptor& descriptor,
-    CSSStyleSheet* default_style_sheet)
-    : descriptor_(descriptor), default_style_sheet_(default_style_sheet) {}
-
-CustomElementDefinition::CustomElementDefinition(
-    const CustomElementDescriptor& descriptor,
-    CSSStyleSheet* default_style_sheet,
     const HashSet<AtomicString>& observed_attributes)
     : descriptor_(descriptor),
       observed_attributes_(observed_attributes),
       has_style_attribute_changed_callback_(
-          observed_attributes.Contains(HTMLNames::styleAttr.LocalName())),
-      default_style_sheet_(default_style_sheet) {}
-
+          observed_attributes.Contains(HTMLNames::styleAttr.LocalName())) {}
 CustomElementDefinition::~CustomElementDefinition() = default;
 
 void CustomElementDefinition::Trace(blink::Visitor* visitor) {

third_party/blink/renderer/core/html/custom/custom_element_definition.h

@@ -91,6 +91,10 @@ class CORE_EXPORT CustomElementDefinition
                                        const AtomicString& old_value,
                                        const AtomicString& new_value);
 
+  void SetDefaultStyleSheet(CSSStyleSheet& default_style_sheet) {
+    default_style_sheet_ = default_style_sheet;
+  }
+
   CSSStyleSheet* DefaultStyleSheet() const { return default_style_sheet_; }
 
   class CORE_EXPORT ConstructionStackScope final {
@@ -110,10 +114,7 @@ class CORE_EXPORT CustomElementDefinition
  protected:
   CustomElementDefinition(const CustomElementDescriptor&);
 
-  CustomElementDefinition(const CustomElementDescriptor&, CSSStyleSheet*);
-
   CustomElementDefinition(const CustomElementDescriptor&,
-                          CSSStyleSheet*,
                           const HashSet<AtomicString>& observed_attributes);
 
   void AddDefaultStyle(Element*);
@@ -132,7 +133,7 @@ class CORE_EXPORT CustomElementDefinition
   bool has_style_attribute_changed_callback_;
   bool added_default_style_sheet_ = false;
 
-  const Member<CSSStyleSheet> default_style_sheet_;
+  Member<CSSStyleSheet> default_style_sheet_;
 
   void EnqueueAttributeChangedCallbackForAllAttributes(Element*);
 

third_party/blink/renderer/core/html/custom/custom_element_registry.cc

@@ -109,12 +109,8 @@ CustomElementDefinition* CustomElementRegistry::define(
     V8CustomElementConstructor* constructor,
     const ElementDefinitionOptions& options,
     ExceptionState& exception_state) {
-  CSSStyleSheet* style_sheet = nullptr;
-  if (RuntimeEnabledFeatures::CustomElementDefaultStyleEnabled() &&
-      options.hasStyle())
-    style_sheet = options.style();
-  ScriptCustomElementDefinitionBuilder builder(script_state, this, style_sheet,
-                                               constructor, exception_state);
+  ScriptCustomElementDefinitionBuilder builder(script_state, this, constructor,
+                                               exception_state);
   return DefineInternal(script_state, name, builder, options, exception_state);
 }
 
@@ -215,6 +211,9 @@ CustomElementDefinition* CustomElementRegistry::DefineInternal(
   CustomElementDefinition* definition = builder.Build(descriptor, id);
   CHECK(!exception_state.HadException());
   CHECK(definition->Descriptor() == descriptor);
+  if (RuntimeEnabledFeatures::CustomElementDefaultStyleEnabled() &&
+      options.hasStyle())
+    definition->SetDefaultStyleSheet(*options.style());
   definitions_.emplace_back(definition);
   NameIdMap::AddResult result = name_id_map_.insert(descriptor.GetName(), id);
   CHECK(result.is_new_entry);
@@ -230,6 +229,7 @@ CustomElementDefinition* CustomElementRegistry::DefineInternal(
     entry->value->Resolve();
     when_defined_promise_map_.erase(entry);
   }
+
   return definition;
 }
 

third_party/blink/renderer/core/html/custom/custom_element_registry_test.cc

@@ -440,41 +440,6 @@ TEST_F(CustomElementRegistryTest, lookupCustomElementDefinition) {
   EXPECT_EQ(nullptr, definition) << "a-a, div should not be registered";
 }
 
-TEST_F(CustomElementRegistryTest, defineCustomElementWithStyle) {
-  RuntimeEnabledFeatures::SetConstructableStylesheetsEnabled(true);
-  V8TestingScope scope;
-  NonThrowableExceptionState should_not_throw;
-  ElementDefinitionOptions options;
-  CSSStyleSheet* sheet = GetDocument().createEmptyCSSStyleSheet(
-      scope.GetScriptState(), CSSStyleSheetInit(), should_not_throw);
-  options.setStyle(sheet);
-  TestCustomElementDefinitionBuilder builder(sheet);
-  CustomElementDefinition* definition_a =
-      Define("a-a", builder, options, should_not_throw);
-  EXPECT_EQ(definition_a, Registry().DefinitionForName("a-a"));
-  EXPECT_NE(nullptr, Registry().DefinitionForName("a-a")->DefaultStyleSheet());
-  StyleSheetContents* contents =
-      Registry().DefinitionForName("a-a")->DefaultStyleSheet()->Contents();
-  EXPECT_NE(nullptr, contents);
-  EXPECT_EQ(sheet->Contents()->ChildRules().size(),
-            contents->ChildRules().size());
-  EXPECT_EQ(sheet->Contents()->ImportRules().size(),
-            contents->ImportRules().size());
-  EXPECT_EQ(sheet->Contents()->NamespaceRules().size(),
-            contents->NamespaceRules().size());
-  EXPECT_EQ(sheet->Contents()->OriginalURL(), contents->OriginalURL());
-  EXPECT_EQ(sheet->Contents()->DefaultNamespace(),
-            contents->DefaultNamespace());
-  EXPECT_EQ(sheet->Contents()->HasFontFaceRule(), contents->HasFontFaceRule());
-  EXPECT_EQ(sheet->Contents()->HasViewportRule(), contents->HasViewportRule());
-  EXPECT_EQ(sheet->Contents()->HasMediaQueries(), contents->HasMediaQueries());
-  EXPECT_EQ(sheet->Contents()->HasSyntacticallyValidCSSHeader(),
-            contents->HasSyntacticallyValidCSSHeader());
-  EXPECT_EQ(
-      sheet->ownerRule(),
-      Registry().DefinitionForName("a-a")->DefaultStyleSheet()->ownerRule());
-}
-
 // The embedder may define its own elements via the CustomElementRegistry
 // whose names are not valid custom element names. Ensure that such a
 // definition may be done.

third_party/blink/renderer/core/html/custom/custom_element_test_helpers.cc

@@ -9,7 +9,7 @@ namespace blink {
 CustomElementDefinition* TestCustomElementDefinitionBuilder::Build(
     const CustomElementDescriptor& descriptor,
     CustomElementDefinition::Id) {
-  return new TestCustomElementDefinition(descriptor, default_style_sheet_);
+  return new TestCustomElementDefinition(descriptor);
 }
 
 }  // namespace blink

third_party/blink/renderer/core/html/custom/custom_element_test_helpers.h

@@ -23,7 +23,6 @@
 
 namespace blink {
 
-class CSSStyleSheet;
 class CustomElementDescriptor;
 
 class TestCustomElementDefinitionBuilder
@@ -32,9 +31,6 @@ class TestCustomElementDefinitionBuilder
 
  public:
   TestCustomElementDefinitionBuilder() = default;
-  explicit TestCustomElementDefinitionBuilder(
-      CSSStyleSheet* default_style_sheet)
-      : default_style_sheet_(default_style_sheet) {}
 
   bool CheckConstructorIntrinsics() override { return true; }
   bool CheckConstructorNotRegistered() override { return true; }
@@ -43,24 +39,18 @@ class TestCustomElementDefinitionBuilder
                                  CustomElementDefinition::Id) override;
 
  private:
-  const Member<CSSStyleSheet> default_style_sheet_;
 
   DISALLOW_COPY_AND_ASSIGN(TestCustomElementDefinitionBuilder);
 };
 
 class TestCustomElementDefinition : public CustomElementDefinition {
  public:
-  TestCustomElementDefinition(const CustomElementDescriptor& descriptor,
-                              CSSStyleSheet* default_style_sheet)
-      : CustomElementDefinition(descriptor, default_style_sheet) {}
-
   TestCustomElementDefinition(const CustomElementDescriptor& descriptor)
       : CustomElementDefinition(descriptor) {}
 
   TestCustomElementDefinition(const CustomElementDescriptor& descriptor,
                               HashSet<AtomicString>&& observed_attributes)
       : CustomElementDefinition(descriptor,
-                                nullptr,
                                 std::move(observed_attributes)) {}
 
   ~TestCustomElementDefinition() override = default;