Skip to content
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

Stylo: replace product={gecko,servo} with engine={gecko,servo-2013,servo-2020} #23856

Merged
merged 3 commits into from Jul 30, 2019

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Jul 25, 2019

Renaming the variable helped make sure I looked at every use.

Also auto-generate relevant parts of CSSStyleDeclaration.webidl, to make libscript compile accordingly.


This change is Reviewable

@highfive
Copy link

highfive commented Jul 25, 2019

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/servo/post_build_commands.py
  • @emilio: components/style/properties/properties.mako.rs, components/style/properties/longhands/inherited_table.mako.rs, components/style/properties/longhands/outline.mako.rs, components/style/properties/longhands/box.mako.rs, components/style/properties/shorthands/font.mako.rs and 44 more
@highfive
Copy link

highfive commented Jul 25, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style and layout code, but no tests are modified. Please consider adding a test!
@SimonSapin
Copy link
Member Author

SimonSapin commented Jul 25, 2019

This still needs work in the script crate, to compile in --with-layout-2020 mode.

Copy link
Member

emilio left a comment

The new variable name makes more sense, too :)

This looks generally good, let me know when it's ready for a final look.

@@ -615,9 +642,14 @@ def _add_logical_props(data, props):
props.add(prop.name)


def remove(set_, item):
if item in set_:

This comment has been minimized.

Copy link
@emilio

emilio Jul 25, 2019

Member

I was intentionally using remove() so that getting it wrong errored loud, fwiw...

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 26, 2019

Author Member

This is intended to catch a typo in the name, right?

My reasoning was that I don’t want to conditionally remove based on the engine, because we’d forget to edit this back when adding that property to the new layout engine.

I also tried not having a property exist at all before it is implemented in layout, but that’s already a lost cause: various parts of the style and script crate already refer to Rust APIs for specific APIs, so I used a pref to disable parsing instead. I’ll do the same here.


#[cfg(feature = "servo-layout-2020")]
fn unimplemented_in_servo_2020(_context: &ParserContext) -> bool {
servo_config::prefs::pref_map().get("layout.2020.unimplemented").as_bool().unwrap_or(false)

This comment has been minimized.

Copy link
@emilio

emilio Jul 25, 2019

Member

This logic reads backwards... You only parse it if it's unimplemented?

I assume that this is what you want and that that pref would always be false, but why making it a pref at all? You could just stash false here and use parse(condition = "not_servo_2020") or such, which would read a bit nicer?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 25, 2019

Author Member

Oh yeah it’s totally backward :] Yes making it always false based on the build-time flag also works, but since there’s a pref for properties this felt more consistent.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 26, 2019

Author Member

Wait no, it’s not backward. The layout logic for this value is unimplemented. We only parse the keyword if the “enable unimplemented syntax” pref is on. I’ve renamed to parse_unimplemented_in_servo_2020 to hopefully clarify.

This pref is basically never intended to be on, but prefs are what already had support in Mako template for disabling parsing of some properties while still generating a Rust type for their values.

This comment has been minimized.

Copy link
@emilio

emilio Jul 27, 2019

Member

Right, my point is that it reads backwards. I understand what it's doing, it just reads "parse if unimplemented_in_servo_2020", which is confusing, I think. Prefixing it with parse_ makes it a bit less confusing I guess.

@SimonSapin SimonSapin force-pushed the stylo-engines branch 2 times, most recently from f08be6e to ed930b4 Jul 26, 2019
@SimonSapin SimonSapin changed the title WIP Stylo: replace product={gecko,servo} with engine={gecko,servo-2013,servo-2020} Stylo: replace product={gecko,servo} with engine={gecko,servo-2013,servo-2020} Jul 26, 2019
@SimonSapin
Copy link
Member Author

SimonSapin commented Jul 26, 2019

Alright, this is ready for landing now.

r? @nox for the WebIDL-related commit
r? @emilio for the Stylo-related commit in case you have thoughts, but feel free to delegate to @nox.

@highfive highfive assigned nox and unassigned asajeffrey Jul 26, 2019
@SimonSapin SimonSapin force-pushed the stylo-engines branch from ed930b4 to 4f3c7cf Jul 26, 2019
@jdm
Copy link
Member

jdm commented Jul 26, 2019

�[0m�[0m�[1m�[31merror:�[0m failed to run custom build command for `script v0.0.1 (/repo/components/script)`

Caused by:
  process didn't exit successfully: `/repo/target/debug/build/script-c748da99a214968c/build-script-build` (exit code: 101)
--- stderr
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', src/libcore/result.rs:1051:5
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.29/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.29/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:47
   3: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:36
   4: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:200
   5: std::panicking::default_hook
             at src/libstd/panicking.rs:214
   6: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:477
   7: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:384
   8: rust_begin_unwind
             at src/libstd/panicking.rs:311
   9: core::panicking::panic_fmt
             at src/libcore/panicking.rs:85
  10: core::result::unwrap_failed
             at src/libcore/result.rs:1051
  11: core::result::Result<T,E>::unwrap
             at /rustc/273f42b5964c29dda2c5a349dd4655529767b07f/src/libcore/result.rs:852
  12: build_script_build::main
             at components/script/build.rs:21
  13: std::rt::lang_start::{{closure}}
             at /rustc/273f42b5964c29dda2c5a349dd4655529767b07f/src/libstd/rt.rs:64
  14: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:49
  15: std::panicking::try::do_call
             at src/libstd/panicking.rs:296
  16: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:82
  17: std::panicking::try
             at src/libstd/panicking.rs:275
  18: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  19: std::rt::lang_start_internal
             at src/libstd/rt.rs:48
  20: std::rt::lang_start
             at /rustc/273f42b5964c29dda2c5a349dd4655529767b07f/src/libstd/rt.rs:64
  21: main
  22: __libc_start_main
  23: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
@SimonSapin SimonSapin force-pushed the stylo-engines branch from 4f3c7cf to 2cc632e Jul 27, 2019
@SimonSapin
Copy link
Member Author

SimonSapin commented Jul 27, 2019

Added a line in script/build.rs to create the $OUT_DIR/build/ directory before copying a file into it. In my local testing the directory was already there from an earlier build.

@nox
Copy link
Member

nox commented Jul 27, 2019

LGTM, feel free to r=me.

@jdm
Copy link
Member

jdm commented Jul 27, 2019

@bors-servo r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2019

📌 Commit 2cc632e has been approved by nox

bors-servo added a commit that referenced this pull request Jul 27, 2019
Stylo: replace product={gecko,servo} with engine={gecko,servo-2013,servo-2020}

Renaming the variable helped make sure I looked at every use.

Also auto-generate relevant parts of `CSSStyleDeclaration.webidl`, to make libscript compile accordingly.

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

bors-servo commented Jul 27, 2019

Testing commit 2cc632e with merge aa8b9fd...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2019

💔 Test failed - status-taskcluster

@SimonSapin SimonSapin force-pushed the stylo-engines branch from 2cc632e to e2f99a6 Jul 29, 2019
…’ build scripts

… rather than as an extra step after `cargo doc`.
This helps always using the correct set of CSS properties
(for layout 2013 v.s. 2020).
@SimonSapin SimonSapin force-pushed the stylo-engines branch from 46848c4 to 0215d09 Jul 30, 2019
@SimonSapin
Copy link
Member Author

SimonSapin commented Jul 30, 2019

Removed the unused import, and removed the bit of unit test that runs a Python script that doesn’t exist anymore. (Kept the part where it check that the generated JSON file looks reasonable.)

@bors-servo r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2019

📌 Commit 0215d09 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2019

Testing commit 0215d09 with merge 8e7daa7...

bors-servo added a commit that referenced this pull request Jul 30, 2019
Stylo: replace product={gecko,servo} with engine={gecko,servo-2013,servo-2020}

Renaming the variable helped make sure I looked at every use.

Also auto-generate relevant parts of `CSSStyleDeclaration.webidl`, to make libscript compile accordingly.

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

bors-servo commented Jul 30, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: nox
Pushing 8e7daa7 to master...

@bors-servo bors-servo merged commit 0215d09 into master Jul 30, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
emilio added a commit to emilio/servo that referenced this pull request Jul 30, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 30, 2019
… properties based on the style crate. r=nox

Cherry-picked from servo/servo#23856
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 30, 2019
…,servo-2013,servo-2020}. r=emilio,nox

Renaming the variable helped make sure I looked at every use.

Cherry-picked from: servo/servo#23856
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 30, 2019
…ocs as part of crates' build scripts. r=nox

… rather than as an extra step after `cargo doc`.
This helps always using the correct set of CSS properties
(for layout 2013 v.s. 2020).

Cherry-picked from: servo/servo#23856
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jul 30, 2019
… properties based on the style crate. r=nox

Cherry-picked from servo/servo#23856
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jul 30, 2019
…,servo-2013,servo-2020}. r=emilio,nox

Renaming the variable helped make sure I looked at every use.

Cherry-picked from: servo/servo#23856
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jul 30, 2019
…ocs as part of crates' build scripts. r=nox

… rather than as an extra step after `cargo doc`.
This helps always using the correct set of CSS properties
(for layout 2013 v.s. 2020).

Cherry-picked from: servo/servo#23856
@SimonSapin SimonSapin deleted the stylo-engines branch Sep 30, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
… properties based on the style crate. r=nox

Cherry-picked from servo/servo#23856

UltraBlame original commit: 144f4ff252829f1fa36bb74d2663836ce2f7f298
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
…,servo-2013,servo-2020}. r=emilio,nox

Renaming the variable helped make sure I looked at every use.

Cherry-picked from: servo/servo#23856

UltraBlame original commit: d6be422cd134b0cc94f42bfacdd972923a54ccee
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
…ocs as part of crates' build scripts. r=nox

… rather than as an extra step after `cargo doc`.
This helps always using the correct set of CSS properties
(for layout 2013 v.s. 2020).

Cherry-picked from: servo/servo#23856

UltraBlame original commit: 741d67d502bd15ee4c463e99abb290e691813e06
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
… properties based on the style crate. r=nox

Cherry-picked from servo/servo#23856

UltraBlame original commit: 144f4ff252829f1fa36bb74d2663836ce2f7f298
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
…,servo-2013,servo-2020}. r=emilio,nox

Renaming the variable helped make sure I looked at every use.

Cherry-picked from: servo/servo#23856

UltraBlame original commit: d6be422cd134b0cc94f42bfacdd972923a54ccee
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
…ocs as part of crates' build scripts. r=nox

… rather than as an extra step after `cargo doc`.
This helps always using the correct set of CSS properties
(for layout 2013 v.s. 2020).

Cherry-picked from: servo/servo#23856

UltraBlame original commit: 741d67d502bd15ee4c463e99abb290e691813e06
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
… properties based on the style crate. r=nox

Cherry-picked from servo/servo#23856

UltraBlame original commit: 144f4ff252829f1fa36bb74d2663836ce2f7f298
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
…,servo-2013,servo-2020}. r=emilio,nox

Renaming the variable helped make sure I looked at every use.

Cherry-picked from: servo/servo#23856

UltraBlame original commit: d6be422cd134b0cc94f42bfacdd972923a54ccee
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
…ocs as part of crates' build scripts. r=nox

… rather than as an extra step after `cargo doc`.
This helps always using the correct set of CSS properties
(for layout 2013 v.s. 2020).

Cherry-picked from: servo/servo#23856

UltraBlame original commit: 741d67d502bd15ee4c463e99abb290e691813e06
@SimonSapin SimonSapin added this to Done / resolved in Layout 2020 Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Layout 2020
  
Merged / resolved
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.