Have Element::ensureUserAgentShadowRoot() return a reference

Source/core/dom/shadow/ElementShadow.cpp

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Neither the name of Google Inc. nor the names of its
@@ skipping 113 matching lines @@
     , m_applyAuthorStyles(false)
     , m_needsSelectFeatureSet(false)
 {
 }
 
 ElementShadow::~ElementShadow()
 {
     removeAllShadowRoots();
 }
 
-ShadowRoot* ElementShadow::addShadowRoot(Element& shadowHost, ShadowRoot::ShadowRootType type)
+ShadowRoot& ElementShadow::addShadowRoot(Element& shadowHost, ShadowRoot::ShadowRootType type)
 {
     RefPtr<ShadowRoot> shadowRoot = ShadowRoot::create(&shadowHost.document(), type);
 
     shadowRoot->setParentOrShadowHostNode(&shadowHost);
     shadowRoot->setParentTreeScope(&shadowHost.treeScope());
     m_shadowRoots.push(shadowRoot.get());
     ChildNodeInsertionNotifier(shadowHost).notify(*shadowRoot);
     setNeedsDistributionRecalc();
     shadowHost.lazyReattachIfAttached();
 
     // addShadowRoot() affects apply-author-styles. However, we know that the youngest shadow root has not had any children yet.
     // The youngest shadow root's apply-author-styles is default (false). So we can just set m_applyAuthorStyles false.
     m_applyAuthorStyles = false;
 
     shadowHost.didAddShadowRoot(*shadowRoot);
     InspectorInstrumentation::didPushShadowRoot(&shadowHost, shadowRoot.get());
 
-    return shadowRoot.get();
+    ASSERT(m_shadowRoots.head());

[eseidel, 2013/11/07 23:30:07]

Did you want to ASSERT that m_shadowRoots.head() == shadowRoot.get()?
I guess we could also ASSERT(m_shadowRoots->refCount() > 1) if we really wanted
to.  The ownership in thsi function is just so screwy.

[adamk, 2013/11/08 00:01:51]

The ASSERT you suggest will fail, so it can't be added without making some
actual functional changes to this code.

[Inactive, 2013/11/08 15:09:38]

Well, I really wanted this assertion because I want to make sure I never
dereference a null pointer. However, adding an assertion to make sure
m_shadowRoots.head() == shadowRoot.get() makes a lot of sense as well. I will
add it before landing, thanks.

+    return *m_shadowRoots.head();
 }
 
 void ElementShadow::removeAllShadowRoots()
 {
     // Dont protect this ref count.
     Element* shadowHost = host();
     ASSERT(shadowHost);
 
     while (RefPtr<ShadowRoot> oldRoot = m_shadowRoots.head()) {
         InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get());
@@ skipping 197 matching lines @@
 
 void ElementShadow::clearDistribution()
 {
     m_nodeToInsertionPoints.clear();
 
     for (ShadowRoot* root = youngestShadowRoot(); root; root = root->olderShadowRoot())
         root->setShadowInsertionPointOfYoungerShadowRoot(0);
 }
 
 } // namespace