Get rid of dom::bindings::global by nox · Pull Request #13596 · servo/servo

nox · 2016-10-05T09:12:15Z

Globals in that PR are now represented by the fake IDL interface GlobalScope.

This change is

highfive · 2016-10-05T09:12:19Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/request.rs, components/script/dom/workerlocation.rs, components/script/dom/textencoder.rs, components/script/dom/dompoint.rs, components/script/dom/webglframebuffer.rs, components/script/dom/crypto.rs, components/script/dom/client.rs, components/script/dom/focusevent.rs, components/script/dom/bindings/codegen/parser/inline.patch, components/script/dom/htmlcanvaselement.rs, components/script/dom/servohtmlparser.rs, components/script/dom/blob.rs, components/script/dom/location.rs, components/script/dom/eventtarget.rs, components/script/timers.rs, components/script/dom/popstateevent.rs, components/script/dom/mimetypearray.rs, components/script/dom/htmlformcontrolscollection.rs, components/script/dom/workerglobalscope.rs, components/script/dom/bindings/error.rs, components/script/dom/pluginarray.rs, components/script/dom/imagedata.rs, components/script/dom/domstringmap.rs, components/script/fetch.rs, components/script/dom/bindings/iterable.rs, components/script/dom/touchlist.rs, components/script/dom/file.rs, components/script/dom/webglbuffer.rs, components/script/dom/htmltextareaelement.rs, components/script/dom/validitystate.rs, components/script/dom/htmldetailselement.rs, components/script/dom/urlsearchparams.rs, components/script/dom/htmlimageelement.rs, components/script/script_thread.rs, components/script/dom/css.rs, components/script/dom/promise.rs, components/script/dom/worker.rs, components/script/dom/attr.rs, components/script/dom/console.rs, components/script/dom/globalscope.rs, components/script/dom/domrectreadonly.rs, components/script/dom/event.rs, components/script/dom/radionodelist.rs, components/script/dom/webglshader.rs, components/script/dom/domrect.rs, components/script/dom/node.rs, components/script/dom/workernavigator.rs, components/script/dom/canvasgradient.rs, components/script/dom/documentfragment.rs, components/script/dom/serviceworkercontainer.rs, components/script/dom/progressevent.rs, components/script/dom/textdecoder.rs, components/script/dom/canvaspattern.rs, components/script/dom/dommatrix.rs, components/script/dom/bluetoothcharacteristicproperties.rs, components/script/dom/mouseevent.rs, components/script/dom/range.rs, components/script/dom/xmldocument.rs, components/script/dom/mod.rs, components/script/dom/pagetransitionevent.rs, components/script/dom/bluetoothuuid.rs, components/script/dom/webidls/WorkerGlobalScope.webidl, components/script/dom/bluetoothdevice.rs, components/script/devtools.rs, components/script/dom/htmllinkelement.rs, components/script/dom/performance.rs, components/script/dom/testbinding.rs, components/script/dom/cssstyledeclaration.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/browsingcontext.rs, components/script/dom/messageevent.rs, components/script/dom/webglcontextevent.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/customevent.rs, components/script/dom/uievent.rs, components/script/dom/extendableevent.rs, components/script/dom/history.rs, components/script/webdriver_handlers.rs, components/script/dom/testbindingpairiterable.rs, components/script/dom/namednodemap.rs, components/script/dom/domrectlist.rs, components/script/dom/touch.rs, components/script/dom/webidls/GlobalScope.webidl, components/script/dom/htmlinputelement.rs, components/script/dom/response.rs, components/script/dom/bluetoothadvertisingdata.rs, components/script/dom/domimplementation.rs, components/script/dom/serviceworkerregistration.rs, components/script/dom/domtokenlist.rs, components/script/dom/servoxmlparser.rs, components/script/dom/bindings/reflector.rs, components/script/dom/xmlhttprequestupload.rs, components/script/dom/eventsource.rs, components/script/dom/closeevent.rs, components/script/dom/webglrenderbuffer.rs, components/script/dom/domexception.rs, components/script/dom/htmlcollection.rs, components/script/dom/treewalker.rs, components/script/dom/nodeiterator.rs, components/script/dom/filereader.rs, components/script/dom/beforeunloadevent.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script/dom/webgluniformlocation.rs, components/script/dom/hashchangeevent.rs, components/script/task_source/mod.rs, components/script/dom/bluetoothremotegattserver.rs, components/script/dom/document.rs, components/script/dom/domquad.rs, components/script/dom/element.rs, components/script/dom/extendablemessageevent.rs, components/script/dom/webglobject.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/text.rs, components/script/dom/bindings/codegen/parser/update.sh, components/script/dom/stylesheet.rs, components/script/dom/bluetoothremotegattservice.rs, components/script/dom/url.rs, components/script/dom/filereadersync.rs, components/script/dom/dompointreadonly.rs, components/script/dom/stylesheetlist.rs, components/script/dom/performancetiming.rs, components/script/dom/webglshaderprecisionformat.rs, components/script/dom/storage.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/keyboardevent.rs, components/script/dom/webglactiveinfo.rs, components/script/task_source/dom_manipulation.rs, components/script/dom/bindings/codegen/parser/WebIDL.py, components/script/dom/storageevent.rs, components/script/dom/bindings/global.rs, components/script/dom/htmloptionscollection.rs, components/script/dom/navigator.rs, components/script/dom/screen.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/bindings/codegen/Configuration.py, components/script/dom/webgltexture.rs, components/script/dom/touchevent.rs, components/script/dom/bindings/callback.rs, components/script/dom/webglprogram.rs, components/script/dom/filelist.rs, components/script/dom/forcetouchevent.rs, components/script/dom/comment.rs, components/script/dom/mediaerror.rs, components/script/dom/bindings/mod.rs, components/script/dom/eventdispatcher.rs, components/script/dom/serviceworker.rs, components/script/dom/errorevent.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/nodelist.rs, components/script/dom/promisenativehandler.rs, components/script/dom/headers.rs, components/script/dom/domparser.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bluetoothremotegattcharacteristic.rs, components/script/script_runtime.rs, components/script/dom/htmlbodyelement.rs, components/script/dom/htmliframeelement.rs, components/script/dom/dommatrixreadonly.rs, components/script/dom/testbindingiterable.rs, components/script/body.rs, components/net_traits/request.rs, components/net_traits/request.rs, components/script/dom/htmlformelement.rs, components/script/task_source/user_interaction.rs, components/script/dom/serviceworkerglobalscope.rs, components/script/dom/websocket.rs, components/script/dom/bindings/structuredclone.rs, components/script/dom/formdata.rs
@emilio: components/script/dom/webglframebuffer.rs, components/script/dom/webglbuffer.rs, components/script/dom/webglshader.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/webglcontextevent.rs, components/script/dom/webglrenderbuffer.rs, components/script/dom/webgluniformlocation.rs, components/script/dom/webglobject.rs, components/script/dom/webglshaderprecisionformat.rs, components/script/dom/webglactiveinfo.rs, components/script/dom/webgltexture.rs, components/script/dom/webglprogram.rs

highfive · 2016-10-05T09:12:20Z

Warning

These commits modify unsafe code. Please review it carefully!

nox · 2016-10-05T09:12:24Z

Cc @jdm @Ms2ger

jdm · 2016-10-05T11:44:53Z

For the record, when I glanced through a few of the first and last commits the other day, I was very much in favour of these changes.

nox · 2016-10-05T11:45:50Z

@jdm To be honest, I was expecting the opposite. Woooo! :D

nox · 2016-10-05T13:37:24Z

@bors-servo try

Get rid of dom::bindings::global Globals in that PR are now represented by the fake IDL interface `GlobalScope`.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13596)

bors-servo · 2016-10-05T13:37:27Z

⌛ Trying commit b644157 with merge 12f8180...

bors-servo · 2016-10-05T14:58:26Z

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

Ms2ger

LGTM, I guess.

Ms2ger · 2016-10-06T12:58:36Z

components/script/dom/file.rs

        let replaced_filename = DOMString::from_string(filename.replace("/", ":"));
-        Ok(File::new(global, BlobImpl::new_from_bytes(bytes), replaced_filename, modified, typeString))
+        Ok(File::new(global.as_global_scope(),
+                    BlobImpl::new_from_bytes(bytes),


Indentation

Ms2ger · 2016-10-06T13:17:04Z

components/script/dom/xmlhttprequest.rs

        // Step 1
-        match self.global() {
-            GlobalRoot::Window(ref window) => {
+        match Root::downcast::<Window>(self.global_scope()) {


Ms2ger · 2016-10-06T13:18:21Z

components/script/dom/globalscope.rs

+        match *self.type_id() {
+            // https://html.spec.whatwg.org/multipage/#script-settings-for-browsing-contexts:api-base-url
+            GlobalScopeTypeId::Window => {
+                self.downcast::<Window>().unwrap().Document().base_url()


I'd prefer two if lets on self.downcast::<T>().

Ms2ger · 2016-10-06T13:23:16Z

components/script/dom/globalscope.rs

        }
    }
+
+    /// Extract a `Window`, causing thread failure if the global object is not


We call that a panic now.

Ms2ger · 2016-10-06T13:26:16Z

components/script/dom/blob.rs

                    None => {
-                        let global = self.global();
-                        let bytes = read_file(global.r(), f.id.clone())?;
+                        let bytes = read_file(&self.global_scope(), f.id.clone())?;


Who's been putting ? in my code...

Ms2ger · 2016-10-06T13:34:48Z

components/script/dom/globalscope.rs

+    pub fn script_chan(&self) -> Box<ScriptChan + Send> {
+        match *self.type_id() {
+            GlobalScopeTypeId::Window => {
+                let window = self.downcast::<Window>().unwrap();


Ms2ger · 2016-10-06T13:35:12Z

components/script/dom/globalscope.rs

+    /// this of this global scope.
+    pub fn networking_task_source(&self) -> Box<ScriptChan + Send> {
+        match *self.type_id() {
+            GlobalScopeTypeId::Window => {


You guessed it...

Ms2ger · 2016-10-06T13:35:51Z

components/script/dom/bindings/mod.rs

 //! Methods that use certain WebIDL types like `any` or `object` will get a
 //! `*mut JSContext` argument prepended to the argument list. Static methods
-//! will be passed a [`GlobalRef`](global/enum.GlobalRef.html) for the relevant
+//! will be passed a `&GlobalScope` for the relevant


Keep linking, please.

Ms2ger · 2016-10-06T13:47:37Z

components/script/dom/window.rs

    // https://fetch.spec.whatwg.org/#fetch-method
    fn Fetch(&self, input: RequestOrUSVString, init: &RequestInit) -> Rc<Promise> {
-        fetch::Fetch(self.global().r(), input, init)
+        fetch::Fetch(&self.global_scope(), input, init)


Ms2ger · 2016-10-06T13:47:46Z

components/script/dom/workerglobalscope.rs

    // https://fetch.spec.whatwg.org/#fetch-method
    fn Fetch(&self, input: RequestOrUSVString, init: &RequestInit) -> Rc<Promise> {
-        fetch::Fetch(self.global().r(), input, init)
+        fetch::Fetch(&self.global_scope(), input, init)


bors-servo · 2016-10-06T18:28:54Z

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

Inline interfaces just appear as a Rust type and in the TypeId hierarchy. They are completely invisible on the JS side.

Ms2ger · 2016-10-07T12:50:58Z

@bors-servo delegate+

LGTM assuming only changes were rebasing and fixing my comments.

bors-servo · 2016-10-07T12:50:59Z

✌️ @nox can now approve this pull request

nox · 2016-10-07T12:52:07Z

@bors-servo r=Ms2ger p=10

bors-servo · 2016-10-07T12:52:08Z

📌 Commit d8e92bb has been approved by Ms2ger

bors-servo · 2016-10-07T12:52:10Z

⌛ Testing commit d8e92bb with merge a6e4b5b...

Get rid of dom::bindings::global Globals in that PR are now represented by the fake IDL interface `GlobalScope`.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13596)

bors-servo · 2016-10-07T13:26:40Z

💔 Test failed - linux-rel-wpt

highfive · 2016-10-07T13:26:42Z

  ▶ FAIL [expected PASS] /_mozilla/css/inline_block_opacity_change.html
  └   → /_mozilla/css/inline_block_opacity_change.html aed6845267bba6c52d3aa69c4889449b454d8117
/_mozilla/css/inline_block_opacity_change_ref.html febec1b4730afab195f280694dad47200b09ea3c
Testing aed6845267bba6c52d3aa69c4889449b454d8117 == febec1b4730afab195f280694dad47200b09ea3c

nox · 2016-10-07T13:36:07Z

@bors-servo retry #13360

bors-servo · 2016-10-07T13:36:09Z

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

bors-servo · 2016-10-07T14:08:45Z

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

nox added A-content/bindings The DOM bindings I-refactor No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring. A-content/script Related to the script thread labels Oct 5, 2016

highfive assigned emilio Oct 5, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 5, 2016

Ms2ger suggested changes Oct 6, 2016

View reviewed changes

nox force-pushed the inline branch from b644157 to 1dc6664 Compare October 6, 2016 15:38

highfive added the S-needs-rebase There are merge conflict errors. label Oct 6, 2016

nox added 12 commits October 6, 2016 20:59

Prevent doing use self::FooBinding::{}

b745866

Implement [Inline] interfaces

5a42bb5

Inline interfaces just appear as a Rust type and in the TypeId hierarchy. They are completely invisible on the JS side.

Remove workerglobalscope::WorkerGlobalScopeTypeId

093b189

Make reflect_dom_object take a &GlobalScope

fcb59d3

Introduce GlobalScope::crypto

38273fe

Introduce GlobalScope::get_next_worker_id

3e5c0c3

Introduce GlobalScope::live_devtools_updates

d7c2da4

Move console timers to GlobalScope

14a0b8d

Introduce GlobalScope::devtools_chan

fe6fca9

Introduce GlobalScope::mem_profiler_chan

bfa7d04

Introduce GlobalScope::time_profiler_chan

ae3763e

Introduce GlobalScope::constellation_chan

f789e73

Rename Reflectable::global_scope to global

d8e92bb

nox force-pushed the inline branch from 1dc6664 to d8e92bb Compare October 6, 2016 19:41

nox removed the S-needs-rebase There are merge conflict errors. label Oct 6, 2016

highfive assigned Ms2ger and unassigned emilio Oct 7, 2016

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 Oct 7, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 7, 2016

bors-servo merged commit d8e92bb into servo:master Oct 7, 2016

bors-servo mentioned this pull request Oct 7, 2016

Implement job queue for ServiceWorkerRegistration #13574

Merged

5 tasks

nox deleted the inline branch October 7, 2016 14:09

Uh oh!

Conversation

nox commented Oct 5, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Oct 5, 2016

Uh oh!

highfive commented Oct 5, 2016

Uh oh!

nox commented Oct 5, 2016

Uh oh!

jdm commented Oct 5, 2016

Uh oh!

nox commented Oct 5, 2016

Uh oh!

nox commented Oct 5, 2016

Uh oh!

bors-servo commented Oct 5, 2016

Uh oh!

bors-servo commented Oct 5, 2016

Uh oh!

Ms2ger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Oct 6, 2016

Uh oh!

Ms2ger commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

nox commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

highfive commented Oct 7, 2016

Uh oh!

nox commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nox commented Oct 5, 2016 •

edited by larsbergstrom

Loading