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

Implement [Unscopable] (fixes #11583) #11620

Merged
merged 1 commit into from Jun 7, 2016
Merged

Implement [Unscopable] (fixes #11583) #11620

merged 1 commit into from Jun 7, 2016

Conversation

@nox
Copy link
Member

nox commented Jun 5, 2016

This change is Reviewable

@highfive
Copy link

highfive commented Jun 5, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/interface.rs, components/script/dom/webidls/ChildNode.webidl, components/script/dom/webidls/ParentNode.webidl
@highfive
Copy link

highfive commented Jun 5, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@nox
Copy link
Member Author

nox commented Jun 5, 2016

r? @Ms2ger

@highfive highfive assigned Ms2ger and unassigned SimonSapin Jun 5, 2016
@nox
Copy link
Member Author

nox commented Jun 5, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2016

Trying commit 64c1e00 with merge 36360b9...

bors-servo added a commit that referenced this pull request Jun 5, 2016
Implement [Unscopable] (fixes #11583)

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

bors-servo commented Jun 5, 2016

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 7, 2016

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

Previously, bors-servo wrote…

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows


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


components/script/dom/bindings/interface.rs, line 249 [r1] (raw file):

        let unscopable_symbol = GetWellKnownSymbol(cx, SymbolCode::unscopables);
        assert!(!unscopable_symbol.is_null());
        let unscopable_id = RootedId::new(cx, RUST_SYMBOL_TO_JSID(unscopable_symbol));

Please try to add a few empty lines in this block


components/script/dom/bindings/codegen/CodegenRust.py, line 5121 [r1] (raw file):

        haveUnscopables = (len(unscopableNames) != 0 and
                           descriptor.interface.hasInterfacePrototypeObject())

Please don't use hasInterfacePrototypeObject; make this if unscopableNames: under the if not descriptor.interface.isCallback(): block.


components/script/dom/webidls/ChildNode.webidl, line 11 [r1] (raw file):

[NoInterfaceObject]
interface ChildNode {
  [Throws, Unscopable]

I'd like all of those to be tested. (idlharness?)


Comments from Reviewable

@nox nox force-pushed the nox:unscopable branch from 64c1e00 to 2419cb9 Jun 7, 2016
@highfive
Copy link

highfive commented Jun 7, 2016

New code was committed to pull request.

@nox
Copy link
Member Author

nox commented Jun 7, 2016

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


components/script/dom/bindings/interface.rs, line 249 [r1] (raw file):

Previously, Ms2ger wrote…

Please try to add a few empty lines in this block

Done.

components/script/dom/bindings/codegen/CodegenRust.py, line 5121 [r1] (raw file):

Previously, Ms2ger wrote…

Please don't use hasInterfacePrototypeObject; make this if unscopableNames: under the if not descriptor.interface.isCallback(): block.

Done.

components/script/dom/webidls/ChildNode.webidl, line 11 [r1] (raw file):

Previously, Ms2ger wrote…

I'd like all of those to be tested. (idlharness?)

Done.

Comments from Reviewable

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 7, 2016

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


tests/wpt/web-platform-tests/dom/nodes/remove-unscopable.html, line 18 [r2] (raw file):

    "append"
];
for (i in unscopables) {

var i


Comments from Reviewable

@nox nox force-pushed the nox:unscopable branch from 2419cb9 to 3529803 Jun 7, 2016
@highfive
Copy link

highfive commented Jun 7, 2016

New code was committed to pull request.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 7, 2016

@bors-servo r+

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, all discussions resolved.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2016

📌 Commit 3529803 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2016

Testing commit 3529803 with merge f56848a...

bors-servo added a commit that referenced this pull request Jun 7, 2016
Implement [Unscopable] (fixes #11583)

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

bors-servo commented Jun 7, 2016

@bors-servo bors-servo merged commit 3529803 into servo:master Jun 7, 2016
2 checks passed
2 checks passed
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.

None yet

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