Move all methods on `Window` back to methods #847

alexcrichton · 2018-09-17T21:27:53Z

As brought up in #834 these are both fallible (in the worker context Window isn't defined) and you can also have multiple Window objects!

Closes #834

fitzgen · 2018-09-17T22:22:54Z

crates/web-sys/src/lib.rs

@@ -17,4 +17,15 @@ extern crate wasm_bindgen;
 extern crate js_sys;
 use js_sys::Object;

+#[cfg(feature = "Window")]
+pub fn window() -> Option<Window> {


I think we should automatically generate this function for every [Global] interface, so that one can get self in workers too.

Yeah I was originally wondering that as well, but I'm somewhat wary to do that because we get things like:

crates/web-sys/webidls/enabled/ServiceWorkerGlobalScope.webidl:[Global=(Worker,ServiceWorker), crates/web-sys/webidls/enabled/ServiceWorkerGlobalScope.webidl- Exposed=ServiceWorker] crates/web-sys/webidls/enabled/ServiceWorkerGlobalScope.webidl-interface ServiceWorkerGlobalScope : WorkerGlobalScope { -- crates/web-sys/webidls/enabled/AudioWorkletGlobalScope.webidl:[Global=(Worklet,AudioWorklet),Exposed=AudioWorklet] crates/web-sys/webidls/enabled/AudioWorkletGlobalScope.webidl-interface AudioWorkletGlobalScope : WorkletGlobalScope { crates/web-sys/webidls/enabled/AudioWorkletGlobalScope.webidl- void registerProcessor (DOMString name, VoidFunction processorCtor); -- crates/web-sys/webidls/enabled/PaintWorkletGlobalScope.webidl:[Global=(Worklet,PaintWorklet),Exposed=PaintWorklet] crates/web-sys/webidls/enabled/PaintWorkletGlobalScope.webidl-interface PaintWorkletGlobalScope : WorkletGlobalScope { crates/web-sys/webidls/enabled/PaintWorkletGlobalScope.webidl- void registerPaint(DOMString name, VoidFunction paintCtor); -- crates/web-sys/webidls/enabled/Window.webidl:[Global=Window, crates/web-sys/webidls/enabled/Window.webidl- Exposed=Window, crates/web-sys/webidls/enabled/Window.webidl- LegacyUnenumerableNamedProperties] -- crates/web-sys/webidls/enabled/DedicatedWorkerGlobalScope.webidl:[Global=(Worker,DedicatedWorker), crates/web-sys/webidls/enabled/DedicatedWorkerGlobalScope.webidl- Exposed=DedicatedWorker] crates/web-sys/webidls/enabled/DedicatedWorkerGlobalScope.webidl-interface DedicatedWorkerGlobalScope : WorkerGlobalScope { -- crates/web-sys/webidls/enabled/WorkerDebuggerGlobalScope.webidl:[Global=(WorkerDebugger), Exposed=WorkerDebugger] crates/web-sys/webidls/enabled/WorkerDebuggerGlobalScope.webidl-interface WorkerDebuggerGlobalScope : EventTarget { crates/web-sys/webidls/enabled/WorkerDebuggerGlobalScope.webidl- [Throws] -- crates/web-sys/webidls/enabled/SharedWorkerGlobalScope.webidl:[Global=(Worker,SharedWorker), crates/web-sys/webidls/enabled/SharedWorkerGlobalScope.webidl- Exposed=SharedWorker] crates/web-sys/webidls/enabled/SharedWorkerGlobalScope.webidl-interface SharedWorkerGlobalScope : WorkerGlobalScope {

It looks like there's a lot of semi-weird *GlobalScope objects perhaps?

One option we could do though is to perhaps define a self_ method which returns self, and then have other handwritten methods that cast this to the various scopes? I also figured that a top-level window accessor should at least be forwards compatible with an automatic solution!

Yeah, I suppose it is fine to special case and hand-code globals right now.

If you are writing an audio worklet, you will want to get the audio worklet global scope... but almost no one is doing that (relative to people using window)

WorkerDebuggerGlobalScope can probably be removed -- I think that is only for the debugger.

I think a fn self_() -> js_sys::Object function would be useful regardless. 👍

fitzgen · 2018-09-17T22:23:37Z

crates/web-sys/src/lib.rs

+
+    js_sys::Function::new_no_args("return this")
+        .call0(&JsValue::undefined())
+        .ok()?


This can be unwrapped because we know that the function we are constructing is not going to throw.

True! LLVM doesn't know this though and I figured it may be good to avoid all the panic code unwrap() brings in for something as small as window. (not that the current code is that small code-footprint-wise)

Ah, good point. Sounds good to me!

fitzgen · 2018-09-17T22:24:21Z

crates/web-sys/src/lib.rs

+pub fn window() -> Option<Window> {
+    use wasm_bindgen::{JsValue, JsCast};
+
+    js_sys::Function::new_no_args("return this")


Constructing a new function on every invocation is super heavy. Can we stuff this inside a Once and cache the results?

Yeah I was just kinda shooting in the dark here adding "simply anything that worked". Do you have an idea of how we could make this "better"? (apart from caching in a static, which would of course be better than this!)

I was thinking a static mut, yeah. Do you have concerns with that approach?

This is intended to address rustwasm#834 where we don't actually want methods scoped like this! Instead we'll provide one unique accessor for the `window` object itself.

All APIs on `Windows` are tagged where `Window` has `[Global]`, and they all need to be structurally accessed rather than through a prototype.

This is roughly defined by https://html.spec.whatwg.org/multipage/window-object.html#windowproxy and otherwise fits the bill how otherwise only `interface WindowProxy;` exists in the WebIDL.

Returns `Option<Window>` and can be used as a convenience to get a handle to the global `window` object.

alexcrichton · 2018-09-18T21:16:48Z

@fitzgen do you think we should go ahead and add fn self_? Or go with just window() for now?

fitzgen · 2018-09-18T21:57:45Z

I think it is fine to go with just window right now.

The more I think about it, the more I think that the generic give-me-the-global-object function (what we've been calling self_) should probably be in js_sys, since you might want to get the global object in non-Web environments as well.

alexcrichton · 2018-09-18T22:01:03Z

True yeah! I'm not sure what a good name for that would be though? fn global_scope() -> Object perhaps?

fitzgen · 2018-09-18T22:08:08Z

or just global?

alexcrichton · 2018-09-18T22:41:04Z

Ok, done!

fitzgen

Thanks! 👍

alexcrichton requested a review from fitzgen September 17, 2018 21:27

fitzgen reviewed Sep 17, 2018

View reviewed changes

alexcrichton added 7 commits September 17, 2018 17:36

Remove support for scoped static methods

c67582a

This is intended to address rustwasm#834 where we don't actually want methods scoped like this! Instead we'll provide one unique accessor for the `window` object itself.

Translate the Global attribute to "structural"

8cf9da4

All APIs on `Windows` are tagged where `Window` has `[Global]`, and they all need to be structurally accessed rather than through a prototype.

Translate the WindowProxy type to Window.

99e1b35

This is roughly defined by https://html.spec.whatwg.org/multipage/window-object.html#windowproxy and otherwise fits the bill how otherwise only `interface WindowProxy;` exists in the WebIDL.

Add a top-level web_sys::window function

f24828a

Returns `Option<Window>` and can be used as a convenience to get a handle to the global `window` object.

Fix webidl-tests fallout

fe31615

Tweak package.json for fetch example

4ea5d70

Fix botched merge

e66d01f

alexcrichton force-pushed the fix-window branch from 5088ff0 to 562406d Compare September 18, 2018 00:44

Fix web-sys Location test

bbc46f9

alexcrichton force-pushed the fix-window branch from 562406d to bbc46f9 Compare September 18, 2018 21:16

Squelch warnings in webidl tests

604ecd9

Add js_sys::global

a83d561

fitzgen approved these changes Sep 18, 2018

View reviewed changes

alexcrichton merged commit 5832ff3 into rustwasm:master Sep 18, 2018

alexcrichton deleted the fix-window branch September 18, 2018 23:59

chinedufn mentioned this pull request Sep 27, 2018

Detecting a global window object Node.js + jsdom #896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all methods on `Window` back to methods #847

Move all methods on `Window` back to methods #847

alexcrichton commented Sep 17, 2018

fitzgen Sep 17, 2018

alexcrichton Sep 18, 2018

fitzgen Sep 18, 2018

fitzgen Sep 17, 2018

alexcrichton Sep 18, 2018

fitzgen Sep 18, 2018

fitzgen Sep 17, 2018

alexcrichton Sep 18, 2018

fitzgen Sep 18, 2018

alexcrichton commented Sep 18, 2018

fitzgen commented Sep 18, 2018

alexcrichton commented Sep 18, 2018

fitzgen commented Sep 18, 2018

alexcrichton commented Sep 18, 2018

fitzgen left a comment

Move all methods on Window back to methods #847

Move all methods on Window back to methods #847

Conversation

alexcrichton commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 18, 2018

fitzgen commented Sep 18, 2018

alexcrichton commented Sep 18, 2018

fitzgen commented Sep 18, 2018

alexcrichton commented Sep 18, 2018

fitzgen left a comment

Choose a reason for hiding this comment

Move all methods on `Window` back to methods #847

Move all methods on `Window` back to methods #847