Skip to content

Conversation

jdm
Copy link
Member

@jdm jdm commented Mar 18, 2016

This was easier to throw together than per-attribute/method support, and it gets rid of some nonstandard properties from our globals.

Fixes #7626.

r? @Ms2ger


This change is Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 18, 2016
@jdm
Copy link
Member Author

jdm commented Mar 18, 2016

I discovered that in the case of:

[Pref="foo"]
interface Parent {}

interface Child : Parent {}

Parent still gets defined in the current implementation even if foo is false. What should we do in this case?

@nox
Copy link
Contributor

nox commented Mar 19, 2016

Do you mean that Child still gets defined? Anyway whether it is parent or child I don't think either of them should be visible.

@jdm
Copy link
Member Author

jdm commented Mar 19, 2016

No, I mean that window.Parent is still defined even if the preference controlling it's visibility is false.

@nox
Copy link
Contributor

nox commented Mar 20, 2016

Well that's obviously wrong, no?

@paulrouget paulrouget mentioned this pull request Mar 21, 2016
@larsbergstrom
Copy link
Contributor

👍 to `[Pref="dom.bluetooth.enabled"], and off by default.

cc @dati91

@jdm
Copy link
Member Author

jdm commented Mar 23, 2016

This is ready for review.

@jdm
Copy link
Member Author

jdm commented Mar 24, 2016

This is still ready for review; I just got carried away and implemented the Pref annotation for members of interfaces as well.

@jdm jdm changed the title Support controlling interface visibility via preferences Support controlling interface and member visibility via preferences Mar 24, 2016
@jdm jdm force-pushed the interfacepref branch 2 times, most recently from 83e93ac to e06f60d Compare March 24, 2016 14:03
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #10204) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Mar 26, 2016
@paulrouget
Copy link
Contributor

Can we get a review on that?

@jdm
Copy link
Member Author

jdm commented Apr 27, 2016

This has been ignored for more than a month. I would really appreciate if someone would take responsibility for reviewing my work :(

@nox
Copy link
Contributor

nox commented Apr 29, 2016

On it.

Could you give more details about what you meant by "This was easier to throw together than per-attribute/method support"?

@nox
Copy link
Contributor

nox commented Apr 29, 2016

@jdm I think you are cursed.

capture d ecran 2016-04-29 a 11 05 07

@pkaminski
Copy link

Hey @jdm, how did you make that happen? Reviewable thinks that somehow there's a discussion attached to a file that doesn't exist in the review, hence boom.


Comments from Reviewable

@pkaminski
Copy link

I mean @nox, sorry.


Comments from Reviewable

@nox
Copy link
Contributor

nox commented Apr 29, 2016

@pkaminski I just clicked on the Reviewable button and it failed to load.

@jdm I don't get the purpose of avoiding JS_DefineProperties and friends, they just loop over items and call JS_DefineProperty sequentially. Couldn't we just rewrite JS_DefineProperties ourselves, putting a new field in JSPropertySpec or a similar structure?

else:
properties[arrayName] = "&[]"
properties[arrayName] = array.variableName()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this. It worked before and you didn't even document EMPTY_CONSTANTS.

@highfive
Copy link

highfive commented May 2, 2016

  ▶ Unexpected subtest result in /_mozilla/mozilla/preferences.html:
  │ FAIL [expected PASS] prefs
  │   → TestBinding is not defined
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/preferences.html:2:6
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/preferences.html:1:1

@jdm jdm force-pushed the interfacepref branch from 48b2b50 to 2a9bf70 Compare May 2, 2016 18:33
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels May 2, 2016
@jdm
Copy link
Member Author

jdm commented May 2, 2016

@bors-servo: r=nox

@bors-servo
Copy link
Contributor

📌 Commit 2a9bf70 has been approved by nox

@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 May 2, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 2a9bf70 with merge 8255e74...

bors-servo pushed a commit that referenced this pull request May 2, 2016
Support controlling interface and member visibility via preferences

This was easier to throw together than per-attribute/method support, and it gets rid of some nonstandard properties from our globals.

Fixes #7626.

r? @Ms2ger

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

💔 Test failed - linux-rel

@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 2, 2016
@highfive
Copy link

highfive commented May 2, 2016

  ▶ FAIL [expected PASS] /_mozilla/css/iframe/mozbrowser_navigation.html
  └   → /_mozilla/css/iframe/mozbrowser_navigation.html e60785ff75153e5d215bd0f65141fbb13cfbc61b
/_mozilla/css/iframe/mozbrowser_navigation_ref.html 8250706e506ad24231cc23be315b9b868387c598
Testing e60785ff75153e5d215bd0f65141fbb13cfbc61b == 8250706e506ad24231cc23be315b9b868387c598

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label May 2, 2016
@cbrewster
Copy link
Contributor

cbrewster commented May 2, 2016

@bors-servo
Copy link
Contributor

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit 2a9bf70 into servo:master May 2, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-tests-failed The changes caused existing tests to fail.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants