Skip to content

Commit

Permalink
Auto merge of #22923 - peterjoel:issue_8539_prefs_refactor, r=jdm
Browse files Browse the repository at this point in the history
#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 20, 2019
2 parents 43cc2a0 + 8bfd4dc commit 5ec7254
Show file tree
Hide file tree
Showing 53 changed files with 1,751 additions and 683 deletions.
12 changes: 12 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions components/bluetooth/lib.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -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()
Expand Down
4 changes: 2 additions & 2 deletions components/canvas/webgl_mode/inprocess.rs
Expand Up @@ -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.
Expand All @@ -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(),
Expand Down
4 changes: 2 additions & 2 deletions components/config/Cargo.toml
Expand Up @@ -9,8 +9,6 @@ publish = false
[lib]
name = "servo_config"
path = "lib.rs"
test = false
doctest = false

[dependencies]
euclid = "0.19"
Expand All @@ -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"
Expand Down
6 changes: 5 additions & 1 deletion components/config/lib.rs
Expand Up @@ -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]
Expand All @@ -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");
Expand Down
54 changes: 25 additions & 29 deletions components/config/opts.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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, ()> {
Expand Down

0 comments on commit 5ec7254

Please sign in to comment.