Allow setting preferences to false in WPT tests#10204
Allow setting preferences to false in WPT tests#10204bors-servo merged 1 commit intoservo:masterfrom awalgarg:fix10161
Conversation
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. |
|
Heads up! This PR modifies the following files:
|
highfive
added
the
S-awaiting-review
components/util/opts.rs
Outdated
| // on the resource path. | ||
| for pref in opt_match.opt_strs("pref").iter() { | ||
| prefs::set_pref(pref, PrefValue::Boolean(true)); | ||
| let split: Vec<&str> = pref.split('=').collect(); |
There was a problem hiding this comment.
this should use splitn I guess? since the value itself might have an equal sign in it
Also, it should do it in a way that doesn't crash if the pref isn't formatted correctly
There was a problem hiding this comment.
@Manishearth fixed, thanks!
Also, it should do it in a way that doesn't crash if the pref isn't formatted correctly
Umm, example of a pref which will crash this presently?
There was a problem hiding this comment.
Oh, never mind.
But this still doesn't use splitn.
There was a problem hiding this comment.
@Manishearth well, that's the old diff to which you added the comment :) Here is the updated one: https://github.com/servo/servo/pull/10204/files#diff-0543c2dfa73f1f316c2846ebb815deb5R771
|
Could you squash the two commits? |
|
@Manishearth was planning to wait for further feedback, but sure, done :) Thanks! |
|
@bors-servo r+ |
|
📌 Commit b1ff30f has been approved by |
highfive
added
S-awaiting-merge
Allow setting preferences to false in WPT tests First patch to servo - apologies if I did something stupid :) This is a fix for #10161. I have squashed the commits into one. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10204) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor |
|
FYI: the changes to wpt/harness need to be made upstream in w3c/wptrunner since we do not have bidirectional sync with at repository. |
|
It sort of feels like this should use |
7 participants
First patch to servo - apologies if I did something stupid :)
This is a fix for #10161. I have squashed the commits into one.
This change is