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

Expose public getter to stylesheet's owner #23073

Merged
merged 1 commit into from Mar 23, 2019
Merged

Conversation

sbansal3096
Copy link
Contributor

@sbansal3096 sbansal3096 commented Mar 21, 2019

  • expose a public getter on CSSStyleSheet to return the stylesheet's owner
  • in CSSRuleList::insert_rule, use the new getter to pass a non-None value as an argument to arc.insert_rule
  • add a test for this that verifies that the new stylesheet is actually loaded

  • There are tests for these changes

This change is Reviewable

@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon.

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/cssstylesheet.rs, components/script/dom/cssrulelist.rs
  • @KiChjang: components/script/dom/cssstylesheet.rs, components/script/dom/cssrulelist.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 21, 2019
@highfive
Copy link

warning Warning warning

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

@sbansal3096
Copy link
Contributor Author

@jdm I am still to add tests. Can you check the work done till now?

@jdm
Copy link
Member

jdm commented Mar 21, 2019

That looks like what I expected to see :)

@sbansal3096
Copy link
Contributor Author

sbansal3096 commented Mar 21, 2019

That looks like what I expected to see :)

@jdm
Cool! Thanks
Can you help me a bit in writing tests? I am new to the community.

@jdm
Copy link
Member

jdm commented Mar 21, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit e01e4a8 with merge b6824cd...

bors-servo pushed a commit that referenced this pull request Mar 21, 2019
Work in Progress

<!-- Please describe your changes on the following line: -->

- [x] expose a public getter on CSSStyleSheet to return the stylesheet's owner
- [x] in CSSRuleList::insert_rule, use the new getter to pass a non-None value as an argument to arc.insert_rule
- [ ] add a test for this that verifies that the new stylesheet is actually loaded
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23028

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/23073)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Mar 21, 2019

The idea of the test we want to write is the following:

  • there is a <style> element in the page
  • we use the insertRule API to add a new rule to this element's stylesheet
  • the new rule inserted is an @import rule that causes a new stylesheet to be loaded

Just having these steps complete successfully will mean that the crash is fixed. However, we also want to test that the imported stylesheet is loaded. Therefore, if the loaded stylesheet contains a CSS style that changes the CSS property of an element on the page, we can compare the expected rendering output against a test page that is supposed to match. This is called a reference test.

We can add the test to tests/wpt/web-platform-tests/css/cssom, and see an example of another reference test in that directory here. We can use the code from the testcase in #23028 as the basis for this new test.

@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 21, 2019
@jdm
Copy link
Member

jdm commented Mar 21, 2019

The nice thing is that there's already a test that exercises this code, and it no longer crashes:

  ▶ OK [expected CRASH] /css/cssom/insertRule-namespace-no-index.html

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Mar 21, 2019
@servo-wpt-sync
Copy link
Collaborator

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15987.

@sbansal3096 sbansal3096 changed the title Work in Progress Expose public getter to stylesheet's owner Mar 21, 2019
@sbansal3096
Copy link
Contributor Author

sbansal3096 commented Mar 21, 2019

@jdm I added test on the basis of the test case you provided me.

@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15987.

@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15987.

@CYBAI
Copy link
Member

CYBAI commented Mar 22, 2019

@bors-servo try=wpt

  • Let's try to see the test result! 👀

@bors-servo
Copy link
Contributor

⌛ Trying commit 66225fa with merge 9d87c1c...

bors-servo pushed a commit that referenced this pull request Mar 22, 2019
Expose public getter to stylesheet's owner

<!-- Please describe your changes on the following line: -->

- [x] expose a public getter on CSSStyleSheet to return the stylesheet's owner
- [x] in CSSRuleList::insert_rule, use the new getter to pass a non-None value as an argument to arc.insert_rule
- [x] add a test for this that verifies that the new stylesheet is actually loaded
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23028

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 22, 2019
@CYBAI
Copy link
Member

CYBAI commented Mar 22, 2019

You would want to label this test case as OK :) In this case, you can just remove the tests/wpt/metadata/css/cssom/insertRule-namespace-no-index.html.ini file!

{
    "status": "OK", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/css/cssom/insertRule-namespace-no-index.html", 
    "line": 86887, 
    "action": "test_result", 
    "expected": "CRASH"
}

Also, you'd want to add a spec link to the new WPT test.

$ ./mach test-tidy --no-progress --all
 0:09.77 INFO Diffing old and new manifests /home/travis/build/servo/servo/tests/wpt/mozilla/meta/MANIFEST.json
 0:09.93 INFO Diffing old and new manifests /home/travis/build/servo/servo/tests/wpt/metadata/MANIFEST.json
 0:16.46 INFO Diffing old and new manifests /home/travis/build/servo/servo/tests/wpt/webgl/meta/MANIFEST.json
Checking the config file...
Checking the wpt manifest file...
Checking directories for correct file extensions...
Running the WPT lint on ./tests/wpt/web-platform-tests/...
{"path": "css/cssom/stylesheet-owner.html", "message": "Testcase file must have a link to a spec", "lineno": null, "rule": "MISSING-LINK"}
WPT Lint Tool:: lint error(s) in Web Platform Tests: exit status 1
Running the WPT lint on ./tests/wpt/mozilla/tests/...
...
The command "./mach test-tidy --no-progress --all" exited with 1.

Besides, could you squash all the commits :)?

@bors-servo
Copy link
Contributor

⌛ Trying commit 13eebbf with merge 549b5ec...

bors-servo pushed a commit that referenced this pull request Mar 22, 2019
Expose public getter to stylesheet's owner

<!-- Please describe your changes on the following line: -->

- [x] expose a public getter on CSSStyleSheet to return the stylesheet's owner
- [x] in CSSRuleList::insert_rule, use the new getter to pass a non-None value as an argument to arc.insert_rule
- [x] add a test for this that verifies that the new stylesheet is actually loaded
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23028

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 22, 2019
@CYBAI
Copy link
Member

CYBAI commented Mar 22, 2019

💔 Test failed - linux-rel-css

Failed with #13480

Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, sounds like Servo still doesn't support ownerNode, right? If so, at that point self.owner may need to become an Option<> ... But anyhow, this looks good on its own. Thanks for fixing this!

@emilio
Copy link
Member

emilio commented Mar 22, 2019

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 13eebbf has been approved by emilio

@highfive highfive assigned emilio and unassigned paulrouget Mar 22, 2019
@highfive 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. S-tests-failed The changes caused existing tests to fail. labels Mar 22, 2019
@bors-servo
Copy link
Contributor

⌛ Testing commit 13eebbf with merge 7e5758b...

bors-servo pushed a commit that referenced this pull request Mar 22, 2019
Expose public getter to stylesheet's owner

<!-- Please describe your changes on the following line: -->

- [x] expose a public getter on CSSStyleSheet to return the stylesheet's owner
- [x] in CSSRuleList::insert_rule, use the new getter to pass a non-None value as an argument to arc.insert_rule
- [x] add a test for this that verifies that the new stylesheet is actually loaded
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23028

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

💔 Test failed - status-taskcluster

@highfive 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 Mar 22, 2019
@CYBAI
Copy link
Member

CYBAI commented Mar 22, 2019

@bors-servo retry

@bors-servo
Copy link
Contributor

💣 Failed to start rebuilding: Unknown error

@bors-servo
Copy link
Contributor

⌛ Testing commit 13eebbf with merge 059ac12...

bors-servo pushed a commit that referenced this pull request Mar 22, 2019
Expose public getter to stylesheet's owner

<!-- Please describe your changes on the following line: -->

- [x] expose a public getter on CSSStyleSheet to return the stylesheet's owner
- [x] in CSSRuleList::insert_rule, use the new getter to pass a non-None value as an argument to arc.insert_rule
- [x] add a test for this that verifies that the new stylesheet is actually loaded
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23028

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/23073)
<!-- Reviewable:end -->
@highfive 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 Mar 22, 2019
@bors-servo
Copy link
Contributor

☀️ 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
Pushing 059ac12 to master...

@bors-servo bors-servo merged commit 13eebbf into servo:master Mar 23, 2019
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 23, 2019
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1553302774944.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

styleSheet.insertRule('@import url("<url>");', 0) crashes servo
8 participants