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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Feb 24, 2017
Merged

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

merged 1 commit into from
Feb 24, 2017

Conversation

nox
Copy link
Contributor

@nox nox commented Jan 20, 2017 •
edited by larsbergstrom
Loading

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/document.rs
  • @KiChjang: components/script/dom/document.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 20, 2017
jdm
jdm requested changes Jan 20, 2017
View reviewed changes
tests/wpt/web-platform-tests/dom/nodes/Document-createEvent.html Outdated

test(function() {
assert_equals(document.createEvent('storageevent'), "");
}, "document.createEvent('storageevent') does not propagate the document's URL.")
Copy link
Member

@jdm jdm Jan 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

(given the text at https://dom.spec.whatwg.org/#concept-event-create and https://dom.spec.whatwg.org/#dom-document-createevent)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@wafflespeanut wafflespeanut assigned jdm and unassigned wafflespeanut Jan 22, 2017
@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 26, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 19, 2017
jdm
jdm previously requested changes Feb 19, 2017
View reviewed changes
tests/wpt/mozilla/tests/mozilla/createEvent-storageevent.html Outdated
<script src="/resources/testharnessreport.js"></script>
<script>
test(function() {
assert_equals(document.createEvent('storageevent'), "");
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this test passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@jdm
Copy link
Member

jdm commented Feb 22, 2017

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

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 22, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 24, 2017
@nox nox dismissed jdm’s stale review February 24, 2017 11:22

This was addressed.

@nox
Copy link
Contributor Author

nox commented Feb 24, 2017

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

📌 Commit b2adcfb has been approved by jdm

@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. labels Feb 24, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit b2adcfb with merge 6665d59...

bors-servo pushed a commit that referenced this pull request Feb 24, 2017
Auto merge of #15131 - nox:storageevent, r=jdm
6665d59 
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 mentioned this pull request Feb 24, 2017
Merged
5 tasks
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: jdm
Pushing 6665d59 to master...

@bors-servo bors-servo merged commit b2adcfb into servo:master Feb 24, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm jdm left review comments

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nox @highfive @jdm @bors-servo @wafflespeanut