Bug 1435942 - Fix buggy getters in Preferences.h. r=glandium draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Tue, 06 Feb 2018 17:06:46 +1100
changeset 751396 7259b71a47bb757bd02c55a60bdf408cff91f542
parent 751395 971fe3dace62ec0080fb0eabe05f5f51ac66f188
push id97960
push usernnethercote@mozilla.com
push dateTue, 06 Feb 2018 06:07:08 +0000
reviewersglandium
bugs1435942
milestone60.0a1
Bug 1435942 - Fix buggy getters in Preferences.h. r=glandium They currently fail to pass on `aKind`, always getting the user value (when possible). There are three callsites that are affected: - nsSHistory::Startup, docshell/shistory/nsSHistory.cpp. - FeatureState::SetDefaultFromPref(), in gfx/config/gfxFeature.cpp. - gfxPlatform::InitOMTPConfig(), in gfx/thebes/gfxPlatform.cpp. The patch also adds a gtest that would have failed prior to the fix. MozReview-Commit-ID: L0U1XQmPUFc
modules/libpref/Preferences.cpp
modules/libpref/Preferences.h
modules/libpref/test/gtest/Basics.cpp
modules/libpref/test/gtest/moz.build
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -3918,17 +3918,17 @@ Preferences::GetString(const char* aPref
 }
 
 /* static */ nsresult
 Preferences::GetLocalizedCString(const char* aPrefName,
                                  nsACString& aResult,
                                  PrefValueKind aKind)
 {
   nsAutoString result;
-  nsresult rv = GetLocalizedString(aPrefName, result);
+  nsresult rv = GetLocalizedString(aPrefName, result, aKind);
   if (NS_SUCCEEDED(rv)) {
     CopyUTF16toUTF8(result, aResult);
   }
   return rv;
 }
 
 /* static */ nsresult
 Preferences::GetLocalizedString(const char* aPrefName,
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -89,17 +89,18 @@ public:
     NS_ENSURE_TRUE(InitStaticMembers(), nullptr);
     return (aKind == PrefValueKind::Default) ? sPreferences->mDefaultRootBranch
                                              : sPreferences->mRootBranch;
   }
 
   // Gets the type of the pref.
   static int32_t GetType(const char* aPrefName);
 
-  // Fallible value getters.
+  // Fallible value getters. When `aKind` is `User` they will get the user
+  // value if possible, and fall back to the default value otherwise.
   static nsresult GetBool(const char* aPrefName,
                           bool* aResult,
                           PrefValueKind aKind = PrefValueKind::User);
   static nsresult GetInt(const char* aPrefName,
                          int32_t* aResult,
                          PrefValueKind aKind = PrefValueKind::User);
   static nsresult GetUint(const char* aPrefName,
                           uint32_t* aResult,
@@ -124,48 +125,48 @@ public:
                                      nsAString& aResult,
                                      PrefValueKind aKind = PrefValueKind::User);
   static nsresult GetComplex(const char* aPrefName,
                              const nsIID& aType,
                              void** aResult,
                              PrefValueKind aKind = PrefValueKind::User);
 
   // Infallible getters of user or default values, with fallback results on
-  // failure.
-  // Infallible value getters.
+  // failure. When `aKind` is `User` they will get the user value if possible,
+  // and fall back to the default value otherwise.
   static bool GetBool(const char* aPrefName,
                       bool aFallback = false,
                       PrefValueKind aKind = PrefValueKind::User)
   {
     bool result = aFallback;
-    GetBool(aPrefName, &result);
+    GetBool(aPrefName, &result, aKind);
     return result;
   }
   static int32_t GetInt(const char* aPrefName,
                         int32_t aFallback = 0,
                         PrefValueKind aKind = PrefValueKind::User)
   {
     int32_t result = aFallback;
-    GetInt(aPrefName, &result);
+    GetInt(aPrefName, &result, aKind);
     return result;
   }
   static uint32_t GetUint(const char* aPrefName,
                           uint32_t aFallback = 0,
                           PrefValueKind aKind = PrefValueKind::User)
   {
     uint32_t result = aFallback;
-    GetUint(aPrefName, &result);
+    GetUint(aPrefName, &result, aKind);
     return result;
   }
   static float GetFloat(const char* aPrefName,
                         float aFallback = 0.0f,
                         PrefValueKind aKind = PrefValueKind::User)
   {
     float result = aFallback;
-    GetFloat(aPrefName, &result);
+    GetFloat(aPrefName, &result, aKind);
     return result;
   }
 
   // Value setters. These fail if run outside the parent process.
 
   static nsresult SetBool(const char* aPrefName,
                           bool aValue,
                           PrefValueKind aKind = PrefValueKind::User);
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/gtest/Basics.cpp
@@ -0,0 +1,36 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "gtest/gtest.h"
+#include "mozilla/Preferences.h"
+
+using namespace mozilla;
+
+TEST(PrefsBasics, Errors)
+{
+  Preferences::SetBool("foo.bool", true, PrefValueKind::Default);
+  Preferences::SetBool("foo.bool", false, PrefValueKind::User);
+  ASSERT_EQ(Preferences::GetBool("foo.bool", false, PrefValueKind::Default),
+            true);
+  ASSERT_EQ(Preferences::GetBool("foo.bool", true, PrefValueKind::User), false);
+
+  Preferences::SetInt("foo.int", -66, PrefValueKind::Default);
+  Preferences::SetInt("foo.int", -77, PrefValueKind::User);
+  ASSERT_EQ(Preferences::GetInt("foo.int", 1, PrefValueKind::Default), -66);
+  ASSERT_EQ(Preferences::GetInt("foo.int", 1, PrefValueKind::User), -77);
+
+  Preferences::SetUint("foo.uint", 88, PrefValueKind::Default);
+  Preferences::SetUint("foo.uint", 99, PrefValueKind::User);
+  ASSERT_EQ(Preferences::GetUint("foo.uint", 1, PrefValueKind::Default), 88U);
+  ASSERT_EQ(Preferences::GetUint("foo.uint", 1, PrefValueKind::User), 99U);
+
+  Preferences::SetFloat("foo.float", 3.33, PrefValueKind::Default);
+  Preferences::SetFloat("foo.float", 4.44, PrefValueKind::User);
+  ASSERT_FLOAT_EQ(
+    Preferences::GetFloat("foo.float", 1.0, PrefValueKind::Default), 3.33);
+  ASSERT_FLOAT_EQ(Preferences::GetFloat("foo.float", 1.0, PrefValueKind::User),
+                  4.44);
+}
--- a/modules/libpref/test/gtest/moz.build
+++ b/modules/libpref/test/gtest/moz.build
@@ -6,16 +6,17 @@
 
 Library('libpreftests')
 
 LOCAL_INCLUDES += [
     '../..',
 ]
 
 UNIFIED_SOURCES = [
+    'Basics.cpp',
     'CallbackAndVarCacheOrder.cpp',
     'Parser.cpp',
 ]
 
 if CONFIG['CC_TYPE'] in ('clang', 'gcc'):
     CXXFLAGS += ['-Wno-error=shadow']
 
 # THE MOCK_METHOD2 macro from gtest triggers this clang warning and it's hard