Bug 1459497: Refactor the preferred style set stuff in order to move the state away from the loader. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 06 May 2018 14:59:34 +0200
changeset 473200 f240bdfb1d699c7b1bde2acf615250fa926485be
parent 473199 b663c7fd766c422bc8935f2b59d78cb46b2f2b85
child 473201 d78b83757f4c1ede11b940347743a3954053b3b1
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1459497
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -4432,16 +4445,18 @@ protected:
   nsCOMPtr<nsIRunnable> mMaybeEndOutermostXBLUpdateRunner;
   nsCOMPtr<nsIRequest> mOnloadBlocker;
 
   nsTArray<RefPtr<mozilla::StyleSheet>> mOnDemandBuiltInUASheets;
   nsTArray<RefPtr<mozilla::StyleSheet>> mAdditionalSheets[AdditionalSheetTypeCount];
 
   // Member to store out last-selected stylesheet set.
   nsString mLastStyleSheetSet;
+  nsString mPreferredStyleSheetSet;
+
   RefPtr<nsDOMStyleSheetSetList> mStyleSheetSetList;
 
   // We lazily calculate declaration blocks for SVG elements with mapped
   // attributes in Servo mode. This list contains all elements which need lazy
   // resolution.
   nsTHashtable<nsPtrHashKey<nsSVGElement>> mLazySVGPresElements;
 
   // Restyle root for servo's style system.
--- 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);
 
   // 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