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

11402 implement window alert #11432

Merged
merged 1 commit into from May 26, 2016
Merged

Conversation

@ab22
Copy link
Contributor

ab22 commented May 26, 2016

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 #11402

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.


This change is Reviewable

@highfive
Copy link

highfive commented May 26, 2016

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

@highfive
Copy link

highfive commented May 26, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/window.rs, components/script/dom/webidls/Window.webidl
@KiChjang
Copy link
Member

KiChjang commented May 26, 2016

Please squash your commits into one. r=me when post squash.

@KiChjang KiChjang assigned KiChjang and unassigned metajack May 26, 2016
@ab22 ab22 force-pushed the ab22:11402-implement-window-alert branch from db01f2b to f14303d May 26, 2016
@highfive
Copy link

highfive commented May 26, 2016

New code was committed to pull request.

@KiChjang
Copy link
Member

KiChjang commented May 26, 2016

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2016

📌 Commit f14303d has been approved by KiChjang

@ab22
Copy link
Contributor Author

ab22 commented May 26, 2016

@KiChjang Thanks for approving .. was waiting for the checks to pass before @ you :P

@KiChjang
Copy link
Member

KiChjang commented May 26, 2016

@ab22 The "r=me" comment is meant for other reviewers. @bors-servo is very specific about who he listens to.

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2016

Testing commit f14303d with merge c8bf62e...

bors-servo added a commit that referenced this pull request May 26, 2016
11402 implement window alert

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11402

Either:
- [X] 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11432)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2016

💔 Test failed - mac-rel-wpt

@highfive
Copy link

highfive commented May 26, 2016

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  │ FAIL [expected PASS] Window interface: operation alert()
  │   → assert_own_property: global object missing non-static operation expected property &#34;alert&#34; missing
  │ 
  │ IdlInterface.prototype.test_member_operation/&lt;@http://web-platform.test:8000/resources/idlharness.js:1179:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ IdlInterface.prototype.test_member_operation@http://web-platform.test:8000/resources/idlharness.js:1137:5
  │ IdlInterface.prototype.test_members@http://web-platform.test:8000/resources/idlharness.js:1361:21
  │ IdlInterface.prototype.test@http://web-platform.test:8000/resources/idlharness.js:693:5
  │ IdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:358:9
  └ window.onload@http://web-platform.test:8000/html/dom/interfaces.html:180:3
@cbrewster
Copy link
Member

cbrewster commented May 26, 2016

Looks like the same issue as #11407 (comment) I guess there is an issue with the codegen.

@@ -5025,9 +5025,6 @@
[Window interface: attribute applicationCache]
expected: FAIL

[Window interface: operation alert()]
expected: FAIL

This comment has been minimized.

Copy link
@KiChjang

KiChjang May 26, 2016

Member

This change here needs to be reverted.

@KiChjang
Copy link
Member

KiChjang commented May 26, 2016

You'll need to create some new tests under tests/wpt/web-platform-tests/html/browsers/the-window-object/browser-interface-elements/alert.html.

@jdm
Copy link
Member

jdm commented May 26, 2016

What sort of test do you have in mind? Alert seems challenging to me, since it's supposed to have a modal UI.

@cbrewster
Copy link
Member

cbrewster commented May 26, 2016

@jdm @KiChjang Maybe add to tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsershowmodalprompt_event.html? Just add a new iframe that calls alert with no parameters, make sure we get the mozbrowser event and the message is an empty string

@jdm
Copy link
Member

jdm commented May 26, 2016

Good idea!

@ab22 ab22 force-pushed the ab22:11402-implement-window-alert branch from f14303d to d72dbb6 May 26, 2016
@highfive
Copy link

highfive commented May 26, 2016

New code was committed to pull request.

@ab22
Copy link
Contributor Author

ab22 commented May 26, 2016

Pushed the new test and reverted the previous mentioned change.

Again: ./mach build -d && ./mach test-tidy does not report any errors and ./mach test-wpt tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsershowmodalprompt_event.html shows all tests as PASS.

@KiChjang
Copy link
Member

KiChjang commented May 26, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2016

📌 Commit d72dbb6 has been approved by KiChjang

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2016

Testing commit d72dbb6 with merge 6d0c420...

bors-servo added a commit that referenced this pull request May 26, 2016
11402 implement window alert

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11402

Either:
- [X] 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11432)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2016

@bors-servo bors-servo merged commit d72dbb6 into servo:master May 26, 2016
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
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.