Servo: Expose interfaces only where they should be.

[Ms2ger]

In #12353, I added Exposed=(Window,Worker) to all interfaces, to preserve our previous behaviour. We need to go through all the interfaces, check in the specification what the correct value is, and update our implementation.

• Code: components/script/dom/webidls/.
• Test: tests/wpt/mozilla/tests/mozilla/interfaces.html, tests/wpt/mozilla/tests/mozilla/interfaces.worker.js and probably some others.

It would be quite difficult to review a PR that changed all interfaces at once, so please avoid doing that.

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[johannhof]

Since I started with Worker I might as well go through the whole list. So I'm taking this.

[Ms2ger]

@johannhof if you haven't started on the rest, perhaps it would be better to let others help out wit this, and move on to other bugs where you might learn more :)

[johannhof]

Good point! Other people who want to solve this, you can find reference webidls for Firefox here: http://searchfox.org/mozilla-central/source/dom/webidl

Untaking this!

[ice9js]

I would like to take this, but I need to make sure I know what's to be done. From what I understand, I need to go through all webidls in components/scripts/dom/webidls, and expose Window or Worker only where it appears in the specification ?

Also, is there any particular way you would like me to split this up so it doesn't go into one PR ?

[jdm]

I would choose an arbitrary number of interfaces that isn't too large like 10. And yes, that is correct.

[woshilapin]

Hi there, I'd like to help also on this one. I'm still trying to understand exactly what to do but, @ice9js, could you tell which webidl you're going to modify so we don't overlap? (if it's fine we're more than one on this issue at the same time, of course!)

[ice9js]

@woshilapin Sure. We can just split it in half alphabetically. I will do everything from Attr.webidl to HTMLOutputElement.webidl. Everything from HTMLParagraphElement.webidl on, I'll leave up to you.

[woshilapin]

Nice, thanks :-)

[woshilapin]

Builds are failing, don't loose time on reviewing them, I'm gonna investigate.

[woshilapin]

Should we keep track of which webidl file has been already reviewed? I'm proposing a list in the following comment (it may be better to have it in the first comment?).

[woshilapin]

• [x] Attr.webidl
• [x] BeforeUnloadEvent.webidl
• [x] Blob.webidl
• [x] BluetoothAdvertisingData.webidl
• [x] BluetoothCharacteristicProperties.webidl
• [x] BluetoothDevice.webidl
• [x] BluetoothRemoteGATTCharacteristic.webidl
• [x] BluetoothRemoteGATTDescriptor.webidl
• [x] BluetoothRemoteGATTServer.webidl
• [x] BluetoothRemoteGATTService.webidl
• [x] BluetoothUUID.webidl
• [x] Bluetooth.webidl
• [x] BrowserElement.webidl
• [x] CanvasGradient.webidl
• [x] CanvasPattern.webidl
• [x] CanvasRenderingContext2D.webidl
• [x] CharacterData.webidl
• [x] ChildNode.webidl
• [x] Client.webidl
• [x] CloseEvent.webidl
• [x] Comment.webidl
• [x] Console.webidl
• [x] Crypto.webidl
• [x] CSSStyleDeclaration.webidl
• [x] CSS.webidl
• [x] CustomEvent.webidl
• [x] DedicatedWorkerGlobalScope.webidl
• [x] DocumentFragment.webidl
• [x] DocumentType.webidl
• [x] Document.webidl
• [x] DOMException.webidl
• [x] DOMImplementation.webidl
• [x] DOMParser.webidl
• [x] DOMPointReadOnly.webidl
• [x] DOMPoint.webidl
• [x] DOMQuad.webidl
• [x] DOMRectList.webidl
• [x] DOMRectReadOnly.webidl
• [x] DOMRect.webidl
• [x] DOMStringMap.webidl
• [x] DOMTokenList.webidl
• [x] ElementContentEditable.webidl
• [x] ElementCSSInlineStyle.webidl
• [x] Element.webidl
• [x] ErrorEvent.webidl
• [ ] EventHandler.webidl
• [x] EventListener.webidl
• [x] EventModifierInit.webidl
• [x] EventSource.webidl
• [x] EventTarget.webidl
• [x] Event.webidl
• [x] FileList.webidl
• [x] FileReader.webidl
• [x] File.webidl
• [x] FocusEvent.webidl
• [x] ForceTouchEvent.webidl
• [x] FormData.webidl
• [x] Function.webidl
• [x] HashChangeEvent.webidl
• [x] HTMLAnchorElement.webidl
• [x] HTMLAppletElement.webidl
• [x] HTMLAreaElement.webidl
• [x] HTMLAudioElement.webidl
• [x] HTMLBaseElement.webidl
• [x] HTMLBodyElement.webidl
• [x] HTMLBRElement.webidl
• [x] HTMLButtonElement.webidl
• [x] HTMLCanvasElement.webidl
• [x] HTMLCollection.webidl
• [x] HTMLDataElement.webidl
• [x] HTMLDataListElement.webidl
• [x] HTMLDetailsElement.webidl
• [x] HTMLDialogElement.webidl
• [x] HTMLDirectoryElement.webidl
• [x] HTMLDivElement.webidl
• [x] HTMLDListElement.webidl
• [x] HTMLElement.webidl
• [x] HTMLEmbedElement.webidl
• [x] HTMLFieldSetElement.webidl
• [x] HTMLFontElement.webidl
• [x] HTMLFormControlsCollection.webidl
• [x] HTMLFormElement.webidl
• [x] HTMLFrameElement.webidl
• [x] HTMLFrameSetElement.webidl
• [x] HTMLHeadElement.webidl
• [x] HTMLHeadingElement.webidl
• [x] HTMLHRElement.webidl
• [x] HTMLHtmlElement.webidl
• [x] HTMLHyperlinkElementUtils.webidl
• [x] HTMLIFrameElement.webidl
• [x] HTMLImageElement.webidl
• [x] HTMLInputElement.webidl
• [x] HTMLLabelElement.webidl
• [x] HTMLLegendElement.webidl
• [x] HTMLLIElement.webidl
• [x] HTMLLinkElement.webidl
• [x] HTMLMapElement.webidl
• [x] HTMLMediaElement.webidl
• [x] HTMLMetaElement.webidl
• [x] HTMLMeterElement.webidl
• [x] HTMLModElement.webidl
• [x] HTMLObjectElement.webidl
• [x] HTMLOListElement.webidl
• [x] HTMLOptGroupElement.webidl
• [x] HTMLOptionElement.webidl
• [x] HTMLOutputElement.webidl
• [x] HTMLParagraphElement.webidl
• [x] HTMLParamElement.webidl
• [x] HTMLPreElement.webidl
• [x] HTMLProgressElement.webidl
• [x] HTMLQuoteElement.webidl
• [x] HTMLScriptElement.webidl
• [x] HTMLSelectElement.webidl
• [x] HTMLSourceElement.webidl
• [x] HTMLSpanElement.webidl
• [x] HTMLStyleElement.webidl
• [x] HTMLTableCaptionElement.webidl
• [x] HTMLTableCellElement.webidl
• [x] HTMLTableColElement.webidl
• [x] HTMLTableDataCellElement.webidl
• [x] HTMLTableElement.webidl
• [x] HTMLTableHeaderCellElement.webidl
• [x] HTMLTableRowElement.webidl
• [x] HTMLTableSectionElement.webidl
• [x] HTMLTemplateElement.webidl
• [x] HTMLTextAreaElement.webidl
• [x] HTMLTimeElement.webidl
• [x] HTMLTitleElement.webidl
• [x] HTMLTrackElement.webidl
• [x] HTMLUListElement.webidl
• [x] HTMLUnknownElement.webidl
• [x] HTMLVideoElement.webidl
• [x] ImageData.webidl
• [x] KeyboardEvent.webidl
• [x] Location.webidl
• [x] MediaError.webidl
• [x] MessageEvent.webidl
• [x] MimeTypeArray.webidl
• [x] MimeType.webidl
• [x] MouseEvent.webidl
• [x] NamedNodeMap.webidl
• [x] Navigator.webidl
• [x] NodeFilter.webidl
• [x] NodeIterator.webidl
• [x] NodeList.webidl
• [x] Node.webidl
• [x] NonElementParentNode.webidl
• [x] PageTransitionEvent.webidl
• [x] ParentNode.webidl
• [ ] PerformanceTiming.webidl
• [ ] Performance.webidl
• [x] PluginArray.webidl
• [x] Plugin.webidl
• [x] PopStateEvent.webidl
• [x] ProcessingInstruction.webidl
• [x] ProgressEvent.webidl
• [x] RadioNodeList.webidl
• [x] Range.webidl
• [x] Screen.webidl
• [x] ServiceWorkerContainer.webidl
• [x] ServiceWorkerGlobalScope.webidl
• [x] ServiceWorkerRegistration.webidl
• [x] ServiceWorker.webidl
• [x] ServoParser.webidl
• [ ] StorageEvent.webidl
• [ ] Storage.webidl
• [x] StyleSheetList.webidl
• [x] StyleSheet.webidl
• [x] TestBindingProxy.webidl
• [x] TestBinding.webidl
• [x] TextDecoder.webidl
• [x] TextEncoder.webidl
• [x] Text.webidl
• [x] TouchEvent.webidl
• [x] TouchList.webidl
• [x] Touch.webidl
• [x] TreeWalker.webidl
• [x] UIEvent.webidl
• [x] URLSearchParams.webidl
• [x] URL.webidl
• [x] ValidityState.webidl
• [x] WebGLActiveInfo.webidl
• [x] WebGLBuffer.webidl
• [x] WebGLContextEvent.webidl
• [x] WebGLFramebuffer.webidl
• [x] WebGLObject.webidl
• [x] WebGLProgram.webidl
• [x] WebGLRenderbuffer.webidl
• [x] WebGLRenderingContext.webidl
• [x] WebGLShaderPrecisionFormat.webidl
• [x] WebGLShader.webidl
• [x] WebGLTexture.webidl
• [x] WebGLUniformLocation.webidl
• [x] WebSocket.webidl
• [x] Window.webidl
• [x] WorkerGlobalScope.webidl
• [x] WorkerLocation.webidl
• [x] WorkerNavigator.webidl
• [x] Worker.webidl
• [x] XMLDocument.webidl
• [x] XMLHttpRequestEventTarget.webidl
• [x] XMLHttpRequestUpload.webidl
• [x] XMLHttpRequest.webidl

[scotttrinh]

Is the checklist ☝🏼 current? Is there a process with which to claim a part of it and submit PR's?

[jdm]

I think you should be able to claim the ones that start with WebGL.

[scotttrinh]

Thanks, @jdm! Should I check them off to claim them, or when they're done?

[jdm]

Let's check them to claim them.

[scotttrinh]

Can't seem to check them (they're greyed out), but I won't let that hold me up. Consider this notice that I intend to take the WebGL .webidls

[mskrzypkows]

Fixed interfaces:
HTMLHeadingElement.webidl
HTMLHRElement.webidl
HTMLHtmlElement.webidl
HTMLHyperlinkElementUtils.webidl
HTMLIFrameElement.webidl
HTMLImageElement.webidl
HTMLInputElement.webidl
HTMLLabelElement.webidl
HTMLLegendElement.webidl
HTMLLIElement.webidl

[jdm]

I unmarked the WebGL interfaces since that work was never performed.

[scotttrinh]

Sorry if that was on me! I went to go do them, and I saw they were all checked! I'll be happy to hop back on that since it wasn't finished.

[jdm]

@scotttrinh Ah, I guess we crossed wires. I checked them to ensure that nobody else tried to do the work at the same time :P Please do!

[scotttrinh]

Looks like everything in WebGL* was is exposed correctly according to the equivalent Firefox webidl. I couldn't find a reference IDL for WebGLObject in Firefox's webidl definitions, but I'm assuming it should be exposed to both.

Having said that I just checked the specs at https://www.khronos.org/registry/webgl/specs/latest/1.0/webgl.idl, I didn't see _any_ of the WebGL interfaces exposed, but I'm not sure if that's relevant.

[Ms2ger]

If there's no [Exposed], that means the same as [Exposed=Window].

[scotttrinh]

@Ms2ger I would assume that the spec should trump Firefox's implementation, but I don't want to assume that. Any guidance? Firefox says everything is available to the Worker, but the spec has no exposed (therefore [Exposed=Window] as you mentioned).

[Ms2ger]

Let's keep WebGL limited to Window, at least for now.

[scotttrinh]

Taking CanvasGradient, CanvasPattern, CanvasRenderContext2D, if someone can check those boxes.

[scotttrinh]

Turns out I should take HTMLCanvasElement! Check the box for that one for me, too.

[scotttrinh]

Once #13747 is finished, I'll take:

• BluetoothAdvertisingData.webidl
• BluetoothCharacteristicProperties.webidl
• BluetoothDevice.webidl
• BluetoothRemoteGATTCharacteristic.webidl
• BluetoothRemoteGATTDescriptor.webidl
• BluetoothRemoteGATTServer.webidl
• BluetoothRemoteGATTService.webidl
• BluetoothUUID.webidl
• Bluetooth.webidl
• Navigator.webidl

[scotttrinh]

Oof. Bluetooth* seems to be one of those threads that is pulling a bunch of other things into it: Navigator, Document, Element, Comment, etc. etc. etc.

Should I let someone else handle this or are you guys cool with a big PR that fixes a few dozen things all in one?

[jdm]

Another fix you can do is add an Exposed annotation on the properties that return/accept interfaces that are conditionally exposed (like matchMedia in Window). That may avoid the need for a large PR right now. However, those will need to be done sometime, so I wouldn't mind it happening now.

[scotttrinh]

I'm all for not kicking the can down the road, so I'll scoop up a bunch of interfaces into this PR.

[lucasloisp]

I'll take

• HTMLLinkElement.webidl
• HTMLMapElement.webidl
• HTMLMediaElement.webidl
• HTMLMetaElement.webidl
• HTMLMeterElement.webidl
• HTMLModElement.webidl
• HTMLObjectElement.webidl
• HTMLOListElement.webidl
• HTMLOptGroupElement.webidl
• HTMLOptionElement.webidl
• HTMLOutputElement.webidl

[scotttrinh]

@lucasloisp
Those are all on the list of interfaces I would need to remove in the aforementioned "bunch of interfaces", so thanks for taking them. I'll keep an eye out for your PR to be merged and then I'll finish up mine with whatever is left. 🙌🏽

[lucasloisp]

Should be done with it later today if nothing in unexpected comes up. I'll mention you in my PR so you know it's done

[lucasloisp]

@woshilapin If you could update your list it would be great. I just updated a few Interfaces (list above)

@jdm this issue should be reopened. It was closed automatically by bors-servo. Just letting you know, as you reopened it last time

[scotttrinh]

I'm working on XMLHttpRequest.webidl and I'm running into a syntax error:

WebIDL.WebIDLError: error: invalid syntax, /Users/strinh/projects/servo/components/script/dom/webidls/XMLHttpRequest.webidl line 73:2
  [Exposed=Window] readonly attribute Document? responseXML;
  ^

Anyone know how to correctly annotate this attribute with the Exposed extended attribute? It's written like that in the spec, and it's commented out in the original just like that...

[jdm]

The WebIDL parser is complaining about multiple independent annotations: [Throws] [Exposed=Window]. They should be merged into one annotation instead.

[scotttrinh]

I'll take DOM*.

[Haggus]

I just added a PR which fixes Touch, TouchList and ValidityState. Please check them off the list.

[nox]

AFAICT, WebGL interfaces haven't been taken care of yet.

[nox]

The WebGL interfaces are taken care of by https://github.com/servo/servo/pull/14376.

[montrivo]

I checked the following interfaces, they can be removed from the list:
Location.webidl
MediaError.webidl
MimeTypeArray.webidl
MimeType.webidl
PageTransitionEvent.webidl
PluginArray.webidl
Plugin.webidl
PopStateEvent.webidl

I noticed PerformanceTiming.webidl and Performance.webidl specs have changed. Should those files be updated ?

[jdm]

Updating those other interfaces separately would be useful!

[montrivo]

I checked

CSSStyleDeclaration.webidl
ElementCSSInlineStyle.webidl
ElementContentEditable.webidl
EventHandler.webidl
Screen.webidl
StyleSheetList.webidl
StyleSheet.webidl
Client.webidl

I had issues with the following:

• BrowserElement.webidl no specs

• ProgressEvent.webidl

 WebIDL.WebIDLError: error: Unknown [Exposed] value SharedWorker, /home/tim/dev/servo/components/script/dom/webidls/ProgressEvent.webidl line 17:0
interface ProgressEvent : Event {

• StorageEvent.webidl and Storage.webidl

error[E0308]: mismatched types
    --> /home/tim/dev/servo/target/debug/build/script-2db9432a6e2c7798/out/Bindings/StorageEventBinding.rs:1031:83
     |
1031 |         let result: Result<Root<StorageEvent>, Error> = StorageEvent::Constructor(&global, arg0, &arg1);
     |                                                                                   ^^^^^^^ expected struct `dom::globalscope::GlobalScope`, found struct `dom::bindings::js::Root`
     |
     = note: expected type `&dom::globalscope::GlobalScope`
                found type `&dom::bindings::js::Root<dom::window::Window>`

error: aborting due to previous error

error: Could not compile `script`.

[jdm]

Yeah, BrowserElement.webidl is a Servo-only interface. The error about SharedWorker makes sense since we don't support Shared Workers yet (so it can be left out). The errors in the storage IDLs can be fixed by making StorageEvent::Constructor take a &Window argument instead of &GlobalScope in storageevent.rs.

[jdm]

We've done as much as we can in this issue. I'm filing individual ones for any remaining interfaces.