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

Show warning when module scripts are ignored #24142

Merged
merged 6 commits into from Sep 6, 2019

Conversation

@CYBAI
Copy link
Collaborator

CYBAI commented Sep 5, 2019

The first five commits are cherry-pick-ed from the module script PR.
I think it might be also good to have this PR first so that reviewers can focus on module script things on that PR!


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #24089
  • These changes do not require tests because it just ignored module scripts (or should we turn on module script tests in this PR and update those test expectation to TIMEOUT?)

This change is Reviewable

@highfive
Copy link

highfive commented Sep 5, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmlscriptelement.rs
  • @KiChjang: components/script/dom/htmlscriptelement.rs
@highfive
Copy link

highfive commented Sep 5, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@CYBAI
Copy link
Collaborator Author

CYBAI commented Sep 5, 2019

r? @jdm

@highfive highfive assigned jdm and unassigned Manishearth Sep 5, 2019
@jdm
Copy link
Member

jdm commented Sep 5, 2019

@bors-servo r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

📌 Commit 6b7294e has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 6, 2019

Testing commit 6b7294e with merge bf86cf8...

bors-servo added a commit that referenced this pull request Sep 6, 2019
Show warning when module scripts are ignored

The first five commits are `cherry-pick`-ed from the module script PR.
I think it might be also good to have this PR first so that reviewers can focus on module script things on that PR!

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24089
- [x] These changes do not require tests because it just ignored module scripts (or should we turn on module script tests in this PR and update those test expectation to TIMEOUT?)

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

bors-servo commented Sep 6, 2019

💔 Test failed - linux-rel-wpt

CYBAI added 6 commits May 12, 2019
This is basically revert 965370c. After introducing module script, we
can reuse this struct and add field to see the type of script.
The warning can be removed in #23545.
@CYBAI CYBAI force-pushed the CYBAI:warn-module-script branch from 6b7294e to 95ddfb3 Sep 6, 2019
@CYBAI
Copy link
Collaborator Author

CYBAI commented Sep 6, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Sep 6, 2019

Trying commit 95ddfb3 with merge 02440dd...

bors-servo added a commit that referenced this pull request Sep 6, 2019
Show warning when module scripts are ignored

The first five commits are `cherry-pick`-ed from the module script PR.
I think it might be also good to have this PR first so that reviewers can focus on module script things on that PR!

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24089
- [x] These changes do not require tests because it just ignored module scripts (or should we turn on module script tests in this PR and update those test expectation to TIMEOUT?)

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

bors-servo commented Sep 6, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

@jdm
Copy link
Member

jdm commented Sep 6, 2019

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 6, 2019

📌 Commit 95ddfb3 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 6, 2019

Testing commit 95ddfb3 with merge 7a67261...

bors-servo added a commit that referenced this pull request Sep 6, 2019
Show warning when module scripts are ignored

The first five commits are `cherry-pick`-ed from the module script PR.
I think it might be also good to have this PR first so that reviewers can focus on module script things on that PR!

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24089
- [x] These changes do not require tests because it just ignored module scripts (or should we turn on module script tests in this PR and update those test expectation to TIMEOUT?)

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

bors-servo commented Sep 6, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 7a67261 to master...

@bors-servo bors-servo merged commit 95ddfb3 into servo:master Sep 6, 2019
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@CYBAI CYBAI deleted the CYBAI:warn-module-script branch Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

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