Skip to content

/ servo

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

Invoke failure steps if there's no metadata #13412

Merged
merged 1 commit into from Sep 27, 2016
Merged

Invoke failure steps if there's no metadata #13412

merged 1 commit into from Sep 27, 2016
+149 −35

Conversation

@gterzian
Copy link
Member

gterzian commented Sep 25, 2016 •
edited by larsbergstrom

Media element network response processing code should invoke failure steps if there's no metadata.

Fixes #13375.

This change is Reviewable
@highfive
Copy link

highfive commented Sep 25, 2016

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

highfive commented Sep 25, 2016

Heads up! This PR modifies the following files:

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

highfive commented Sep 25, 2016

warning Warning warning

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

KiChjang commented Sep 25, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2016

⌛ Trying commit 92a3bac with merge fe440f1...
bors-servo added a commit that referenced this pull request Sep 25, 2016
@bors-servo
Auto merge of #13412 - gterzian:failure_if_no_metadata, r=<try>
Loading status checks…
fe440f1 
Invoke failure steps if there's no metadata

<!-- Please describe your changes on the following line: -->
Media element network response processing code should invoke failure steps if there's no metadata #13375

WIP...
---
<!-- 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 _____

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

bors-servo commented Sep 25, 2016

💔 Test failed - linux-rel
@gterzian
Copy link
Member Author

gterzian commented Sep 25, 2016

@jdm When you said that some tests need updating, did you mean changing the currently failing one's (/html/semantics/embedded-content/media-elements/loading-the-media-resource/resource-selection-invoke-insert-into-iframe.html) expectation to FAIL?
@KiChjang
Copy link
Member

KiChjang commented Sep 25, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2016

⌛ Trying commit 9667603 with merge e895609...
bors-servo added a commit that referenced this pull request Sep 25, 2016
@bors-servo
Auto merge of #13412 - gterzian:failure_if_no_metadata, r=<try>
Loading status checks…
e895609 
Invoke failure steps if there's no metadata

<!-- Please describe your changes on the following line: -->
Media element network response processing code should invoke failure steps if there's no metadata #13375

WIP...
---
<!-- 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
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13375 (github issue number if applicable).

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

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

bors-servo commented Sep 25, 2016

💔 Test failed - linux-dev
@jdm
Copy link
Member

jdm commented Sep 25, 2016

@gterzian Yes, since the audio elements don't get any viable metadata it's expected that those tests will now fail.
@gterzian
Copy link
Member Author

gterzian commented Sep 25, 2016

@jdm Ok, thanks for clarifying and I'll dig around a bit!
@gterzian gterzian force-pushed the gterzian:failure_if_no_metadata branch from 3e81358 to 92a3bac Sep 25, 2016
@gterzian
Copy link
Member Author

gterzian commented Sep 25, 2016

@jdm Ok so I updated the .ini files using ./mach update-wpt...
@jdm
jdm reviewed Sep 25, 2016
View changes
tests/wpt/mozilla/meta/mozilla/webgl/tex_image_2d_canvas_no_context.html.ini Outdated
@@ -1,4 +0,0 @@
[tex_image_2d_canvas_no_context.html]

This comment has been minimized.

Sign in to view
@jdm

jdm Sep 25, 2016

Member

The change to this file should be reverted, but otherwise it looks good!

This comment has been minimized.

Sign in to view
@gterzian

gterzian Sep 25, 2016

Author Member

@jdm thanks for checking, it's done!
@jdm
Copy link
Member

jdm commented Sep 25, 2016

@gterzian If you squash the commits, this should be ready to merge :)
@gterzian gterzian force-pushed the gterzian:failure_if_no_metadata branch from 3c24d83 to e544b29 Sep 25, 2016
@gterzian
Copy link
Member Author

gterzian commented Sep 25, 2016 •
edited

@jdm squashed :)
@jdm
Copy link
Member

jdm commented Sep 25, 2016

@bors-servo: r+
@KiChjang
Copy link
Member

KiChjang commented Sep 27, 2016

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2016

📌 Commit afffba8 has been approved by jdm
@gterzian
Copy link
Member Author

gterzian commented Sep 27, 2016 •
edited

I can't tell from the logs what the failure is, will try to build locally...
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2016

⌛ Testing commit afffba8 with merge a642500...
bors-servo added a commit that referenced this pull request Sep 27, 2016
@bors-servo
Auto merge of #13412 - gterzian:failure_if_no_metadata, r=jdm
Loading status checks…
a642500 
Invoke failure steps if there's no metadata

Media element network response processing code should invoke failure steps if there's no metadata.

Fixes #13375.

<!-- 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/13412)
<!-- Reviewable:end -->
@gterzian
Copy link
Member Author

gterzian commented Sep 27, 2016

re the Travis failure, ./mach build --dev --verbose works locally, and I can't tell from the Travis logs what went wrong...
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2016

💔 Test failed - mac-rel-wpt
@gterzian
checking for metadata in htmlmediaelement::response_complete
Loading status checks…
3e33495 
updated test expectations

reverting changes to webgl test
@gterzian gterzian force-pushed the gterzian:failure_if_no_metadata branch from afffba8 to 3e33495 Sep 27, 2016
@gterzian
Copy link
Member Author

gterzian commented Sep 27, 2016

@jdm TIMEOUT [expected OK] /webgl/conformance-1.0.3/conformance/attribs/gl-matrix-attributes.html

seems like a weird failure... just did another rebase, hope it will fix it...
@jdm
Copy link
Member

jdm commented Sep 27, 2016

@gterzian No need to keep rebasing after any test failure. We have lots of known intermittent test failures that occur on our build machines.
@jdm
Copy link
Member

jdm commented Sep 27, 2016 •
edited

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2016

📌 Commit 3e33495 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2016

⌛ Testing commit 3e33495 with merge ec5ae85...
bors-servo added a commit that referenced this pull request Sep 27, 2016
@bors-servo
Auto merge of #13412 - gterzian:failure_if_no_metadata, r=jdm
Loading status checks…
ec5ae85 
Invoke failure steps if there's no metadata

Media element network response processing code should invoke failure steps if there's no metadata.

Fixes #13375.

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

bors-servo commented Sep 27, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit 3e33495 into servo:master Sep 27, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@gterzian
Copy link
Member Author

gterzian commented Sep 28, 2016

@jdm cheers, and thanks for your help 👍
paulrouget added a commit to servo/blog.servo.org that referenced this pull request Sep 29, 2016
@paulrouget
Adding gterzian to "New Contributors"
ce6c1bd 
See servo/servo#13412
@paulrouget paulrouget mentioned this pull request Sep 29, 2016
Closed
@gterzian gterzian deleted the gterzian:failure_if_no_metadata branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@gterzian @highfive @KiChjang @bors-servo @jdm @Ms2ger @emilio
You can’t perform that action at this time.