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.

Sign up for GitHub

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

Jump to bottom

Placed worklets implementation behind a pref #17763

Merged
merged 1 commit into from Jul 18, 2017
Merged

Placed worklets implementation behind a pref #17763

merged 1 commit into from Jul 18, 2017

Conversation

asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 17, 2017 •
edited by larsbergstrom

At the moment, worklets are always exposed, even though their spec is still a draft. This PR hides them behind a pref.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix Worklets: hide them behind a pref? #17680.
  • These changes do not require tests because the existing worklet tests do the job.

This change is Reviewable

@asajeffrey asajeffrey added A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/script Related to the script thread labels Jul 17, 2017
@highfive
Copy link

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/webidls/StylePropertyMapReadOnly.webidl, components/script/dom/webidls/PaintWorkletGlobalScope.webidl, components/script/dom/webidls/PaintRenderingContext2D.webidl, components/script/dom/webidls/WorkletGlobalScope.webidl, components/script/dom/webidls/TestWorkletGlobalScope.webidl and 4 more
  • @KiChjang: components/script/dom/webidls/StylePropertyMapReadOnly.webidl, components/script/dom/webidls/PaintWorkletGlobalScope.webidl, components/script/dom/webidls/PaintRenderingContext2D.webidl, components/script/dom/webidls/WorkletGlobalScope.webidl, components/script/dom/webidls/TestWorkletGlobalScope.webidl and 4 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 17, 2017
@asajeffrey
Copy link
Member Author

r? @SimonSapin

@highfive highfive assigned SimonSapin and unassigned pcwalton Jul 17, 2017
@SimonSapin
Copy link
Member

Looks good to me, but I’d prefer to have this double-checked but someone who knows the first thing about our WebIDL codegen. r? @jdm

@SimonSapin SimonSapin assigned jdm and unassigned SimonSapin Jul 17, 2017
@jdm
Copy link
Member

jdm commented Jul 17, 2017

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 955c84b has been approved by jdm

@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. labels Jul 17, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 955c84b with merge 2b71f8f...

bors-servo pushed a commit that referenced this pull request Jul 18, 2017
Auto merge of #17763 - asajeffrey:script-worklets-pref, r=jdm
2b71f8f 
Placed worklets implementation behind a pref

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

At the moment, worklets are always exposed, even though their spec is still a draft. This PR hides them behind a pref.

---
<!-- 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 #17680.
- [X] These changes do not require tests because the existing worklet tests do the job.

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

💔 Test failed - linux-rel-wpt

@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 Jul 18, 2017
@jdm
Copy link
Member

jdm commented Jul 18, 2017 •
edited 

  ▶ Unexpected subtest result in /_mozilla/mozilla/interfaces.html:
  │ FAIL [expected PASS] Interfaces exposed on the window
  │   → assert_true: Worklet should be defined on the global scope expected true got false
  │ 
  │ test_interfaces/<@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:83:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  │ test_interfaces@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:2:3
  └ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:13:1

@asajeffrey
Copy link
Member Author

Oops, l'll fix that tomorrow. This is a good test for the feature gate working!

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 18, 2017
@asajeffrey
Copy link
Member Author

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

📌 Commit a3fe0b7 has been approved by jdm

@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. labels Jul 18, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit a3fe0b7 with merge 92eeec2...

bors-servo pushed a commit that referenced this pull request Jul 18, 2017
Auto merge of #17763 - asajeffrey:script-worklets-pref, r=jdm
92eeec2 
Placed worklets implementation behind a pref

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

At the moment, worklets are always exposed, even though their spec is still a draft. This PR hides them behind a pref.

---
<!-- 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 #17680.
- [X] These changes do not require tests because the existing worklet tests do the job.

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

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 92eeec2 to master...

@bors-servo bors-servo merged commit a3fe0b7 into servo:master Jul 18, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/script Related to the script thread
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Worklets: hide them behind a pref?
6 participants
@asajeffrey @highfive @SimonSapin @jdm @bors-servo @pcwalton