Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: Sync changes from mozilla-central. #22645

Merged
merged 6 commits into from Jan 8, 2019
Merged

style: Sync changes from mozilla-central. #22645

merged 6 commits into from Jan 8, 2019

Conversation

@emilio
Copy link
Member

emilio commented Jan 7, 2019 •
edited by SimonSapin

See each individual commit for details.

This change is Reviewable
@highfive
Copy link

highfive commented Jan 7, 2019

Heads up! This PR modifies the following files:
@highfive
Copy link

highfive commented Jan 7, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@emilio
style: Simplify computed::LengthOrPercentage and friends.
ca503b4 
This is a first step to share LengthOrPercentage representation between Rust and
Gecko.

We need to preserve whether the value came from a calc() expression, for now at
least, since we do different things depending on whether we're calc or not right
now. See w3c/csswg-drafts#3482 and dependent bugs for
example.

That means that the gecko conversion code needs to handle calc() in a bit of an
awkward way until I change it to not be needed (patches for that incoming in the
next few weeks I hope).

I need to add a hack to exclude other things from the PartialEq implementation
because the new conversion code is less lossy than the old one, and we relied on
the lousiness in AnimationValue comparison (in order to start transitions and
such, in [1] for example).

I expect to remove that manual PartialEq implementation as soon as I'm done with
the conversion.

The less lossy conversion does fix a few serialization bugs for animation values
though, like not loosing 0% values in calc() when interpolating lengths and
percentages, see the two modified tests:

 * property-types.js
 * test_animation_properties.html

Differential Revision: https://phabricator.services.mozilla.com/D15793
@emilio emilio force-pushed the emilio:gecko-sync branch 3 times, most recently from 57d351d to d94faf9 Jan 7, 2019
@emilio
Copy link
Member Author

emilio commented Jan 7, 2019

I think this is mostly straight-forward, but ac8317a probably deserves some review.

r? @jdm / @nox / @SimonSapin for that?

@bors-servo try
@highfive highfive assigned jdm and unassigned paulrouget Jan 7, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2019

⌛ Trying commit d94faf9 with merge 7edb095...
bors-servo added a commit that referenced this pull request Jan 7, 2019
@bors-servo
Auto merge of #22645 - emilio:gecko-sync, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7edb095 
style: Sync changes from mozilla-central.

See each individual commit for details.

<!-- 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/22645)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2019

💔 Test failed - linux-rel-css
@emilio
Copy link
Member Author

emilio commented Jan 7, 2019

Well that's a lot of broken tests, but the few I took a look at was just a logic error in margin collapsing which I introduced. Should be fixed now.

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2019

⌛ Trying commit 995c4b4 with merge 34a4a8c...
bors-servo added a commit that referenced this pull request Jan 7, 2019
@bors-servo
Auto merge of #22645 - emilio:gecko-sync, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
34a4a8c 
style: Sync changes from mozilla-central.

See each individual commit for details.

<!-- 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/22645)
<!-- Reviewable:end -->
@emilio emilio force-pushed the emilio:gecko-sync branch from 995c4b4 to dc6b5d2 Jan 7, 2019
@emilio
Copy link
Member Author

emilio commented Jan 7, 2019

@bors-servo try=wpt

  • Now with tidy passing.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2019

⌛ Trying commit dc6b5d2 with merge a93b623...
bors-servo added a commit that referenced this pull request Jan 7, 2019
@bors-servo
Auto merge of #22645 - emilio:gecko-sync, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a93b623 
style: Sync changes from mozilla-central.

See each individual commit for details.

<!-- 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/22645)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2019

💔 Test failed - linux-rel-css
@emilio emilio force-pushed the emilio:gecko-sync branch from dc6b5d2 to 24fde74 Jan 7, 2019
@highfive highfive removed the S-tests-failed label Jan 7, 2019
@emilio
Copy link
Member Author

emilio commented Jan 7, 2019

Alright, those were two expected new passes \o/.
@emilio
Copy link
Member Author

emilio commented Jan 7, 2019

The hash of the interesting commit now is 83f0286.
@emilio
Copy link
Member Author

emilio commented Jan 7, 2019

cc @mbrubeck, which I forgot about in my previous comment, in case you feel like it :)
@CYBAI
Copy link
Collaborator

CYBAI commented Jan 8, 2019

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2019

⌛ Testing commit 24fde74 with merge 18bc1dc...
bors-servo added a commit that referenced this pull request Jan 8, 2019
@bors-servo
Auto merge of #22645 - emilio:gecko-sync, r=emilio,mbrubeck
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
18bc1dc 
style: Sync changes from mozilla-central.

See each individual commit for details.

<!-- 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/22645)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2019

💔 Test failed - linux-rel-wpt
@CYBAI
Copy link
Collaborator

CYBAI commented Jan 8, 2019

I found this test case FAIL again 👀

{
    "status": "FAIL", 
    "group": "default", 
    "message": "/css/CSS2/normal-flow/block-formatting-context-height-002.xht 293911574a59c7f3947436264557174a97acc7a5\n/css/CSS2/reference/ref-filled-black-96px-square.xht f73d1fa4091b541ed29c5bc4f99358740860d9e8\nTesting 293911574a59c7f3947436264557174a97acc7a5 == f73d1fa4091b541ed29c5bc4f99358740860d9e8", 
    "stack": null, 
    "subtest": null, 
    "test": "/css/CSS2/normal-flow/block-formatting-context-height-002.xht", 
    "line": 12380, 
    "action": "test_result", 
    "expected": "PASS"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "/_mozilla/css/floats_percentage_width_a.html eff35a4d6501e47e30663f90d48e5c67ff1b9036\n/_mozilla/css/floats_percentage_width_ref.html 7f9d03440f202f4fc3640fcba59efac763583039\nTesting eff35a4d6501e47e30663f90d48e5c67ff1b9036 == 7f9d03440f202f4fc3640fcba59efac763583039", 
    "stack": null, 
    "subtest": null, 
    "test": "/_mozilla/css/floats_percentage_width_a.html", 
    "line": 34141, 
    "action": "test_result", 
    "expected": "PASS"
}
emilio added 5 commits Jan 7, 2019
@emilio
style: Fix servo build.
4a31509 
This also fixes a bunch of calc handling issues and such.

Also remove tests that no longer compile and are covered by WPT.
@emilio
style: Rename LengthOrPercentage to LengthPercentage.
daf1f02 
It does not represent `<length> | <percentage>`, but `<length-percentage>`, so
`LengthOrPercentage` is not the right name.

This patch is totally autogenerated using:

rg 'LengthOrPercentage' servo | cut -d : -f 1 | sort | uniq > files
for file in $(cat files); do sed -i "s#LengthOrPercentage#LengthPercentage#g" $file; done

Differential Revision: https://phabricator.services.mozilla.com/D15812
@emilio
style: Manually rename some variables.
80651fd 
lop is not an acceptable variable name for LengthPercentage.

Differential Revision: https://phabricator.services.mozilla.com/D15813
@emilio
style: Fix servo build.
c7f30ad
@emilio
Rustfmt recent changes.
Loading status checks…
2a6cdaa
@emilio emilio force-pushed the emilio:gecko-sync branch from 24fde74 to 2a6cdaa Jan 8, 2019
@emilio
Copy link
Member Author

emilio commented Jan 8, 2019 •
edited

@bors-servo r=emilio,mbrubeck

  • Was a typo in my patch:
diff --git a/components/layout/floats.rs b/components/layout/floats.rs
index e6a9b048a3..5a58e27f9d 100644
--- a/components/layout/floats.rs
+++ b/components/layout/floats.rs
@@ -551,7 +551,7 @@ impl SpeculatedFloatPlacement {
                 let fixed = match inline_size {
                     LengthPercentageOrAuto::Auto => false,
                     LengthPercentageOrAuto::LengthPercentage(ref lp) => {
-                        lp.is_definitely_zero() || lp.maybe_to_used_value(None).is_none()
+                        lp.is_definitely_zero() || lp.maybe_to_used_value(None).is_some()
                     },
                 };
                 if !fixed {
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2019

📌 Commit 2a6cdaa has been approved by emilio,mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2019

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

  • There's another pull request that is currently being tested, blocking this pull request: #22640
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2019

📌 Commit 2a6cdaa has been approved by emilio,mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2019

⌛ Testing commit 2a6cdaa with merge a0ba56c...
bors-servo added a commit that referenced this pull request Jan 8, 2019
@bors-servo
Auto merge of #22645 - emilio:gecko-sync, r=emilio,mbrubeck
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a0ba56c 
style: Sync changes from mozilla-central.

See each individual commit for details.

<!-- 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/22645)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: emilio,mbrubeck
Pushing a0ba56c to master...
@bors-servo bors-servo merged commit 2a6cdaa into servo:master Jan 8, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@emilio emilio deleted the emilio:gecko-sync branch Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@emilio @highfive @bors-servo @CYBAI @jdm @paulrouget
You can’t perform that action at this time.