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

Test that StorageEvent's URL is empty from document.createEvent #15131

Merged
merged 1 commit into from Feb 24, 2017

Conversation

@nox
Copy link
Member

commented Jan 20, 2017

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jan 20, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/document.rs
  • @KiChjang: components/script/dom/document.rs
test(function() {
assert_equals(document.createEvent('storageevent'), "");
}, "document.createEvent('storageevent') does not propagate the document's URL.")

This comment has been minimized.

Copy link
@jdm

jdm Jan 20, 2017

Member

This is oddly specific for such a general test file. It would be more correct to test that for every event type that document.createEvent works, var event2 = new event.constructor(event.type) yields an event with matching properties (with the exception of timeStamp which is set every time an event is created).

This comment has been minimized.

This comment has been minimized.

Copy link
@nox

nox Feb 7, 2017

Author Member

What you are asking me is way more involved than a simple Servo fix though. Should I file that we don't test that on the WPT repos?

This comment has been minimized.

Copy link
@jdm

jdm Feb 7, 2017

Member

Yes. If you want to add a regression test for this specific fix, please do so in wpt/mozilla instead.

This comment has been minimized.

Copy link
@nox

nox Feb 19, 2017

Author Member

Done.

@wafflespeanut wafflespeanut assigned jdm and unassigned wafflespeanut Jan 22, 2017
@nox nox force-pushed the nox:storageevent branch from 954dce1 to 4a0c47e Feb 19, 2017
@nox nox force-pushed the nox:storageevent branch from 4a0c47e to 8805fb9 Feb 19, 2017
<script src="/resources/testharnessreport.js"></script>
<script>
test(function() {
assert_equals(document.createEvent('storageevent'), "");

This comment has been minimized.

Copy link
@jdm

jdm Feb 19, 2017

Member

I don't see how this test passes.

This comment has been minimized.

Copy link
@nox

nox Feb 19, 2017

Author Member

Oops, probably left uncommitted changes and cleared them... Will fix.

This comment has been minimized.

Copy link
@nox

nox Feb 19, 2017

Author Member

Fixed.

@nox nox force-pushed the nox:storageevent branch from 8805fb9 to 1751a36 Feb 19, 2017
@jdm

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

This needs to have ./mach update-manifest run again; after that r=me.

@nox nox force-pushed the nox:storageevent branch from 1751a36 to b2adcfb Feb 24, 2017
@nox nox dismissed jdm’s stale review Feb 24, 2017

This was addressed.

@nox

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

📌 Commit b2adcfb has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

⌛️ Testing commit b2adcfb with merge 6665d59...

bors-servo added a commit that referenced this pull request Feb 24, 2017
Test that StorageEvent's URL is empty from document.createEvent

<!-- 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/15131)
<!-- Reviewable:end -->
@bors-servo bors-servo referenced this pull request Feb 24, 2017
3 of 5 tasks complete
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

@bors-servo bors-servo merged commit b2adcfb into servo:master Feb 24, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.