Skip to content
Permalink
Browse files

Auto merge of #22923 - peterjoel:issue_8539_prefs_refactor, r=asajeffrey

#8539 Config preferences backend restructure

<!-- Please describe your changes on the following line: -->

A procedural macro for generating static structures for use as the backend for config preferences, as well a mapping from string names to accessors.

Preferences can be accessed and updated via a map-like interface with `String` keys, and now also via a convenience macro: `get_pref!(path.to.pref)`. Various `serde`-compatible field attributes are also supported, including renaming the string keys. This could be useful when changing the backend structure without breaking existing usages.

I have added the choice to use `i64` as well as `f64` for storing numbers. As it turns out, none of the existing preferences used non-integer values. Setting a floating point value from a command-line argument requires a decimal point in order to parse correctly.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #8539

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

-----

I have a few outstanding problems or questions:

1. I am unable to get rid of this warning:
    ```
    warning: unnecessary path disambiguator
       --> components/config/prefs.rs:130:51
        |
    130 |         accessor_type = crate::pref_util::Accessor::<Prefs, crate::pref_util::PrefValue>,
        |                                                   ^^ try removing `::`
    ```
    See: https://stackoverflow.com/questions/54710797/how-to-disable-unnecessary-path-disambiguator-warning
2. Several of the preferences in use were not represented in `prefs.json`. Some of these may be in error, but it is hard to tell. For example `js.offthread_compilation.enabled` vs `js.ion.offthread_compilation.enabled` could be different preferences or could be intended to have the same value.
3. Previously, several pieces of code provided default values when accessing a preference that may not be present. For example:
    ```Rust
    let DBL_CLICK_TIMEOUT = Duration::from_millis(
        PREFS
            .get("dom.document.dblclick_timeout")
            .as_u64()
            .unwrap_or(300),
    );
    ```
    Fallback values don't really make sense now and I've added these defaults to `prefs.json`. Does this sound right?
4. I have kept `PrefValue::Missing`, though it doesn't seem very useful any more. An error might be more appropriate now for an incorrect preference key. I've kept this partly because [`webdriver_server` uses it](https://github.com/servo/servo/blob/master/components/webdriver_server/lib.rs#L224).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22923)
<!-- Reviewable:end -->
  • Loading branch information...
bors-servo committed Mar 15, 2019
2 parents bc03d32 + b5dd323 commit 1fe05d9a71c0c81f82f6f29dd16b42259f2b6881
Showing with 1,674 additions and 670 deletions.
  1. +12 −0 Cargo.lock
  2. +2 −2 components/bluetooth/lib.rs
  3. +2 −2 components/canvas/webgl_mode/inprocess.rs
  4. +2 −2 components/config/Cargo.toml
  5. +5 −1 components/config/lib.rs
  6. +25 −29 components/config/opts.rs
  7. +282 −0 components/config/pref_util.rs
  8. +362 −214 components/config/prefs.rs
  9. +50 −19 components/config/tests/opts.rs
  10. +284 −38 components/config/tests/prefs.rs
  11. +19 −0 components/config_plugins/Cargo.toml
  12. +219 −0 components/config_plugins/lib.rs
  13. +149 −0 components/config_plugins/parse.rs
  14. +2 −6 components/constellation/constellation.rs
  15. +4 −4 components/constellation/pipeline.rs
  16. +2 −6 components/layout_thread/lib.rs
  17. +2 −10 components/net/filemanager_thread.rs
  18. +1 −6 components/net/http_cache.rs
  19. +2 −0 components/net/lib.rs
  20. +2 −5 components/net/tests/filemanager_thread.rs
  21. +3 −2 components/script/dom/bindings/codegen/CodegenRust.py
  22. +2 −2 components/script/dom/bindings/guard.rs
  23. +2 −2 components/script/dom/create.rs
  24. +6 −21 components/script/dom/document.rs
  25. +2 −2 components/script/dom/htmlcanvaselement.rs
  26. +4 −5 components/script/dom/htmlmediaelement.rs
  27. +2 −6 components/script/dom/htmlmetaelement.rs
  28. +2 −6 components/script/dom/mouseevent.rs
  29. +2 −5 components/script/dom/paintworkletglobalscope.rs
  30. +2 −6 components/script/dom/permissions.rs
  31. +2 −5 components/script/dom/serviceworkerglobalscope.rs
  32. +2 −6 components/script/dom/servoparser/mod.rs
  33. +7 −4 components/script/dom/testbinding.rs
  34. +2 −5 components/script/dom/webglrenderingcontext.rs
  35. +126 −179 components/script/script_runtime.rs
  36. +2 −5 components/script/serviceworker_manager.rs
  37. +2 −5 components/script/timers.rs
  38. +5 −3 components/servo/lib.rs
  39. +2 −2 components/style/global_style_data.rs
  40. +2 −2 components/style/properties/properties.mako.rs
  41. +2 −5 components/style/stylesheets/viewport_rule.rs
  42. +24 −13 components/webdriver_server/lib.rs
  43. +2 −5 components/webvr/webvr_thread.rs
  44. +6 −7 ports/libsimpleservo/api/src/lib.rs
  45. +3 −8 ports/servo/browser.rs
  46. +2 −3 ports/servo/glutin_app/window.rs
  47. +6 −6 ports/servo/non_android_main.rs
  48. +17 −1 resources/prefs.json
  49. +2 −2 tests/unit/style/stylesheets.rs
  50. +3 −3 tests/unit/style/viewport.rs

Some generated files are not rendered by default. Learn more.

Oops, something went wrong.
@@ -21,7 +21,7 @@ use device::bluetooth::{BluetoothGATTDescriptor, BluetoothGATTService};
use embedder_traits::{EmbedderMsg, EmbedderProxy};
use ipc_channel::ipc::{self, IpcReceiver, IpcSender};
use servo_config::opts;
use servo_config::prefs::PREFS;
use servo_config::pref;
use servo_rand::{self, Rng};
use std::borrow::ToOwned;
use std::collections::{HashMap, HashSet};
@@ -65,7 +65,7 @@ pub trait BluetoothThreadFactory {
impl BluetoothThreadFactory for IpcSender<BluetoothRequest> {
fn new(embedder_proxy: EmbedderProxy) -> IpcSender<BluetoothRequest> {
let (sender, receiver) = ipc::channel().unwrap();
let adapter = if Some(true) == PREFS.get("dom.bluetooth.enabled").as_boolean() {
let adapter = if pref!(dom.bluetooth.enabled) {
BluetoothAdapter::init()
} else {
BluetoothAdapter::init_mock()
@@ -11,7 +11,7 @@ use canvas_traits::webgl::{WebGLSender, WebVRCommand, WebVRRenderHandler};
use euclid::Size2D;
use fnv::FnvHashMap;
use gleam::gl;
use servo_config::prefs::PREFS;
use servo_config::pref;
use std::rc::Rc;

/// WebGL Threading API entry point that lives in the constellation.
@@ -35,7 +35,7 @@ impl WebGLThreads {
webrender_api_sender,
webvr_compositor.map(|c| WebVRRenderWrapper(c)),
);
let output_handler = if PREFS.is_dom_to_texture_enabled() {
let output_handler = if pref!(dom.webgl.dom_to_texture.enabled) {
Some(Box::new(OutputHandler::new(
webrender_gl.clone(),
channel.clone(),
@@ -9,8 +9,6 @@ publish = false
[lib]
name = "servo_config"
path = "lib.rs"
test = false
doctest = false

[dependencies]
euclid = "0.19"
@@ -20,10 +18,12 @@ lazy_static = "1"
log = "0.4"
num_cpus = "1.1.0"
serde = "1.0"
serde_derive = "1.0"
serde_json = "1.0"
servo_geometry = {path = "../geometry"}
servo_url = {path = "../url"}
url = "1.2"
servo_config_plugins = { path = "../config_plugins" }

[dev-dependencies]
env_logger = "0.6"
@@ -2,6 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

#![feature(core_intrinsics)]
#![deny(unsafe_code)]

#[macro_use]
@@ -11,10 +12,13 @@ extern crate log;
#[macro_use]
extern crate serde;

pub mod pref_util;
#[macro_use]
pub mod prefs;

pub mod basedir;
#[allow(unsafe_code)]
pub mod opts;
pub mod prefs;

pub fn servo_version() -> String {
let cargo_version = env!("CARGO_PKG_VERSION");
@@ -5,13 +5,12 @@
//! Configuration options for a single run of the servo application. Created
//! from command line arguments.

use crate::prefs::{self, PrefValue, PREFS};
use crate::prefs::{self, PrefValue};
use euclid::TypedSize2D;
use getopts::Options;
use servo_geometry::DeviceIndependentPixel;
use servo_url::ServoUrl;
use std::borrow::Cow;
use std::cmp;
use std::default::Default;
use std::env;
use std::fs::{self, File};
@@ -975,17 +974,11 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
})
.collect();

let do_not_use_native_titlebar = opt_match.opt_present("b") ||
!PREFS
.get("shell.native-titlebar.enabled")
.as_boolean()
.unwrap();
let do_not_use_native_titlebar =
opt_match.opt_present("b") || !(pref!(shell.native_titlebar.enabled));

let enable_subpixel_text_antialiasing = !debug_options.disable_subpixel_aa &&
PREFS
.get("gfx.subpixel-text-antialiasing.enabled")
.as_boolean()
.unwrap();
let enable_subpixel_text_antialiasing =
!debug_options.disable_subpixel_aa && pref!(gfx.subpixel_text_antialiasing.enabled);

let is_printing_version = opt_match.opt_present("v") || opt_match.opt_present("version");

@@ -1065,15 +1058,7 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
}

if let Some(layout_threads) = layout_threads {
PREFS.set("layout.threads", PrefValue::Number(layout_threads as f64));
} else if let Some(layout_threads) = PREFS.get("layout.threads").as_string() {
PREFS.set(
"layout.threads",
PrefValue::Number(layout_threads.parse::<f64>().unwrap()),
);
} else if *PREFS.get("layout.threads") == PrefValue::Missing {
let layout_threads = cmp::max(num_cpus::get() * 3 / 4, 1);
PREFS.set("layout.threads", PrefValue::Number(layout_threads as f64));
set_pref!(layout.threads, layout_threads as i64);
}

ArgumentParsingResult::ChromeProcess
@@ -1104,15 +1089,26 @@ pub fn get() -> RwLockReadGuard<'static, Opts> {
pub fn parse_pref_from_command_line(pref: &str) {
let split: Vec<&str> = pref.splitn(2, '=').collect();
let pref_name = split[0];
let value = split.get(1);
match value {
Some(&"false") => PREFS.set(pref_name, PrefValue::Boolean(false)),
Some(&"true") | None => PREFS.set(pref_name, PrefValue::Boolean(true)),
Some(value) => match value.parse::<f64>() {
Ok(v) => PREFS.set(pref_name, PrefValue::Number(v)),
Err(_) => PREFS.set(pref_name, PrefValue::String(value.to_string())),
let pref_value = parse_cli_pref_value(split.get(1).cloned());
prefs::pref_map()
.set(pref_name, pref_value)
.expect(format!("Error setting preference: {}", pref).as_str());
}

fn parse_cli_pref_value(input: Option<&str>) -> PrefValue {
match input {
Some("true") | None => PrefValue::Bool(true),
Some("false") => PrefValue::Bool(false),
Some(string) => {
if let Some(int) = string.parse::<i64>().ok() {
PrefValue::Int(int)
} else if let Some(float) = string.parse::<f64>().ok() {
PrefValue::Float(float)
} else {
PrefValue::from(string)
}
},
};
}
}

pub fn parse_url_or_filename(cwd: &Path, input: &str) -> Result<ServoUrl, ()> {
Oops, something went wrong.

0 comments on commit 1fe05d9

Please sign in to comment.
You can’t perform that action at this time.