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

Performance interface's timing member as an attribute change #24466

Conversation

@tomasdivito
Copy link
Contributor

tomasdivito commented Oct 16, 2019

In this patch we change the Performance Interface's timing member to be an attribute as the spec requires: https://w3c.github.io/navigation-timing/#extensions-to-the-performance-interface

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23330

  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented Oct 16, 2019

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

@highfive
Copy link

highfive commented Oct 16, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/Performance.webidl
  • @KiChjang: components/script/dom/webidls/Performance.webidl
@highfive
Copy link

highfive commented Oct 16, 2019

warning Warning warning

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

tomasdivito commented Oct 16, 2019

I have yet to check but I believe some tests are broken or at least it gave me that impression when I tried ./mach test-wpt tests/wpt/web-platform-tests/navigation-timing/

@jdm
Copy link
Member

jdm commented Oct 16, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2019

Trying commit 4ba031c with merge 1de5e4d...

bors-servo added a commit that referenced this pull request Oct 16, 2019
…, r=<try>

Performance interface's timing member as an attribute change

In this patch we change the Performance Interface's `timing` member to be an attribute as the spec requires: https://w3c.github.io/navigation-timing/#extensions-to-the-performance-interface

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23330

---

- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___
@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2019

💔 Test failed - linux-rel-css

@tomasdivito
Copy link
Contributor Author

tomasdivito commented Oct 16, 2019

@jdm I'm taking a look at the comments you made here: #23773

when you talk about the panicked test, I believe you meant this:
./mach test-wpt tests/wpt/mozilla/tests/mozilla/window_performance.html
instead of
./mach test-wpt tests/wpt/mozilla/window_performance.html which reports "CRITICAL Unable to find any tests at the path(s)"

Having this in mind, we are currently facing the same issue? Am I right? If that's the case, the solution you proposed should be filled in another issue and worked apart from this one (I can take that, would be nice to work on) but not sure if that would be the steps to follow

@jdm
Copy link
Member

jdm commented Oct 17, 2019

Yes, this is the same issue that I described. Given the number of additional passing tests in https://build.servo.org/builders/linux-rel-wpt/builds/13812/steps/shell__4/logs/filtered-wpt-errorsummary.log I am happy to merge these changes and file that issue separately.

@jdm
Copy link
Member

jdm commented Oct 17, 2019

Filed #24468.

@jdm
Copy link
Member

jdm commented Oct 17, 2019

I recommend running the affected test directories and using these instructions to generate logs you can use to automatically update the expected test results.

@jdm
Copy link
Member

jdm commented Nov 7, 2019

@tomasdivito Are you planning to finish this PR?

@tomasdivito
Copy link
Contributor Author

tomasdivito commented Nov 7, 2019

@jdm Yes, or at least when #24468 is done.

If I don't make progress this weekend I'll report it and remove the assignation. If you think this / or the other patch should have more priority feel free to un-assign me if that's the case

@jdm jdm added this to In progress in PanoMoments Nov 20, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Dec 14, 2019

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

@CYBAI
Copy link
Collaborator

CYBAI commented Dec 14, 2019

Close in favor of #25205

@CYBAI CYBAI closed this Dec 14, 2019
@atouchet atouchet moved this from In progress to Done in PanoMoments Feb 23, 2020
@atouchet atouchet removed this from Done in PanoMoments Feb 23, 2020
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.