Move root_font_size to the device #17041

Manishearth · 2017-05-25T18:59:47Z

ComputedValues is heap allocated, and the number of CVs grows with the number of elements, so we should keep it small. The root_font_size is the same for the document, so we should stash it on the device and use that instead.

Gecko actually just manually walks up the tree here, but we can't do that. A simple AtomicRefcell should be enough.

Nothing else can be styled in parallel with the root so the refcell should never panic.

This change is

highfive · 2017-05-25T18:59:52Z

Heads up! This PR modifies the following files:

@bholley: components/style/servo/media_queries.rs, components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/values/specified/length.rs, components/style/properties/properties.mako.rs and 1 more
@emilio: components/style/servo/media_queries.rs, components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/values/specified/length.rs, components/style/properties/properties.mako.rs and 1 more

highfive · 2017-05-25T18:59:53Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style code, but no tests are modified. Please consider adding a test!

Manishearth · 2017-05-25T18:59:58Z

r? @emilio

Manishearth · 2017-05-25T19:00:35Z

First commit is from #17041 since I don't want to have to fix merge conflicts

emilio · 2017-05-25T19:00:39Z

components/style/gecko/media_queries.rs

@@ -35,8 +37,13 @@ pub struct Device {
    pub pres_context: RawGeckoPresContextOwned,
    default_values: Arc<ComputedValues>,
    viewport_override: Option<ViewportConstraints>,
+    /// The font size of the root element
+    root_font_size: AtomicRefCell<Au>,


This can just look in pres_context.mDocument.mCachedDocumentElement instead, right?

emilio · 2017-05-25T19:13:58Z

Also needs the servo side changes

emilio · 2017-05-25T20:16:03Z

components/style/gecko/media_queries.rs

@@ -35,8 +37,13 @@ pub struct Device {
    pub pres_context: RawGeckoPresContextOwned,
    default_values: Arc<ComputedValues>,
    viewport_override: Option<ViewportConstraints>,
+    /// The font size of the root element
+    root_font_size: AtomicIsize,


Add comments on how and when is this set, please.

emilio

With those, r=me

emilio · 2017-05-25T20:28:41Z

components/style/gecko/media_queries.rs

@@ -35,8 +37,15 @@ pub struct Device {
    pub pres_context: RawGeckoPresContextOwned,
    default_values: Arc<ComputedValues>,
    viewport_override: Option<ViewportConstraints>,
+    /// The font size of the root element
+    /// This is set when computing the style of the root
+    /// element, and used for rem units in other elements


Please mention why it's fine to use it like this, that is:

When computing the style of the root element, there can't be any other style being computed at the same time, given we need the style of the parent to compute everything else.

emilio · 2017-05-25T20:29:03Z

components/style/gecko/media_queries.rs

@@ -73,6 +83,16 @@ impl Device {
        &self.default_values
    }

+    /// Get the font size of the root element (for rem)
+    pub fn root_font_size(&self) -> Au {
+        Au::new(self.root_font_size.load(Ordering::AcqRel) as i32)


These can be Relaxed, per the above comment.

Manishearth · 2017-05-25T20:32:59Z

@bors-servo r+ p=2

(contains a second PR, better if they both land together)

bors-servo · 2017-05-25T20:33:00Z

📌 Commit ce2237e has been approved by Manishearth

bors-servo · 2017-05-25T20:33:01Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

Manishearth · 2017-05-25T20:33:31Z

@bors-servo r=emilio

bors-servo · 2017-05-25T20:33:31Z

💡 This pull request was already approved, no need to approve it again.

bors-servo · 2017-05-25T20:33:32Z

📌 Commit ce2237e has been approved by emilio

bors-servo · 2017-05-25T20:33:32Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

bors-servo · 2017-05-25T23:29:54Z

⌛ Testing commit ce2237e with merge a961f6a...

Move root_font_size to the device ComputedValues is heap allocated, and the number of CVs grows with the number of elements, so we should keep it small. The root_font_size is the same for the document, so we should stash it on the device and use that instead. Gecko actually just manually walks up the tree here, but we can't do that. A simple AtomicRefcell should be enough. Nothing else can be styled in parallel with the root so the refcell should never panic.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17041)

bors-servo · 2017-05-26T00:58:44Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: emilio
Pushing a961f6a to master...

MozReview-Commit-ID: 7PzklCUmYpU

MozReview-Commit-ID: 7PzklCUmYpU UltraBlame original commit: 4c561f3ab0343437242986179b9010006ef33437

Putting the font computation data in its own struct

af124f2

highfive assigned emilio May 25, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 25, 2017

emilio reviewed May 25, 2017

View reviewed changes

Manishearth force-pushed the root_font_size branch from 1e1a502 to 00e09ce Compare May 25, 2017 20:13

emilio reviewed May 25, 2017

View reviewed changes

Manishearth force-pushed the root_font_size branch from 00e09ce to f55a199 Compare May 25, 2017 20:23

emilio approved these changes May 25, 2017

View reviewed changes

Move root_font_size to the device

ce2237e

Manishearth force-pushed the root_font_size branch from f55a199 to ce2237e Compare May 25, 2017 20:32

highfive assigned Manishearth and unassigned emilio May 25, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 25, 2017

highfive assigned emilio and unassigned Manishearth May 25, 2017

bors-servo mentioned this pull request May 26, 2017

Putting the font computation data in its own struct #17030

Merged

5 tasks

bors-servo merged commit ce2237e into servo:master May 26, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 26, 2017

weilonge pushed a commit to fx-dev-playground/gecko that referenced this pull request May 26, 2017

Bug 1341102 - Update reftest expectations after servo/servo#17041.

98c6d8a

MozReview-Commit-ID: 7PzklCUmYpU

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 26, 2017

Bug 1341102 - Update reftest expectations after servo/servo#17041.

3b374af

MozReview-Commit-ID: 7PzklCUmYpU

aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request May 26, 2017

Bug 1341102 - Update reftest expectations after servo/servo#17041.

42c4314

MozReview-Commit-ID: 7PzklCUmYpU

Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request May 27, 2017

Bug 1341102 - Update reftest expectations after servo/servo#17041.

3bdc1e5

MozReview-Commit-ID: 7PzklCUmYpU

JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request May 30, 2017

Bug 1341102 - Update reftest expectations after servo/servo#17041.

ae2ff91

MozReview-Commit-ID: 7PzklCUmYpU

Manishearth deleted the root_font_size branch May 7, 2019 22:43

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019

Bug 1341102 - Update reftest expectations after servo/servo#17041.

b3773a0

MozReview-Commit-ID: 7PzklCUmYpU UltraBlame original commit: 4c561f3ab0343437242986179b9010006ef33437

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move root_font_size to the device #17041

Move root_font_size to the device #17041

Manishearth commented May 25, 2017 •

edited by larsbergstrom

highfive commented May 25, 2017

highfive commented May 25, 2017

Manishearth commented May 25, 2017

Manishearth commented May 25, 2017

emilio May 25, 2017

emilio commented May 25, 2017

emilio May 25, 2017

emilio left a comment

emilio May 25, 2017

emilio May 25, 2017

Manishearth commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

Manishearth commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 26, 2017

Move root_font_size to the device #17041

Move root_font_size to the device #17041

Conversation

Manishearth commented May 25, 2017 • edited by larsbergstrom

highfive commented May 25, 2017

highfive commented May 25, 2017

Manishearth commented May 25, 2017

Manishearth commented May 25, 2017

emilio May 25, 2017

Choose a reason for hiding this comment

emilio commented May 25, 2017

emilio May 25, 2017

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

emilio May 25, 2017

Choose a reason for hiding this comment

emilio May 25, 2017

Choose a reason for hiding this comment

Manishearth commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

Manishearth commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 25, 2017

bors-servo commented May 26, 2017

Manishearth commented May 25, 2017 •

edited by larsbergstrom