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

WIP: Fixes #11407: Implement Window.status #11434

Merged
merged 1 commit into from May 27, 2016
Merged

Conversation

@catchmrbharath
Copy link
Contributor

catchmrbharath 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 #11407 (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.


This change is Reviewable

@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
@catchmrbharath
Copy link
Contributor Author

catchmrbharath commented May 26, 2016

@Ms2ger mentioned in IRC that the tests won't work because of problems in codegen. I am reverting the test change back to FAIL.

I am not sure how to test these changes though. Any ideas ? @jdm

@nox
Copy link
Member

nox commented May 26, 2016

-S-awaiting-review +S-needs-code-changes +C-needs-test

Previously, catchmrbharath wrote…

@Ms2ger mentioned in IRC that the tests won't work because of problems in codegen. I am reverting the test change back to FAIL.

I am not sure how to test these changes though. Any ideas ? @jdm


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/script/dom/window.rs, line 835 [r1] (raw file):

    fn Status(&self) -> DOMString {
        self.status.borrow_mut().clone()

Just .borrow() is enough here.


Comments from Reviewable

@cbrewster
Copy link
Member

cbrewster commented May 26, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions.


components/script/dom/window.rs, line 834 [r1] (raw file):

    }

    fn Status(&self) -> DOMString {

A comment should be added above this with a link to the spec: https://html.spec.whatwg.org/multipage/#dom-window-status


components/script/dom/window.rs, line 838 [r1] (raw file):

    }

    fn SetStatus(&self, status: DOMString) {

Same thing as above (same spec link aswell)


Comments from Reviewable

@catchmrbharath catchmrbharath force-pushed the catchmrbharath:status branch from 80e3e11 to 001328f May 27, 2016
@highfive
Copy link

highfive commented May 27, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented May 27, 2016

-C-needs-test -S-awaiting-review +C-has-test +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


tests/wpt/web-platform-tests/html/browsers/the-window-object/browser-interface-elements/status.html, line 6 [r2] (raw file):

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<link rel="help" href="https://html.spec.whatwg.org/multipage/browsers.html#dom-window-status">

Nit: remove browsers.html from the link.


Comments from Reviewable

@catchmrbharath catchmrbharath force-pushed the catchmrbharath:status branch from 001328f to 4c0c1d0 May 27, 2016
@highfive
Copy link

highfive commented May 27, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented May 27, 2016

-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@catchmrbharath
Copy link
Contributor Author

catchmrbharath commented May 27, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions.


tests/wpt/web-platform-tests/html/browsers/the-window-object/browser-interface-elements/status.html, line 6 [r2] (raw file):

Previously, nox (Anthony Ramine) wrote…

Nit: remove browsers.html from the link.

You mean remove the whole link?

Comments from Reviewable

@catchmrbharath catchmrbharath force-pushed the catchmrbharath:status branch from 4c0c1d0 to f870c84 May 27, 2016
@highfive
Copy link

highfive commented May 27, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented May 27, 2016

-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


tests/wpt/web-platform-tests/html/browsers/the-window-object/browser-interface-elements/status.html, line 6 [r2] (raw file):

Previously, catchmrbharath (Bharath Mankalale) wrote…

You mean remove the whole link?

No, remove `browsers.html`, so that the link is the same as in the Rust code comment. The links with the filenames can change, while the others aren't supposed to.

Comments from Reviewable

@catchmrbharath catchmrbharath force-pushed the catchmrbharath:status branch from f870c84 to a4d9a1d May 27, 2016
@highfive
Copy link

highfive commented May 27, 2016

New code was committed to pull request.

@nox
Copy link
Member

nox commented May 27, 2016

@bors-servo r+

Thanks for your contribution!

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2016

📌 Commit a4d9a1d has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2016

Testing commit a4d9a1d with merge d890453...

bors-servo added a commit that referenced this pull request May 27, 2016
WIP: Fixes #11407: Implement Window.status

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
- [ ] These changes fix #11407 (github issue number if applicable).

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

bors-servo commented May 27, 2016

@bors-servo bors-servo merged commit a4d9a1d into servo:master May 27, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
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.

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