Bug 1459497: Refactor the preferred style set stuff in order to move the state away from the loader. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 06 May 2018 14:59:34 +0200
changeset 791898 cd19379e3a10ca506a1beeec6ab7c66987a7dd10
parent 791897 b62d1b948ea1966b58f3f6bbcbfdd84768f70b9b
child 791900 b7cc9b4b6672fbbe8979c88a3a5d13fd51470817
push id108913
push userbmo:emilio@crisal.io
push dateSun, 06 May 2018 14:38:53 +0000
reviewersheycam
bugs1459497
milestone61.0a1
Bug 1459497: Refactor the preferred style set stuff in order to move the state away from the loader. r?heycam The main thing to have into account is that the styleset to use is either mLastStyleSheetSet, or mPreferredStyleSheetSet. This last one gets set from Loader::IsAlternateSheet, which is quite nasty and what I'm trying to remove. MozReview-Commit-ID: BI4P1Chqtli
dom/base/nsDocument.cpp
dom/base/nsIDocument.h
dom/xul/nsXULContentSink.cpp
layout/style/Loader.cpp
layout/style/Loader.h
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -3746,26 +3746,17 @@ nsIDocument::SetHeaderData(nsAtom* aHead
     }
   }
 
   if (aHeaderField == nsGkAtoms::headerContentLanguage) {
     CopyUTF16toUTF8(aData, mContentLanguage);
   }
 
   if (aHeaderField == nsGkAtoms::headerDefaultStyle) {
-    // Only mess with our stylesheets if we don't have a lastStyleSheetSet, per
-    // spec.
-    if (DOMStringIsNull(mLastStyleSheetSet)) {
-      // Calling EnableStyleSheetsForSetInternal, not SetSelectedStyleSheetSet,
-      // per spec.  The idea here is that we're changing our preferred set and
-      // that shouldn't change the value of lastStyleSheetSet.  Also, we're
-      // using the Internal version so we can update the CSSLoader and not have
-      // to worry about null strings.
-      EnableStyleSheetsForSetInternal(aData, true);
-    }
+    SetPreferredStyleSheetSet(aData);
   }
 
   if (aHeaderField == nsGkAtoms::refresh) {
     // We get into this code before we have a script global yet, so get to
     // our container via mDocumentContainer.
     nsCOMPtr<nsIRefreshURI> refresher(mDocumentContainer);
     if (refresher) {
       // Note: using mDocumentURI instead of mBaseURI here, for consistency
@@ -6046,25 +6037,29 @@ nsIDocument::SetSelectedStyleSheetSet(co
 
   // Must update mLastStyleSheetSet before doing anything else with stylesheets
   // or CSSLoaders.
   mLastStyleSheetSet = aSheetSet;
   EnableStyleSheetsForSetInternal(aSheetSet, true);
 }
 
 void
-nsIDocument::GetLastStyleSheetSet(nsAString& aSheetSet)
-{
-  aSheetSet = mLastStyleSheetSet;
-}
-
-void
-nsIDocument::GetPreferredStyleSheetSet(nsAString& aSheetSet)
-{
-  GetHeaderData(nsGkAtoms::headerDefaultStyle, aSheetSet);
+nsIDocument::SetPreferredStyleSheetSet(const nsAString& aSheetSet)
+{
+  mPreferredStyleSheetSet = aSheetSet;
+  // Only mess with our stylesheets if we don't have a lastStyleSheetSet, per
+  // spec.
+  if (DOMStringIsNull(mLastStyleSheetSet)) {
+    // Calling EnableStyleSheetsForSetInternal, not SetSelectedStyleSheetSet,
+    // per spec.  The idea here is that we're changing our preferred set and
+    // that shouldn't change the value of lastStyleSheetSet.  Also, we're
+    // using the Internal version so we can update the CSSLoader and not have
+    // to worry about null strings.
+    EnableStyleSheetsForSetInternal(aSheetSet, true);
+  }
 }
 
 DOMStringList*
 nsIDocument::StyleSheetSets()
 {
   if (!mStyleSheetSetList) {
     mStyleSheetSetList = new nsDOMStyleSheetSetList(this);
   }
@@ -6081,32 +6076,32 @@ nsIDocument::EnableStyleSheetsForSet(con
     // non-null) or to our preferredStyleSheetSet.  And this method doesn't
     // change either of those.
     EnableStyleSheetsForSetInternal(aSheetSet, false);
   }
 }
 
 void
 nsIDocument::EnableStyleSheetsForSetInternal(const nsAString& aSheetSet,
-                                            bool aUpdateCSSLoader)
+                                             bool aUpdateCSSLoader)
 {
   BeginUpdate(UPDATE_STYLE);
   size_t count = SheetCount();
   nsAutoString title;
   for (size_t index = 0; index < count; index++) {
     StyleSheet* sheet = SheetAt(index);
     NS_ASSERTION(sheet, "Null sheet in sheet list!");
 
     sheet->GetTitle(title);
     if (!title.IsEmpty()) {
       sheet->SetEnabled(title.Equals(aSheetSet));
     }
   }
   if (aUpdateCSSLoader) {
-    CSSLoader()->SetPreferredSheet(aSheetSet);
+    CSSLoader()->DocumentStyleSheetSetChanged();
   }
   EndUpdate(UPDATE_STYLE);
 }
 
 void
 nsIDocument::GetCharacterSet(nsAString& aCharacterSet) const
 {
   nsAutoCString charset;
--- a/dom/base/nsIDocument.h
+++ b/dom/base/nsIDocument.h
@@ -3259,18 +3259,31 @@ public:
   }
   mozilla::dom::VisibilityState VisibilityState() const
   {
     return mVisibilityState;
   }
 #endif
   void GetSelectedStyleSheetSet(nsAString& aSheetSet);
   void SetSelectedStyleSheetSet(const nsAString& aSheetSet);
-  void GetLastStyleSheetSet(nsAString& aSheetSet);
-  void GetPreferredStyleSheetSet(nsAString& aSheetSet);
+  void GetLastStyleSheetSet(nsAString& aSheetSet)
+  {
+    aSheetSet = mLastStyleSheetSet;
+  }
+  const nsString& GetCurrentStyleSheetSet() const
+  {
+    return mLastStyleSheetSet.IsEmpty()
+      ? mPreferredStyleSheetSet
+      : mLastStyleSheetSet;
+  }
+  void SetPreferredStyleSheetSet(const nsAString&);
+  void GetPreferredStyleSheetSet(nsAString& aSheetSet)
+  {
+    aSheetSet = mPreferredStyleSheetSet;
+  }
   mozilla::dom::DOMStringList* StyleSheetSets();
   void EnableStyleSheetsForSet(const nsAString& aSheetSet);
 
   /**
    * Retrieve the location of the caret position (DOM node and character
    * offset within that node), given a point.
    *
    * @param aX Horizontal point at which to determine the caret position, in
@@ -3751,16 +3764,18 @@ protected:
                                  nsAtom* aAtom, void* aData);
   static void* UseExistingNameString(nsINode* aRootNode, const nsString* aName);
 
   void MaybeResolveReadyForIdle();
 
   nsCString mReferrer;
   nsString mLastModified;
 
+  nsString mPreferredStyleSheetSet;
+
   nsCOMPtr<nsIURI> mDocumentURI;
   nsCOMPtr<nsIURI> mOriginalURI;
   nsCOMPtr<nsIURI> mChromeXHRDocURI;
   nsCOMPtr<nsIURI> mDocumentBaseURI;
   nsCOMPtr<nsIURI> mChromeXHRDocBaseURI;
 
   // A lazily-constructed URL data for style system to resolve URL value.
   RefPtr<mozilla::URLExtraData> mCachedURLData;
--- a/dom/xul/nsXULContentSink.cpp
+++ b/dom/xul/nsXULContentSink.cpp
@@ -274,39 +274,20 @@ XULContentSinkImpl::GetTarget()
 nsresult
 XULContentSinkImpl::Init(nsIDocument* aDocument,
                          nsXULPrototypeDocument* aPrototype)
 {
     NS_PRECONDITION(aDocument != nullptr, "null ptr");
     if (! aDocument)
         return NS_ERROR_NULL_POINTER;
 
-    nsresult rv;
-
     mDocument    = do_GetWeakReference(aDocument);
     mPrototype   = aPrototype;
 
     mDocumentURL = mPrototype->GetURI();
-
-    // XXX this presumes HTTP header info is already set in document
-    // XXX if it isn't we need to set it here...
-    // XXXbz not like GetHeaderData on the proto doc _does_ anything....
-    nsAutoString preferredStyle;
-    rv = mPrototype->GetHeaderData(nsGkAtoms::headerDefaultStyle,
-                                   preferredStyle);
-    if (NS_FAILED(rv)) return rv;
-
-    if (!preferredStyle.IsEmpty()) {
-        aDocument->SetHeaderData(nsGkAtoms::headerDefaultStyle,
-                                 preferredStyle);
-    }
-
-    // Set the right preferred style on the document's CSSLoader.
-    aDocument->CSSLoader()->SetPreferredSheet(preferredStyle);
-
     mNodeInfoManager = aPrototype->GetNodeInfoManager();
     if (! mNodeInfoManager)
         return NS_ERROR_UNEXPECTED;
 
     mState = eInProlog;
     return NS_OK;
 }
 
--- a/layout/style/Loader.cpp
+++ b/layout/style/Loader.cpp
@@ -390,21 +390,16 @@ Loader::Loader(DocGroup* aDocGroup)
   mDocGroup = aDocGroup;
 }
 
 Loader::Loader(nsIDocument* aDocument)
   : Loader()
 {
   mDocument = aDocument;
   MOZ_ASSERT(mDocument, "We should get a valid document from the caller!");
-
-  // We can just use the preferred set, since there are no sheets in the
-  // document yet (if there are, how did they get there? _we_ load the sheets!)
-  // and hence the selected set makes no sense at this time.
-  mDocument->GetPreferredStyleSheetSet(mPreferredSheet);
 }
 
 Loader::~Loader()
 {
   NS_ASSERTION(!mSheets || mSheets->mLoadingDatas.Count() == 0,
                "How did we get destroyed when there are loading data?");
   NS_ASSERTION(!mSheets || mSheets->mPendingDatas.Count() == 0,
                "How did we get destroyed when there are pending data?");
@@ -420,57 +415,45 @@ Loader::DropDocumentReference(void)
   // Flush out pending datas just so we don't leak by accident.  These
   // loads should short-circuit through the mDocument check in
   // LoadSheet and just end up in SheetComplete immediately
   if (mSheets) {
     StartDeferredLoads();
   }
 }
 
-nsresult
-Loader::SetPreferredSheet(const nsAString& aTitle)
+void
+Loader::DocumentStyleSheetSetChanged()
 {
-#ifdef DEBUG
-  if (mDocument) {
-    nsAutoString currentPreferred;
-    mDocument->GetLastStyleSheetSet(currentPreferred);
-    if (DOMStringIsNull(currentPreferred)) {
-      mDocument->GetPreferredStyleSheetSet(currentPreferred);
-    }
-    NS_ASSERTION(currentPreferred.Equals(aTitle),
-                 "Unexpected argument to SetPreferredSheet");
-  }
-#endif
-
-  mPreferredSheet = aTitle;
+  MOZ_ASSERT(mDocument, "Huh?");
 
   // start any pending alternates that aren't alternates anymore
-  if (mSheets) {
-    LoadDataArray arr(mSheets->mPendingDatas.Count());
-    for (auto iter = mSheets->mPendingDatas.Iter(); !iter.Done(); iter.Next()) {
-      SheetLoadData* data = iter.Data();
-      MOZ_ASSERT(data, "Must have a data");
+  if (!mSheets) {
+    return;
+  }
 
-      // Note that we don't want to affect what the selected style set is, so
-      // use true for aHasAlternateRel.
-      auto isAlternate = data->mLoader->IsAlternateSheet(data->mTitle, true);
-      if (isAlternate == IsAlternate::No) {
-        arr.AppendElement(data);
-        iter.Remove();
-      }
-    }
+  LoadDataArray arr(mSheets->mPendingDatas.Count());
+  for (auto iter = mSheets->mPendingDatas.Iter(); !iter.Done(); iter.Next()) {
+    SheetLoadData* data = iter.Data();
+    MOZ_ASSERT(data, "Must have a data");
 
-    mDatasToNotifyOn += arr.Length();
-    for (uint32_t i = 0; i < arr.Length(); ++i) {
-      --mDatasToNotifyOn;
-      LoadSheet(arr[i], eSheetNeedsParser, false);
+    // Note that we don't want to affect what the selected style set is, so
+    // use true for aHasAlternateRel.
+    auto isAlternate = data->mLoader->IsAlternateSheet(data->mTitle, true);
+    if (isAlternate == IsAlternate::No) {
+      arr.AppendElement(data);
+      iter.Remove();
     }
   }
 
-  return NS_OK;
+  mDatasToNotifyOn += arr.Length();
+  for (uint32_t i = 0; i < arr.Length(); ++i) {
+    --mDatasToNotifyOn;
+    LoadSheet(arr[i], eSheetNeedsParser, false);
+  }
 }
 
 static const char kCharsetSym[] = "@charset \"";
 
 static bool GetCharsetFromData(const char* aStyleSheetData,
                                uint32_t aDataLength,
                                nsACString& aCharset)
 {
@@ -847,26 +830,31 @@ Loader::IsAlternateSheet(const nsAString
   // that's a preferred sheet.
   //
   // FIXME(emilio): This should return false for Shadow DOM regardless of the
   // document.
   if (aTitle.IsEmpty()) {
     return IsAlternate::No;
   }
 
-  if (!aHasAlternateRel && mDocument && mPreferredSheet.IsEmpty()) {
-    // There's no preferred set yet, and we now have a sheet with a title.
-    // Make that be the preferred set.
-    mDocument->SetHeaderData(nsGkAtoms::headerDefaultStyle, aTitle);
-    // We're definitely not an alternate.
-    return IsAlternate::No;
-  }
+  if (mDocument) {
+    const nsString& currentSheetSet = mDocument->GetCurrentStyleSheetSet();
+    if (!aHasAlternateRel && currentSheetSet.IsEmpty()) {
+      // There's no preferred set yet, and we now have a sheet with a title.
+      // Make that be the preferred set.
+      // FIXME(emilio): This is kinda wild, can we do it somewhere else?
+      mDocument->SetPreferredStyleSheetSet(aTitle);
+      // We're definitely not an alternate. Also, beware that at this point
+      // currentSheetSet may dangle.
+      return IsAlternate::No;
+    }
 
-  if (aTitle.Equals(mPreferredSheet)) {
-    return IsAlternate::No;
+    if (aTitle.Equals(currentSheetSet)) {
+      return IsAlternate::No;
+    }
   }
 
   return IsAlternate::Yes;
 }
 
 nsresult
 Loader::ObsoleteSheet(nsIURI* aURI)
 {
@@ -2680,17 +2668,16 @@ Loader::SizeOfIncludingThis(mozilla::Mal
   // Measurement of the following members may be added later if DMD finds it is
   // worthwhile:
   // - mLoadingDatas: transient, and should be small
   // - mPendingDatas: transient, and should be small
   // - mPostedEvents: transient, and should be small
   //
   // The following members aren't measured:
   // - mDocument, because it's a weak backpointer
-  // - mPreferredSheet, because it can be a shared string
 
   return n;
 }
 
 void
 Loader::BlockOnload()
 {
   if (mDocument) {
--- a/layout/style/Loader.h
+++ b/layout/style/Loader.h
@@ -211,17 +211,20 @@ public:
   NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(Loader)
   NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(Loader)
 
   void DropDocumentReference(); // notification that doc is going away
 
   void SetCompatibilityMode(nsCompatibility aCompatMode)
   { mCompatMode = aCompatMode; }
   nsCompatibility GetCompatibilityMode() { return mCompatMode; }
-  nsresult SetPreferredSheet(const nsAString& aTitle);
+
+  // TODO(emilio): Is the complexity of this method and carrying the titles
+  // around worth it? The alternate sheets will load anyhow eventually...
+  void DocumentStyleSheetSetChanged();
 
   // XXXbz sort out what the deal is with events!  When should they fire?
 
   /**
    * Load an inline style sheet.  If a successful result is returned and
    * result.WillNotify() is true, then aObserver is guaranteed to be notified
    * asynchronously once the sheet is marked complete.  If an error is
    * returned, or if result.WillNotify() is false, aObserver will not be
@@ -623,17 +626,16 @@ private:
 
   // Number of datas still waiting to be notified on if we're notifying on a
   // whole bunch at once (e.g. in one of the stop methods).  This is used to
   // make sure that HasPendingLoads() won't return false until we're notifying
   // on the last data we're working with.
   uint32_t          mDatasToNotifyOn;
 
   nsCompatibility   mCompatMode;
-  nsString          mPreferredSheet;  // title of preferred sheet
 
   bool              mEnabled; // is enabled to load new styles
 
   nsCOMPtr<nsIConsoleReportCollector> mReporter;
 
 #ifdef DEBUG
   bool              mSyncCallback;
 #endif