Skip to content

Updating performance implementation and putting more measurements in #20459

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

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Mar 28, 2018


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

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/PerformanceResourceTiming.webidl, components/script/dom/performance.rs, components/script/dom/webidls/PerformanceTiming.webidl, components/script/dom/workerglobalscope.rs, components/script/dom/webidls/PerformanceNavigationTiming.webidl and 6 more
  • @fitzgen: components/script/dom/webidls/PerformanceResourceTiming.webidl, components/script/dom/performance.rs, components/script/dom/webidls/PerformanceTiming.webidl, components/script/dom/workerglobalscope.rs, components/script/dom/webidls/PerformanceNavigationTiming.webidl and 6 more
  • @KiChjang: components/script/dom/webidls/PerformanceResourceTiming.webidl, components/script/dom/performance.rs, components/script/dom/webidls/PerformanceTiming.webidl, components/script/dom/workerglobalscope.rs, components/script/dom/webidls/PerformanceNavigationTiming.webidl and 6 more

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

@jdm jdm left a comment

Choose a reason for hiding this comment

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

At least some of the test failures should be addressed by setting up the inheritance of the DOM objects correctly.

#[dom_struct]
pub struct PerformanceResourceTiming {
reflector_: Reflector,
entry: Dom<PerformanceEntry>,
Copy link
Member

Choose a reason for hiding this comment

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

This should be performanceentry: PerformanceEntry instead of the reflector_ and entry members, since this interface inherits from PerformanceEntry (docs).

pub struct PerformanceNavigationTiming {
reflector_: Reflector,
// https://w3c.github.io/navigation-timing/#PerformanceResourceTiming
resource_timing: Dom<PerformanceResourceTiming>,
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be performanceresourcetiming: PerformanceResourceTiming instead of relector_ and resource_timing (docs).

@@ -111,7 +110,6 @@ struct PerformanceObserver {
#[dom_struct]
pub struct Performance {
reflector_: Reflector,
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be eventtarget: EventTarget instead now (docs).

@jdm jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Mar 28, 2018
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 5, 2018
@avadacatavra avadacatavra force-pushed the time-origin branch 3 times, most recently from f060d25 to cc72e24 Compare April 10, 2018 12:38
@avadacatavra
Copy link
Contributor Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 0172928 with merge c4faf85...

bors-servo pushed a commit that referenced this pull request Apr 11, 2018
WIP: Updating performance implementation and putting more measurements in

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

---
<!-- 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 #__ (github issue number if applicable).

<!-- 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/20459)
<!-- 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 Apr 11, 2018
@avadacatavra avadacatavra removed the S-tests-failed The changes caused existing tests to fail. label Apr 13, 2018
@avadacatavra
Copy link
Contributor Author

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Trying commit 0172928 with merge 773c0c4...

bors-servo pushed a commit that referenced this pull request Apr 13, 2018
WIP: Updating performance implementation and putting more measurements in

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

---
<!-- 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 #__ (github issue number if applicable).

<!-- 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/20459)
<!-- 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 Apr 13, 2018
@avadacatavra
Copy link
Contributor Author

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Trying commit 0172928 with merge d7635eb...

bors-servo pushed a commit that referenced this pull request Apr 19, 2018
WIP: Updating performance implementation and putting more measurements in

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

---
<!-- 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 #__ (github issue number if applicable).

<!-- 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/20459)
<!-- 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 Apr 19, 2018
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #20533) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Apr 27, 2018
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label May 1, 2018
@bors-servo
Copy link
Contributor

🔒 Merge conflict

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #22227) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 20, 2018
refactoring with ResourceFetchMetadata

implemented deprecated window.timing functionality

created ResourceTimingListener trait

fixed w3c links in navigation timing

updated include.ini to run resource timing tests on ci
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 20, 2018
@avadacatavra
Copy link
Contributor Author

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 26007fd has been approved by avadacatavra

@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-needs-rebase There are merge conflict errors. labels Nov 20, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit 26007fd with merge 4da5390...

bors-servo pushed a commit that referenced this pull request Nov 20, 2018
Updating performance implementation and putting more measurements in

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

---
<!-- 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 #__ (github issue number if applicable).

<!-- 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/20459)
<!-- 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 Nov 20, 2018
@avadacatavra
Copy link
Contributor Author

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit 26007fd with merge 5da1069...

bors-servo pushed a commit that referenced this pull request Nov 20, 2018
Updating performance implementation and putting more measurements in

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

---
<!-- 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 #__ (github issue number if applicable).

<!-- 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/20459)
<!-- 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 Nov 20, 2018
@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 Nov 20, 2018
@avadacatavra
Copy link
Contributor Author

@bors-servo retry

@bors-servo
Copy link
Contributor

@bors-servo
Copy link
Contributor

☀️ Test successful - android, android-mac, android-x86, 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, magicleap, status-taskcluster, windows-msvc-dev
Approved by: avadacatavra
Pushing 5da1069 to master...

1 similar comment
@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit 26007fd into servo:master Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-tests-failed The changes caused existing tests to fail.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants