Implement parsing for mask shorthand #13336

canova · 2016-09-20T15:17:36Z

Implement parsing for mask shorthand. It doesn't contain tests yet. I'll write and update the PR.

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

There are tests for these changes

This change is

highfive · 2016-09-20T15:17:40Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/shorthand/mask.mako.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/properties.mako.rs

highfive · 2016-09-20T15:17:41Z

Warning

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

Manishearth · 2016-09-20T15:19:38Z

r? @Manishearth

Manishearth · 2016-09-23T03:27:56Z

@canaltinova I found an easy way to disable the link failure.

I'm not sure if this is the right way to do it though, but it's good enough for now while you iterate, till I figure out something better.

diff --git a/tests/unit/stylo/fakebindings.rs b/tests/unit/stylo/fakebindings.rs
new file mode 100644
index 0000000..ef5e09d
--- /dev/null
+++ b/tests/unit/stylo/fakebindings.rs
@@ -0,0 +1,14 @@
+#[no_mangle]
+pub extern "C" fn Gecko_ReleaseURIArbitraryThread(aPtr: *mut ()) {
+    println!("Stub for Gecko_ReleaseURIArbitraryThread");
+}
+
+#[no_mangle]
+pub extern "C" fn Gecko_ReleasePrincipalArbitraryThread(aPtr: *mut ()) {
+    println!("Stub for Gecko_ReleasePrincipalArbitraryThread");
+}
+
+#[no_mangle]
+pub extern "C" fn Gecko_ReleaseAtom(aPtr: *mut ()) {
+    println!("Stub for Gecko_ReleaseAtom");
+}
\ No newline at end of file
diff --git a/tests/unit/stylo/lib.rs b/tests/unit/stylo/lib.rs
index a74d74d..8e75b0a 100644
--- a/tests/unit/stylo/lib.rs
+++ b/tests/unit/stylo/lib.rs
@@ -8,5 +8,6 @@ extern crate gecko_bindings;
 extern crate style;
 extern crate style_traits;

+pub mod fakebindings;
 mod sanity_checks;
 mod parsing;

Manishearth · 2016-09-23T03:31:46Z

@canaltinova also, looking at the tests, you don't need to test individual serialization like done in the style unit tests.

Just test that the mask shorthand parses to the right components. Perhaps also test that the mask serialization is correct; but you don't need to build mask from scratch the way tests/unit/style/properties/serialization does, just use the parsed values.

Manishearth · 2016-09-23T04:12:33Z

Another option is to continue using the style_tests crate for parsing tests, and compile it in a special mode where all properties are included. Don't touch the stylo tests crate then. I think you should use this one over the previous one for now, since you can share code with the existing parse tests.

From 3ab46561a67dacc18c291012b9f696afb63b3dac Mon Sep 17 00:00:00 2001
From: Manish Goregaokar <manishsmail@gmail.com>
Date: Fri, 23 Sep 2016 09:40:16 +0530
Subject: [PATCH] Run tests with all properties enabled

---
 components/style/Cargo.toml          | 1 +
 components/style/build.rs            | 1 +
 components/style/properties/build.py | 4 +++-
 components/style/properties/data.py  | 7 ++++---
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml
index 27dd885..14e412e 100644
--- a/components/style/Cargo.toml
+++ b/components/style/Cargo.toml
@@ -18,6 +18,7 @@ servo = ["serde", "serde/unstable", "serde_macros", "heapsize", "heapsize_plugin
          "cssparser/heap_size", "cssparser/serde-serialization",
          "selectors/heap_size", "selectors/unstable", "string_cache",
          "url/heap_size", "plugins"]
+testing = []

 [dependencies]
 app_units = "0.3"
diff --git a/components/style/build.rs b/components/style/build.rs
index 23ee9d5..ab385b8 100644
--- a/components/style/build.rs
+++ b/components/style/build.rs
@@ -55,6 +55,7 @@ fn main() {
         .arg(&script)
         .arg(product)
         .arg("style-crate")
+        .arg(if cfg!(feature = "testing") {"testing"} else {"regular"})
         .status()
         .unwrap();
     if !status.success() {
diff --git a/components/style/properties/build.py b/components/style/properties/build.py
index daa9140..6997588 100644
--- a/components/style/properties/build.py
+++ b/components/style/properties/build.py
@@ -23,10 +23,12 @@ def main():
         abort(usage)
     product = sys.argv[1]
     output = sys.argv[2]
+    testing = len(sys.argv) > 3 and sys.argv[3] == "testing"
+
     if product not in ["servo", "gecko"] or output not in ["style-crate", "geckolib", "html"]:
         abort(usage)

-    properties = data.PropertiesData(product=product)
+    properties = data.PropertiesData(product=product, testing=testing)
     rust = render(os.path.join(BASE, "properties.mako.rs"), product=product, data=properties)
     if output == "style-crate":
         write(os.environ["OUT_DIR"], "properties.rs", rust)
diff --git a/components/style/properties/data.py b/components/style/properties/data.py
index 94ae75f..b34a55b 100644
--- a/components/style/properties/data.py
+++ b/components/style/properties/data.py
@@ -160,8 +160,9 @@ class StyleStruct(object):


 class PropertiesData(object):
-    def __init__(self, product):
+    def __init__(self, product, testing):
         self.product = product
+        self.testing = testing
         self.style_structs = []
         self.current_style_struct = None
         self.longhands = []
@@ -179,7 +180,7 @@ class PropertiesData(object):

     def declare_longhand(self, name, products="gecko servo", **kwargs):
         products = products.split()
-        if self.product not in products:
+        if self.product not in products and not self.testing:
             return

         longhand = Longhand(self.current_style_struct, name, **kwargs)
@@ -194,7 +195,7 @@ class PropertiesData(object):

     def declare_shorthand(self, name, sub_properties, products="gecko servo", *args, **kwargs):
         products = products.split()
-        if self.product not in products:
+        if self.product not in products and not self.testing:
             return

         sub_properties = [self.longhands_by_name[s] for s in sub_properties]
-- 
2.8.3

@emilio, any preferences? I like this option because we won't need to keep stubbing out methods.

Manishearth · 2016-09-23T06:14:12Z

Implemented the latter solution in #13386

You should rebase over that PR and just write your tests in the regular style unit tests crate.

canova · 2016-09-24T19:55:59Z

Thanks for the help. I rebased your commits and added use style::properties::shorthands::mask to new mod inside serialization.rs, but when I try to run ./mach test-unit I still get "no mask in style::properties::shorthands" error (Same for longhands). Am I missing something?

Manishearth · 2016-09-25T06:44:32Z

Got your ping about CSSStyleDeclaration -- you may need to temporarily edit CSSStyleDeclaration.webidl to add the mask shorthands and longhands.

However, you don't need to. ./mach build -p style and ./mach test-unit -p style should work without hitting this error. Since it's due to your temporarily enabling mask for both gecko and servo, we don't need to worry about the build failure.

Manishearth · 2016-09-25T15:04:28Z

#13415 should be a more permanent fix for your testing woes.

Manishearth · 2016-09-26T22:06:50Z

Re: your comment in IRC: assert_roundtrip won't work here, since that's just for longhands.

Instead, use the parse function you added, and assert that the Longhands struct formed has the correct values for the fields.

Manishearth · 2016-09-27T08:37:45Z

Code itself looks good, r=me on that, just needs some parse tests.

Manishearth

still needs the parse tests I mentioned, but otherwise okay

Manishearth · 2016-09-27T13:16:28Z

components/style/properties/shorthand/mask.mako.rs

+                try!(write!(dest, " "));
+
+                try!(position.unwrap_or(&mask_position::single_value
+                                                            ::get_initial_specified_value())


nit: indent should be aligned with ::single_value

canova · 2016-09-27T13:39:30Z

I added some tests about you've mentioned and fixed the nit.
I have a doubt about origin and clip. Now, the clip is not setting in parsing function if just origin is set. Clip is setting the same as origin in serialization function. I choose this behavior because background is doing the same. I'm not sure this is the right behavior. What do you think?

Manishearth · 2016-09-27T14:06:59Z

Spec (https://drafts.fxtf.org/css-masking-1/#the-mask, https://drafts.csswg.org/css-backgrounds-3/#the-background) says that a single value should set both origin and clip.

The current code for both is buggy. However, that can be fixed later.

Manishearth · 2016-09-27T14:11:34Z

tests/unit/style/parsing/mask.rs

+    let mut parser = Parser::new("padding-box");
+    let result = mask::parse_value(&context, &mut parser).unwrap();
+
+    assert_eq!(result.mask_origin.unwrap(), parse_longhand!(mask_origin, "padding-box"));


File a followup about the clip/origin issue and leave a comment here linking to that.

Manishearth · 2016-09-27T14:13:12Z

r=me with the bug filed and links

canova · 2016-09-27T18:37:17Z

filed an issue here: #13466 and added comment about it.

Manishearth · 2016-09-27T22:37:00Z

@bors-servo r+


        Implement parsing for mask shorthand


        Add test for mask shorthand


        Add parsing unit tests for mask shorthand

Manishearth · 2016-09-28T22:51:18Z

@bors-servo r+

bors-servo · 2016-09-28T22:51:19Z

📌 Commit 17d99f6 has been approved by Manishearth

canova · 2016-09-28T22:52:05Z

Fixed merge conflicts.

bors-servo · 2016-09-28T23:39:22Z

⌛ Testing commit 17d99f6 with merge b322226...


        Auto merge of #13336 - canaltinova:mask, r=Manishearth

Implement parsing for mask shorthand  Implement parsing for mask shorthand. It doesn't contain tests yet. I'll write and update the PR. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13235 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13336)

bors-servo · 2016-09-29T00:31:41Z

💔 Test failed - linux-rel

highfive · 2016-09-29T00:31:43Z

  ▶ Unexpected subtest result in /html/webappapis/scripting/event-loops/microtask_after_raf.html:
  └ PASS [expected FAIL] Microtask execute immediately after script

Manishearth · 2016-09-29T06:04:10Z

@bors-servo retry

#13501

bors-servo · 2016-09-29T07:22:55Z

⌛ Testing commit 17d99f6 with merge 47fa315...


        Auto merge of #13336 - canaltinova:mask, r=Manishearth

Implement parsing for mask shorthand  Implement parsing for mask shorthand. It doesn't contain tests yet. I'll write and update the PR. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13235 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13336)

bors-servo · 2016-09-29T08:24:28Z

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

Manishearth · 2016-09-29T08:25:35Z

wooo!

canova · 2016-09-29T08:59:41Z

🎉 🎉 🎉

highfive assigned cbrewster Sep 20, 2016

highfive added the S-awaiting-review label Sep 20, 2016

highfive assigned Manishearth and unassigned cbrewster Sep 20, 2016

canova force-pushed the canova:mask branch 3 times, most recently from 076d59a to d66d8ef Sep 26, 2016

canova force-pushed the canova:mask branch 2 times, most recently from 2c40fac to 16d5466 Sep 27, 2016

Manishearth approved these changes Sep 27, 2016

View changes

canova force-pushed the canova:mask branch from 16d5466 to da9ab9c Sep 27, 2016

Manishearth reviewed Sep 27, 2016

View changes

canova force-pushed the canova:mask branch from da9ab9c to 8174102 Sep 27, 2016

canova added 3 commits Sep 20, 2016

Implement parsing for mask shorthand

3b03cd5

Add test for mask shorthand

564b003

Add parsing unit tests for mask shorthand

Loading status checks…

17d99f6

canova force-pushed the canova:mask branch from 8174102 to 17d99f6 Sep 28, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 28, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Sep 28, 2016

highfive removed the S-awaiting-merge label Sep 29, 2016

highfive added the S-tests-failed label Sep 29, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 29, 2016

bors-servo merged commit 17d99f6 into servo:master Sep 29, 2016
2 of 3 checks passed

2 of 3 checks passed

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

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 29, 2016

canova deleted the canova:mask branch Sep 29, 2016

canova mentioned this pull request Oct 2, 2016

Write tests for parsing of background shorthand in style #13548

Closed

servo / servo

Implement parsing for mask shorthand #13336

Implement parsing for mask shorthand #13336

canova commented Sep 20, 2016 •

edited

highfive commented Sep 20, 2016

highfive commented Sep 20, 2016

Manishearth commented Sep 20, 2016

Manishearth commented Sep 23, 2016

Manishearth commented Sep 23, 2016

Manishearth commented Sep 23, 2016 •

edited

Manishearth commented Sep 23, 2016

canova commented Sep 24, 2016

Manishearth commented Sep 25, 2016

Manishearth commented Sep 25, 2016

Manishearth commented Sep 26, 2016

Manishearth commented Sep 27, 2016

Manishearth left a comment

This comment has been minimized.

canova commented Sep 27, 2016 •

edited

Manishearth commented Sep 27, 2016

This comment has been minimized.

Manishearth commented Sep 27, 2016

canova commented Sep 27, 2016

Manishearth commented Sep 27, 2016

Manishearth commented Sep 28, 2016

bors-servo commented Sep 28, 2016

canova commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 29, 2016

highfive commented Sep 29, 2016

Manishearth commented Sep 29, 2016 •

edited

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

Manishearth commented Sep 29, 2016

canova commented Sep 29, 2016

servo / servo

Join GitHub today

Implement parsing for mask shorthand #13336

Implement parsing for mask shorthand #13336

Conversation

canova commented Sep 20, 2016 • edited

highfive commented Sep 20, 2016

highfive commented Sep 20, 2016

Manishearth commented Sep 20, 2016

Manishearth commented Sep 23, 2016

Manishearth commented Sep 23, 2016

Manishearth commented Sep 23, 2016 • edited

Manishearth commented Sep 23, 2016

canova commented Sep 24, 2016

Manishearth commented Sep 25, 2016

Manishearth commented Sep 25, 2016

Manishearth commented Sep 26, 2016

Manishearth commented Sep 27, 2016

Manishearth left a comment

This comment has been minimized.

Manishearth Sep 27, 2016

canova commented Sep 27, 2016 • edited

Manishearth commented Sep 27, 2016

This comment has been minimized.

Manishearth Sep 27, 2016

Manishearth commented Sep 27, 2016

canova commented Sep 27, 2016

Manishearth commented Sep 27, 2016

Manishearth commented Sep 28, 2016

bors-servo commented Sep 28, 2016

canova commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 29, 2016

highfive commented Sep 29, 2016

Manishearth commented Sep 29, 2016 • edited

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

Manishearth commented Sep 29, 2016

canova commented Sep 29, 2016

canova commented Sep 20, 2016 •

edited

Manishearth commented Sep 23, 2016 •

edited

canova commented Sep 27, 2016 •

edited

Manishearth commented Sep 29, 2016 •

edited