Bug 1550422 - P27. Do not set WebRender preferences as code don't expect them to exists. r=cpearce
☠☠ backed out by af54b2de7028 ☠ ☠
authorJean-Yves Avenard <jyavenard@mozilla.com>
Fri, 24 May 2019 11:40:04 +0000
changeset 535185 0b40296717100fa26c8f7bdea8e365eeb523cfbd
parent 535184 a162952960355c80a50bc502aae46912b90fd7b8
child 535186 6dc17248fa2c34195c97123f3d0e3916a94e8883
push id11522
push userffxbld-merge
push dateMon, 01 Jul 2019 09:00:55 +0000
treeherdermozilla-beta@53ea74d2bd09 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce
bugs1550422
milestone69.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 1550422 - P27. Do not set WebRender preferences as code don't expect them to exists. r=cpearce Listing the preferences in either all.js or StaticPrefList.h would also make them appear in about:config which is something we don't want. Additionally, rename some pref constants to improve code clarity as we can no longer rely on using the StaticPrefs accessor. Differential Revision: https://phabricator.services.mozilla.com/D32416
gfx/thebes/gfxPlatform.cpp
modules/libpref/init/StaticPrefList.h
--- a/gfx/thebes/gfxPlatform.cpp
+++ b/gfx/thebes/gfxPlatform.cpp
@@ -745,18 +745,20 @@ WebRenderMemoryReporter::CollectReports(
 
   return NS_OK;
 }
 
 #undef REPORT_INTERNER
 #undef REPORT_DATA_STORE
 
 static const char* const WR_ROLLOUT_PREF = "gfx.webrender.all.qualified";
-static const char* const WR_ROLLOUT_PREF_DEFAULT =
+static const bool WR_ROLLOUT_PREF_DEFAULTVALUE = true;
+static const char* const WR_ROLLOUT_DEFAULT_PREF =
     "gfx.webrender.all.qualified.default";
+static const bool WR_ROLLOUT_DEFAULT_PREF_DEFAULTVALUE = false;
 static const char* const WR_ROLLOUT_PREF_OVERRIDE =
     "gfx.webrender.all.qualified.gfxPref-default-override";
 static const char* const WR_ROLLOUT_HW_QUALIFIED_OVERRIDE =
     "gfx.webrender.all.qualified.hardware-override";
 static const char* const PROFILE_BEFORE_CHANGE_TOPIC = "profile-before-change";
 
 // If the "gfx.webrender.all.qualified" pref is true we want to enable
 // WebRender for qualified hardware. This pref may be set by the Normandy
@@ -814,17 +816,17 @@ class WrRolloutPrefShutdownSaver : publi
       // 1. the user or the WR SHIELD study has set a user pref value, or
       // 2. we've not had a default pref set by Normandy that needs to be saved
       //    for reading before Normandy has started up.
       return;
     }
 
     bool defaultValue =
         Preferences::GetBool(WR_ROLLOUT_PREF, false, PrefValueKind::Default);
-    Preferences::SetBool(WR_ROLLOUT_PREF_DEFAULT, defaultValue);
+    Preferences::SetBool(WR_ROLLOUT_DEFAULT_PREF, defaultValue);
   }
 };
 
 static void FrameRatePrefChanged(const char* aPref, void*) {
   int32_t newRate = gfxPlatform::ForceSoftwareVsync()
                         ? gfxPlatform::GetSoftwareVsyncRate()
                         : -1;
   if (newRate != gLastUsedFrameRate) {
@@ -2515,39 +2517,51 @@ static bool WebRenderEnvvarDisabled() {
 // observer to save the default value on shutdown, and read the saved default
 // value here instead, and emulate the behavior of the pref system, with
 // respect to default/user values of the rollout pref.
 static bool CalculateWrQualifiedPrefValue() {
   auto clearPrefOnExit = MakeScopeExit([]() {
     // Clear the mirror of the default value of the rollout pref on scope exit,
     // if we have one. This ensures the user doesn't mess with the pref.
     // If we need it again, we'll re-create it on shutdown.
-    Preferences::ClearUser(WR_ROLLOUT_PREF_DEFAULT);
+    Preferences::ClearUser(WR_ROLLOUT_DEFAULT_PREF);
   });
 
   if (!Preferences::HasUserValue(WR_ROLLOUT_PREF) &&
-      Preferences::HasUserValue(WR_ROLLOUT_PREF_DEFAULT)) {
+      Preferences::HasUserValue(WR_ROLLOUT_DEFAULT_PREF)) {
     // The user has not set a user pref, and we have a default value set by the
-    // shutdown observer. We should use that instead of the StaticPrefs's
-    // default, as if Normandy had a chance to set it before startup, that is
-    // the value StaticPrefs would return, rather than the default set by
-    // DECL_GFX_PREF.
-    return StaticPrefs::WebRenderAllQualifiedDefault();
+    // shutdown observer. Let's use this as it should be the value Normandy set
+    // before startup. WR_ROLLOUT_DEFAULT_PREF should only be set on shutdown by
+    // the shutdown observer.
+    // Normandy runs *during* startup, but *after* this code here runs (hence
+    // the need for the workaround).
+    // To have a value stored in the WR_ROLLOUT_DEFAULT_PREF pref here, during
+    // the previous run Normandy must have set a default value on the in-memory
+    // pref, and on shutdown we stored the default value in this
+    // WR_ROLLOUT_DEFAULT_PREF user pref. Then once the user restarts, we
+    // observe this pref. Normandy is the only way a default (not user) value
+    // can be set for this pref.
+    return Preferences::GetBool(WR_ROLLOUT_DEFAULT_PREF,
+                                WR_ROLLOUT_DEFAULT_PREF_DEFAULTVALUE);
   }
 
   // We don't have a user value for the rollout pref, and we don't have the
   // value of the rollout pref at last shutdown stored. So we should fallback
-  // to using whatever default is stored in the gfxPref. *But* if we're running
+  // to using the default. *But* if we're running
   // under the Marionette pref rollout work-around test, we may want to override
   // the default value expressed here, so we can test the "default disabled;
   // rollout pref enabled" case.
+  // Note that those preferences can't be defined in all.js nor
+  // StaticPrefsList.h as they would create the pref, leading SaveRolloutPref()
+  // above to abort early as the pref would have a valid type.
+  //  We also don't want those prefs to appear in about:config.
   if (Preferences::HasUserValue(WR_ROLLOUT_PREF_OVERRIDE)) {
     return Preferences::GetBool(WR_ROLLOUT_PREF_OVERRIDE);
   }
-  return StaticPrefs::WebRenderAllQualified();
+  return Preferences::GetBool(WR_ROLLOUT_PREF, WR_ROLLOUT_PREF_DEFAULTVALUE);
 }
 
 static FeatureState& WebRenderHardwareQualificationStatus(
     const IntSize& aScreenSize, bool aHasBattery, nsCString& aOutFailureId) {
   FeatureState& featureWebRenderQualified =
       gfxConfig::GetFeature(Feature::WEBRENDER_QUALIFIED);
   featureWebRenderQualified.EnableByDefault();
 
--- a/modules/libpref/init/StaticPrefList.h
+++ b/modules/libpref/init/StaticPrefList.h
@@ -4604,30 +4604,16 @@ VARCACHE_PREF(
 VARCACHE_PREF(
   Once,
   "gfx.webrender.all",
   WebRenderAll,
   bool, false
 )
 
 VARCACHE_PREF(
-  Once,
-  "gfx.webrender.all.qualified",
-  WebRenderAllQualified,
-  bool, true
-)
-
-VARCACHE_PREF(
-  Once,
-  "gfx.webrender.all.qualified.default",
-  WebRenderAllQualifiedDefault,
-  bool, false
-)
-
-VARCACHE_PREF(
   Live,
   "gfx.webrender.blob-images",
   WebRenderBlobImages,
   RelaxedAtomicBool, true
 )
 
 VARCACHE_PREF(
   Live,