Stylo - gecko glue code for font-size-adjust. #14227

chenpighead · 2016-11-15T12:12:33Z

Implement the gecko-side glue code for font-size-adjust.
This is a followup for #14125, which is originally filed in #13875.

./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-11-15T12:12:38Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs
@emilio: components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs

highfive · 2016-11-15T12:12:39Z

Warning

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

chenpighead · 2016-11-15T12:14:37Z

Though this is part of the Stylo work, it seems to me that there's no gecko change needed. So, I didn't file a bug on Bugzilla. Please let me know if I should do it. cc @Manishearth @heycam

chenpighead · 2016-11-15T12:22:55Z

Stylo try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22c6d9378551e0fff33b6f679a5794801dc8eaa0

wafflespeanut · 2016-11-15T13:21:33Z

components/style/properties/gecko.mako.rs

+        self.gecko.mFont.sizeAdjust = other.gecko.mFont.sizeAdjust;
+    }
+
+    pub fn clone_font_size_adjust(&self) -> longhands::font_size_adjust::computed_value::T {


Do we really need Clone if it's a Copy type?

If I remove Clone, an error occurs while building stylo:

0:33.04 error: no method named `clone_font_size_adjust` found for type `&gecko_properties::GeckoFont` in the current scope 0:33.04 --> /Users/jeremy/stylo/obj-x86_64-apple-darwin16.1.0/toolkit/library/gtest/rust/./x86_64-apple-darwin/debug/build/style-c9cd1e6c6606a30e/out/properties.rs:42421:50 0:33.04 | 0:33.04 42421 | old_style.get_font().clone_font_size_adjust(), 0:33.04 | ^^^^^^^^^^^^^^^^^^^^^^ 0:33.04

Not sure if I should remove the Clone and try to fix this error somewhere else....

Oh never mind. It was a misunderstanding.

clone_ isn't related to Clone, it's usually used for animation. From Servo's POV the style structs store servo computed values, so "cloning" one out in stylo is a more complex operation than a regular clone.

wafflespeanut · 2016-11-15T13:43:16Z

components/style/properties/gecko.mako.rs

+        use properties::longhands::font_size_adjust::computed_value::T;
+        use values::specified::Number;
+
+        T::Number(Number(self.gecko.mFont.sizeAdjust))


In that case, shouldn't we be checking whether we get a -1.0 here?

Yes, we should. Fixed in the next version.

Manishearth · 2016-11-15T17:07:50Z

@bors-servo r+

thanks!

bors-servo · 2016-11-15T17:07:51Z

📌 Commit 32bf5ab has been approved by Manishearth

chenpighead · 2016-11-16T02:35:35Z

Stylo try w/ this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5aeb62873c69a241806cab6c9e6782d769009f5

Stylo try w/o this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ceeadd2a818d663e29d5d20de5ea3046fcbae11

Looks like the failures are not related to this patch.

chenpighead · 2016-11-16T16:18:08Z

Not sure if this is necessary, but here is the screenshot of rendering font-size-adjust property test page from a stylo build.

Manishearth · 2016-11-16T16:21:06Z

(yeah, usually a screenshot is preferred)

chenpighead · 2016-11-16T16:32:34Z

Thank you for the pointers. Is there anything else that I can do to make this PR be merged?

wafflespeanut · 2016-11-16T16:33:58Z

This PR is on its way to getting merged. Please wait till the build succeeds 😄

bors-servo · 2016-11-17T02:55:32Z

⌛ Testing commit 32bf5ab with merge b5e81bf...

Stylo - gecko glue code for font-size-adjust.  Implement the gecko-side glue code for font-size-adjust. This is a followup for #14125, which is originally filed in #13875. ---  - [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 - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14227)

bors-servo · 2016-11-17T03:51:15Z

💔 Test failed - mac-rel-wpt1

wafflespeanut · 2016-11-17T14:57:57Z

@bors-servo retry #11100

bors-servo · 2016-11-17T15:09:24Z

⌛ Testing commit 32bf5ab with merge 45af839...

Stylo - gecko glue code for font-size-adjust.  Implement the gecko-side glue code for font-size-adjust. This is a followup for #14125, which is originally filed in #13875. ---  - [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 - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14227)

bors-servo · 2016-11-17T16:24:02Z

💔 Test failed - mac-rel-wpt1

bors-servo · 2016-11-18T16:15:51Z

💔 Test failed - linux-rel-wpt

wafflespeanut · 2016-11-18T16:55:16Z

@bors-servo retry #14267

bors-servo · 2016-11-18T22:31:04Z

⌛ Testing commit 32bf5ab with merge ced548a...

Stylo - gecko glue code for font-size-adjust.  Implement the gecko-side glue code for font-size-adjust. This is a followup for #14125, which is originally filed in #13875. ---  - [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 - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14227)

bors-servo · 2016-11-18T23:52:00Z

💔 Test failed - mac-rel-wpt1

Manishearth · 2016-11-18T23:54:20Z

@bors-servo retry

Intermittent timeout/crash in /html/dom/reflection-embedded.html #11100

bors-servo · 2016-11-19T00:43:38Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-css, mac-rel-css, mac-rel-wpt1...

bors-servo · 2016-11-19T01:46:13Z

💔 Test failed - linux-rel-css

KiChjang · 2016-11-19T08:44:38Z

@bors-servo retry

Intermittent failure in /css21_dev/html4/pseudo-elements-001.htm #13593

bors-servo · 2016-11-19T11:07:47Z

⌛ Testing commit 32bf5ab with merge a4e26d5...

Stylo - gecko glue code for font-size-adjust.  Implement the gecko-side glue code for font-size-adjust. This is a followup for #14125, which is originally filed in #13875. ---  - [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 - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14227)

bors-servo · 2016-11-19T11:58:59Z

💔 Test failed - mac-rel-wpt1

wafflespeanut · 2016-11-19T12:18:31Z

@bors-servo retry

I hate #11100 😒

bors-servo · 2016-11-19T12:38:03Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-wpt1...

bors-servo · 2016-11-19T13:29:28Z

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

wafflespeanut · 2016-11-19T13:30:26Z

YESSSSSSS!!!! FINALLY!

highfive assigned Manishearth Nov 15, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 15, 2016

wafflespeanut reviewed Nov 15, 2016

View reviewed changes

Stylo - gecko glue code for font-size-adjust.

32bf5ab

chenpighead force-pushed the font-size-adjust-gecko branch from d66ebac to 32bf5ab Compare November 15, 2016 16:53

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 Nov 15, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 17, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 17, 2016

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 18, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 18, 2016

KiChjang mentioned this pull request Nov 18, 2016

Intermittent timeout/crash in /html/dom/reflection-embedded.html #11100

Open

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 18, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 18, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 19, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 19, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 19, 2016

bors-servo merged commit 32bf5ab into servo:master Nov 19, 2016

Stylo - gecko glue code for font-size-adjust. #14227

Stylo - gecko glue code for font-size-adjust. #14227

Conversation

chenpighead commented Nov 15, 2016 • edited

highfive commented Nov 15, 2016

highfive commented Nov 15, 2016

chenpighead commented Nov 15, 2016

chenpighead commented Nov 15, 2016

wafflespeanut Nov 15, 2016

Choose a reason for hiding this comment

chenpighead Nov 15, 2016 • edited

Choose a reason for hiding this comment

wafflespeanut Nov 15, 2016

Choose a reason for hiding this comment

Manishearth Nov 15, 2016

Choose a reason for hiding this comment

wafflespeanut Nov 15, 2016 • edited

Choose a reason for hiding this comment

chenpighead Nov 15, 2016

Choose a reason for hiding this comment

Manishearth commented Nov 15, 2016

bors-servo commented Nov 15, 2016

chenpighead commented Nov 16, 2016

chenpighead commented Nov 16, 2016

Manishearth commented Nov 16, 2016

chenpighead commented Nov 16, 2016

wafflespeanut commented Nov 16, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

wafflespeanut commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 18, 2016

wafflespeanut commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 18, 2016

Manishearth commented Nov 18, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

KiChjang commented Nov 19, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

wafflespeanut commented Nov 19, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

wafflespeanut commented Nov 19, 2016

chenpighead commented Nov 15, 2016 •

edited

chenpighead Nov 15, 2016 •

edited

wafflespeanut Nov 15, 2016 •

edited