Closed Bug 1317422 (globalThis) Opened 5 years ago Closed 3 years ago
7.45 KB, patch
|Details | Diff | Splinter Review|
5.96 KB, patch
|Details | Diff | Splinter Review|
47 bytes, text/x-phabricator-request
|Details | Review|
"global"  is currently a stage 3 proposal [2, 3]. test262 already tests for the new "global" property  and we're currently failing some tests because of this.  https://github.com/tc39/proposal-global  https://github.com/tc39/proposals  https://github.com/tc39/ecma262/pull/702  https://github.com/tc39/test262/pull/765
#jslang suckered me into it.
Attachment #8815143 - Flags: review?(andrebargull)
Comment on attachment 8815143 [details] [diff] [review] Patch Review of attachment 8815143 [details] [diff] [review]: ----------------------------------------------------------------- Let's find out if this breaks any websites! :-D ::: js/src/tests/ecma_2017/Global/global-property-enumeration.js @@ +28,5 @@ > +assertEq(desc.configurable, true); > +assertEq(desc.writable, true); > +assertEq(desc.value, this); > + > +global = 42; The other test case already tests that `global = 42` works, should this one instead test with `this.global = 42`?
Attachment #8815143 - Flags: review?(andrebargull) → review+
Patch works fine for shell, doesn't work for browser. Turns out I need a ToWindowProxyIfWindow in there. But even that's not enough, because in the browser embedding, nsGlobalWindow::SetNewDocument invokes Object class init (in the process of setting up the window's prototype chain) before it creates the WindowProxy wrapper and calls js::SetWindowProxy to store the wrapper in the WINDOW_PROXY slot in the Window. The right fix to me seems to be to refactor things so the slot's filled early enough that Object class init doesn't have these oddities to deal with. But SetNewDocument looks an awful lot like Mos Eisley -- a wretched hive of scum and villainy -- and doesn't look particularly refactorable, nor does it seem like a good use of time to do so. (I want to refactor global object creation at *some* point, so that embeddings can basically declaratively specify what they want their global object to look like, including its prototype chain. So I'll probably get back to this eventually. Just, not now.) With the Object init approach out, I stole the approach evilpie mentioned on IRC (aside: it's truly awful that there are multiple ways to define a new one-off global property), fixed the one moderately obvious bug in it, and am tryservering it now. Will post for a fresh review if it comes back okay. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7cf02615566f272bd0ad7a89c865262a2a2ce62
Pretty sure jandem dealt with the ToWindow* gunk at one point, so let's throw this at him this time around.
Attachment #8816662 - Flags: review?(jdemooij)
Comment on attachment 8816662 [details] [diff] [review] Patch, v2 Review of attachment 8816662 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8816662 - Flags: review?(jdemooij) → review+
Reminder to land this.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d37fc752b0af Implement a global 'global' property whose value is the global object. r=jandem
Yay! When will this land in nightly and stable?
(In reply to ljharb from comment #10) > Yay! When will this land in nightly and stable? It will be in today's Nightly. Firefox 53 will be released April 18th, according to https://wiki.mozilla.org/RapidRelease/Calendar
Let's back this out. This is not web-compatible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8823427 [details] [diff] [review] Backout Review of attachment 8823427 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +1076,5 @@ > // Object.prototype yet. Force it now. > + if (!global->getOrCreateObjectPrototype(cx)) > + return false; > + > + return true; Mind also landing a separate patch, rs=me, that undoes this undoing, and keeps |global| as a Handle?
Attachment #8823427 - Flags: review?(jwalden+bmo) → review+
Is there a way we could still include `global` behind a flag in nightly, so that continued testing can be done as sites fix their issues?
Sure, we could add something to CompartmentOptions to frob it on, easily enough. Invoking the frob other than as an all-or-nothing approach, tho, seems difficult. I'm not sure exactly how useful such a thing is. We've used such frobs to feature-guard stuff in progress, like some Intl functionality, but that was only with the aim of having an opt-in in shell tests to enable it. And obviously, we can't expose such a thing to web content.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5f92e32f1d Backout the global property propsal. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/89388a238c25 Keep nicer code in JS_ResolveStandardClass. rs=Waldo
How about enabling this just for 'strict' contexts, wouldn't it be more compatible ir current web content?
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
After implementing it in Safari Tech Preview 21 > Added support for the global property on the global object (r210052) https://webkit.org/blog/7265/release-notes-for-safari-technology-preview-21/ It has been reverted for Web Compatibility reasons See https://bugs.webkit.org/show_bug.cgi?id=166915 Ryosuke Niwa said > We had to rollout this feature it broke Polymer tests. https://bugs.webkit.org/show_bug.cgi?id=165171#c22
QA Contact: jorendorff
From Alexandre Folle de Menezes in bug 1496748 > The 'global' proposal was renamed to 'globalThis'. I believe this was > already implemented with the old name, but disabled due to compatibility > issues. > > Chrome 70 already has the renamed implementation.
Should be easy to implement this. I would volunteer to do it.
Oh the original patch was actually not quite correct: We would resolve "global" again after it was deleted. Thanks test262 for catching this!
Summary: Implement "global" proposal → Implement "globalThis" proposal
You need to log in before you can comment on or make changes to this bug.