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

Implement PerformanceNavigation interface #23203

Merged
merged 1 commit into from Jul 15, 2019
Merged

Implement PerformanceNavigation interface #23203

merged 1 commit into from Jul 15, 2019

Conversation

@iulianR
Copy link
Contributor

iulianR commented Apr 15, 2019

This is my first attempt. Please let me know if I should change anything.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22828 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because not sure

This change is Reviewable

@highfive
Copy link

highfive commented Apr 15, 2019

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

@highfive
Copy link

highfive commented Apr 15, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/Performance.webidl, components/script/dom/mod.rs, components/script/dom/webidls/PerformanceNavigation.webidl, components/script/dom/performance.rs, components/script/dom/performancenavigation.rs
  • @KiChjang: components/script/dom/webidls/Performance.webidl, components/script/dom/mod.rs, components/script/dom/webidls/PerformanceNavigation.webidl, components/script/dom/performance.rs, components/script/dom/performancenavigation.rs
@highfive
Copy link

highfive commented Apr 15, 2019

warning Warning warning

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

jdm commented Apr 15, 2019

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Apr 15, 2019
Implement PerformanceNavigation interface

<!-- Please describe your changes on the following line: -->
This is my first attempt. Please let me know if I should change anything.

---
<!-- 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 #22828 (GitHub issue number if applicable)

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

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

bors-servo commented Apr 15, 2019

Trying commit e232ee6 with merge 8687b1f...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2019

💔 Test failed - linux-rel-css

@iulianR
Copy link
Contributor Author

iulianR commented Apr 15, 2019

I see that I should update the mozzila/interfaces.html with PerformanceNavigation, but I'm not sure what to do with the tests in worker-performance.worker.js. What should they test now?

@jdm
Copy link
Member

jdm commented Apr 15, 2019

There are also a lot of new test results in https://build.servo.org/builders/linux-rel-wpt/builds/12027/steps/shell__4/logs/filtered-wpt-errorsummary.log . I recommend following the instructions at https://github.com/servo/servo/blob/master/tests/wpt/README.md#updating-test-expectations using a log generated from ./mach test-wpt tests/wpt/web-platform-tests/navigation-timing/ --log-raw /tmp/servo.log.

@iulianR
Copy link
Contributor Author

iulianR commented Apr 15, 2019

I also noticed this warning that I don't think was there before. It is, however, not clear to me what it means.

warning: unused attribute
  --> components/script/dom/bindings/conversions.rs:74:5
   |
74 |     rustc_on_unimplemented(message = "The IDL interface `{Self}` is not derived from `{T}`.")
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_attributes)] on by default
@jdm
Copy link
Member

jdm commented Apr 15, 2019

It comes and goes, and is not related to your changes.

@iulianR
Copy link
Contributor Author

iulianR commented Apr 15, 2019

I updated the tests as you asked, although there were more changes than I expected.

@jdm
Copy link
Member

jdm commented Apr 15, 2019

@iulianR Would you mind squashing your commits into one?

@iulianR iulianR force-pushed the iulianR:22828 branch from 4e86195 to fedc46f Apr 15, 2019
@jdm
Copy link
Member

jdm commented Apr 22, 2019

@bors-servo r+
Sorry about the delay!

@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2019

📌 Commit fedc46f has been approved by jdm

@highfive highfive assigned jdm and unassigned avadacatavra Apr 22, 2019
bors-servo added a commit that referenced this pull request Apr 22, 2019
Implement PerformanceNavigation interface

<!-- Please describe your changes on the following line: -->
This is my first attempt. Please let me know if I should change anything.

---
<!-- 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 #22828 (GitHub issue number if applicable)

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

<!-- 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/23203)
<!-- Reviewable:end -->
@iulianR iulianR force-pushed the iulianR:22828 branch from 355df1f to 62f0785 Jun 25, 2019
@jdm
Copy link
Member

jdm commented Jun 28, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Jun 28, 2019

📌 Commit 62f0785 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 28, 2019

Testing commit 62f0785 with merge 500369a...

bors-servo added a commit that referenced this pull request Jun 28, 2019
Implement PerformanceNavigation interface

<!-- Please describe your changes on the following line: -->
This is my first attempt. Please let me know if I should change anything.

---
<!-- 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 #22828 (GitHub issue number if applicable)

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

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

bors-servo commented Jun 28, 2019

💔 Test failed - linux-rel-css

@iulianR
Copy link
Contributor Author

iulianR commented Jun 28, 2019

Hmm, I assumed the problem with the worker was fixed now, so I didn't update the expected test result.

I'm digging into the issue with the worker separately. I recommend updating the expected test results for /workers/worker-performance.worker.html as an expected failure so we can merge these changes.

I will do this now.

@jdm
Copy link
Member

jdm commented Jun 28, 2019

The worker issue is being fixed in #23500 but isn't ready yet.

@jdm
Copy link
Member

jdm commented Jul 12, 2019

@iulianR Are you still planning to finish this? :)

@iulianR
Copy link
Contributor Author

iulianR commented Jul 15, 2019

@jdm Can you tell me how I should run the tests so I can figure out if the change I made (locally) was good? I still don't fully understand the testing framework.

@jdm
Copy link
Member

jdm commented Jul 15, 2019

You can run tests locally like so: ./mach test-wpt tests/wpt/web-platform-tests/workers/worker-performance.any.js

@jdm
Copy link
Member

jdm commented Jul 15, 2019

Since #23500 has merged, let's see if that's good enough, though!
@bors-servo try=wpt

@jdm
Copy link
Member

jdm commented Jul 15, 2019

@bors-servo clean try=wpt retry

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2019

Trying commit 62f0785 with merge 5b800d2...

bors-servo added a commit that referenced this pull request Jul 15, 2019
Implement PerformanceNavigation interface

<!-- Please describe your changes on the following line: -->
This is my first attempt. Please let me know if I should change anything.

---
<!-- 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 #22828 (GitHub issue number if applicable)

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

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

bors-servo commented Jul 15, 2019

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

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2019

Testing commit 62f0785 with merge 005320c...

bors-servo added a commit that referenced this pull request Jul 15, 2019
Implement PerformanceNavigation interface

<!-- Please describe your changes on the following line: -->
This is my first attempt. Please let me know if I should change anything.

---
<!-- 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 #22828 (GitHub issue number if applicable)

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

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

bors-servo commented Jul 15, 2019

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

@bors-servo bors-servo merged commit 62f0785 into servo:master Jul 15, 2019
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@iulianR iulianR deleted the iulianR:22828 branch Jul 16, 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.