Refactor `util::prefs` operations to be methods on static struct. #12178

frewsxcv · 2016-07-02T18:00:09Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-07-02T18:00:13Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/properties.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/stylesheets.rs
@asajeffrey: components/constellation/pipeline.rs, components/webdriver_server/lib.rs, components/constellation/constellation.rs
@jgraham: components/webdriver_server/lib.rs
@KiChjang: components/script/dom/bindings/guard.rs, components/script/dom/htmlmetaelement.rs, components/script/dom/mouseevent.rs, components/script/dom/window.rs, components/script/dom/document.rs, components/script/script_runtime.rs, components/script/dom/testbinding.rs, components/script/timers.rs, components/net/http_loader.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/htmliframeelement.rs, components/net/resource_thread.rs, components/script/dom/htmlanchorelement.rs, components/script/dom/serviceworkerglobalscope.rs

frewsxcv · 2016-07-02T18:05:34Z

I was never a fan of the assortment of functions in components/util/pref.rs. I publicized the static datastructure in the module and added some methods around it. I think it's cleaner now, but I don't feel strongly. Just experimenting here.

cbrewster · 2016-07-02T18:26:35Z

Tidy:

Checking files for tidiness...
./components/compositing/compositor.rs:53: use statement is not in alphabetical order
    expected: util::print_tree::PrintTree
    found: util::opts
./components/servo/lib.rs:85: use statement is not in alphabetical order
    expected: util::resource_files::resources_dir_path
    found: util::opts

bors-servo · 2016-07-02T20:29:06Z

☔ The latest upstream changes (presumably #12136) made this pull request unmergeable. Please resolve the merge conflicts.


        Refactor `util::prefs` operations to be methods on static struct.

emilio · 2016-07-02T20:47:49Z

components/util/prefs.rs

@@ -14,9 +14,9 @@ use std::path::PathBuf;
 use std::sync::{Arc, Mutex};

 lazy_static! {
-    static ref PREFS: Arc<Mutex<HashMap<String, Pref>>> = {
+    pub static ref PREFS: Preferences = {
        let prefs = read_prefs().unwrap_or(HashMap::new());


now you're at it, could you change this unwrap_or(HashMap::new()) for unwrap_or_else(HashMap::new)?

It's a nit though, so feel free to not do it :)

emilio · 2016-07-02T20:49:21Z

components/util/prefs.rs

        let prefs = read_prefs().unwrap_or(HashMap::new());
-        Arc::new(Mutex::new(prefs))
+        Preferences(Arc::new(Mutex::new(prefs)))


Actually, I'm wondering if we could use a RWLock instead of a Mutex, this should avoid locking the world when reading preferences, which is the common case.

Valid suggestion, though could we make that a separate PR?

emilio · 2016-07-02T20:55:39Z

maybe .ok().unwrap_or_else(HashMap::new) is a bit cleaner? Not too much though, so feel free to r=me with or without that.

@bors-servo: delegate+

bors-servo · 2016-07-02T20:55:40Z

✌️ @frewsxcv can now approve this pull request


        Only allocate a `HashMap` if we need to.

frewsxcv · 2016-07-02T20:58:24Z

@bors-servo r=emilio

bors-servo · 2016-07-02T20:58:24Z

📌 Commit d9c9d4a has been approved by emilio

bors-servo · 2016-07-03T15:19:05Z

⌛ Testing commit d9c9d4a with merge b0a8ce5...


        Auto merge of #12178 - frewsxcv:prefs, r=emilio

Refactor `util::prefs` operations to be methods on static struct.  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12178)

bors-servo · 2016-07-03T16:56:20Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned SimonSapin Jul 2, 2016

highfive added the S-awaiting-review label Jul 2, 2016

cbrewster added the S-fails-tidy label Jul 2, 2016

frewsxcv force-pushed the frewsxcv:prefs branch from 66d7eaf to 672c138 Jul 2, 2016

frewsxcv removed the S-fails-tidy label Jul 2, 2016

highfive added the S-needs-rebase label Jul 2, 2016

Refactor `util::prefs` operations to be methods on static struct.

Loading status checks…

22928f5

frewsxcv force-pushed the frewsxcv:prefs branch from 672c138 to 22928f5 Jul 2, 2016

emilio reviewed Jul 2, 2016
View changes

frewsxcv force-pushed the frewsxcv:prefs branch from eeada7d to 6bc0f58 Jul 2, 2016

emilio removed S-awaiting-review S-needs-rebase labels Jul 2, 2016

emilio assigned emilio and unassigned SimonSapin Jul 2, 2016

frewsxcv force-pushed the frewsxcv:prefs branch from 6bc0f58 to 272dae9 Jul 2, 2016

highfive added the S-awaiting-review label Jul 2, 2016

Only allocate a `HashMap` if we need to.

Loading status checks…

d9c9d4a

frewsxcv force-pushed the frewsxcv:prefs branch from 272dae9 to d9c9d4a Jul 2, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 2, 2016

bors-servo merged commit d9c9d4a into servo:master Jul 3, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 3, 2016

This was referenced Jul 3, 2016

Send log messages to the constellation #11841

Merged

Implement joint session history #11866

Merged

Switch the default renderer to WebRender #11923

Closed

Support non-QWERTY keyboards #11950

Merged

Excise SubpageId and use only PipelineIds #11698

Merged

frewsxcv deleted the frewsxcv:prefs branch Jul 3, 2016

servo / servo

Refactor `util::prefs` operations to be methods on static struct. #12178

Refactor `util::prefs` operations to be methods on static struct. #12178

frewsxcv commented Jul 2, 2016 •

edited by larsbergstrom

highfive commented Jul 2, 2016

frewsxcv commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 2, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Jul 2, 2016

bors-servo commented Jul 2, 2016

frewsxcv commented Jul 2, 2016

bors-servo commented Jul 2, 2016

bors-servo commented Jul 3, 2016

bors-servo commented Jul 3, 2016

servo / servo

Join GitHub today

Refactor `util::prefs` operations to be methods on static struct. #12178

Refactor `util::prefs` operations to be methods on static struct. #12178

Conversation

frewsxcv commented Jul 2, 2016 • edited by larsbergstrom

highfive commented Jul 2, 2016

frewsxcv commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 2, 2016

This comment has been minimized.

emilio Jul 2, 2016

This comment has been minimized.

frewsxcv Jul 2, 2016

This comment has been minimized.

emilio Jul 2, 2016

This comment has been minimized.

frewsxcv Jul 2, 2016

This comment has been minimized.

emilio Jul 2, 2016

emilio commented Jul 2, 2016

bors-servo commented Jul 2, 2016

frewsxcv commented Jul 2, 2016

bors-servo commented Jul 2, 2016

bors-servo commented Jul 3, 2016

bors-servo commented Jul 3, 2016

frewsxcv commented Jul 2, 2016 •

edited by larsbergstrom