improve spec compliance of window.close by gterzian · Pull Request #20865 · servo/servo

gterzian · 2018-05-26T15:45:30Z

Improve the spec compliance of https://html.spec.whatwg.org/multipage/window-object.html#dom-window-close, mainly by implementing the steps related to closing a browsing context

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-05-26T15:45:33Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs, components/script/dom/window.rs
@cbrewster: components/constellation/constellation.rs
@paulrouget: ports/servo/browser.rs, components/constellation/constellation.rs
@fitzgen: components/script_traits/script_msg.rs, components/script/dom/window.rs
@KiChjang: components/script_traits/script_msg.rs, components/script/dom/window.rs

servo-wpt-sync · 2018-05-26T15:45:51Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

servo-wpt-sync · 2018-05-26T15:49:57Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

servo-wpt-sync · 2018-05-26T15:55:38Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

servo-wpt-sync · 2018-05-26T16:01:48Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

servo-wpt-sync · 2018-05-26T16:10:44Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

servo-wpt-sync · 2018-05-26T16:17:30Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

servo-wpt-sync · 2018-05-26T16:18:10Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

servo-wpt-sync · 2018-05-26T16:19:03Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

gterzian · 2018-05-26T16:24:31Z

@bors-servo try

bors-servo · 2018-05-26T16:24:50Z

⌛ Trying commit caea691 with merge 3b6649f...

… r=<try> improve spec compliance of window.close  Improve the spec compliance of https://html.spec.whatwg.org/multipage/window-object.html#dom-window-close, mainly by implementing the steps related to [closing a browsing context](https://html.spec.whatwg.org/multipage/window-object.html#close-a-browsing-context) ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20865)

bors-servo · 2018-05-26T16:49:20Z

💔 Test failed - linux-dev

CYBAI · 2018-05-26T17:02:19Z

Checking files for tidiness...

./components/compositing/compositor_thread.rs:71: found an empty line following a {

./components/constellation/constellation.rs:1040: trailing whitespace

Running the dependency licensing lint...

gterzian · 2018-05-27T02:38:05Z

@CYBAI thanks, should have waited for CI to finish...
Looks like all tests were passing, with the exceptions of the known websocket intermittent problem.

servo-wpt-sync · 2018-05-27T02:38:26Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

cbrewster · 2018-05-28T19:56:12Z

Sorry I've been busy/without WiFi for a week or so. I should be able to look at this soon.

gterzian · 2018-05-29T04:14:25Z

@jdm I haven't had a chance yet to follow the instructions on how to setup wpt as a standalone server, however just trying out window.close manually in Firefox gives me the following warning Scripts may not close windows that were not opened by script. (which seems to be missing the second or if it is a top-level browsing context whose session history contains only one Document part of the spec...).

The way the test currently behaves with Servo is that the window is indeed closed so you can't really see the results, however the test passes (and does not crash like it would without these changes) because done is called in the unload listener, which per the spec happens before the window is actually closed...

I was also thinking that since unload is fired inside document.unload, as opposed to inside a task, we would get some form of deterministic behavior where we can sure that the test will pass in case unload is indeed fired before the window is closed(and I'm guessing it would timeout in Firefox).

cbrewster

Looks good. I think we still need to make sure that the compositor shuts down properly.

cbrewster · 2018-06-03T20:56:37Z

components/script/dom/window.rs

-        self.main_thread_script_chan()
-            .send(MainThreadScriptMsg::ExitWindow(self.upcast::<GlobalScope>().pipeline_id()))
-            .unwrap();
+        // Note: check the length of the "session history", as opposed to the joint session history?


This would probably be something that should be clarified in the spec, perhaps open an issue there. I would think the jsh is the right thing to look at here, but I could be wrong.

Filed whatwg/html#3734

cbrewster · 2018-06-03T20:59:36Z

components/script_traits/script_msg.rs

-    GetScreenAvailSize(IpcSender<(DeviceUintSize)>),
-    /// Requests that the compositor shut down.
-    Exit,
+    GetScreenAvailSize(IpcSender<(DeviceUintSize)>)


nit: add trailing ,

cbrewster · 2018-06-03T20:59:59Z

components/script_traits/script_msg.rs

            GetScreenSize(..) => "GetScreenSize",
-            GetScreenAvailSize(..) => "GetScreenAvailSize",
-            Exit => "Exit",
+            GetScreenAvailSize(..) => "GetScreenAvailSize"


nit: add trailing ,

cbrewster · 2018-06-03T21:50:24Z

components/compositing/compositor.rs

            }

-            (Msg::Exit, _) => {
-                self.start_shutting_down();


I don't think this gets called anymore after these changes.

By appending WindowEvent::Quit here, compositor.maybe_start_shutting_down ends up being called via Servo's handling of WindowEvent::Quit here, and maybe_start_shutting_down ends up calling start_shutting_down.

The difference is that the compositor is shut down only after the unload steps have been run.

If the embedder were to handle tabs or something similar, it wouldn't have to send a WindowEvent::Quit in case other "tabs" were still running... (and I might actually have to end up doing something similar as part of #20678 to make some of the related tests pass)

Ah missed that! This sounds reasonable.

CYBAI · 2018-06-04T03:07:07Z

ports/servo/browser.rs

+                    // Nothing left to do for now,
+                    // but could hide a tab, and show another one, instead of quitting.
+                    self.event_queue.push(WindowEvent::Quit);
+                }


Just found the , is inconsistent in this pattern matching.
Should we fix it in this PR or have an E-easy issue to make the consistency of comma for pattern matching?

Thanks, I can add a comma for this specific change, and you could make it consistent across the rest in a separate PR?

Yes, I think I can send one after this one merged.

servo-wpt-sync · 2018-06-04T05:52:24Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

gterzian · 2018-06-04T05:52:42Z

@cbrewster Thanks for the review, I've fixed the nits and replied to the other points...

servo-wpt-sync · 2018-06-04T07:03:24Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#63.

cbrewster · 2018-06-04T10:16:41Z

LGTM, the test does fail in other browsers, but I believe it is spec compliant. The test should be updated in the future if the spec ends up changing.

@bors-servo r+

bors-servo · 2018-06-04T10:16:42Z

📌 Commit 2753e5e has been approved by cbrewster

bors-servo · 2018-06-04T10:16:49Z

⌛ Testing commit 2753e5e with merge 65eff4f...

… r=cbrewster improve spec compliance of window.close  Improve the spec compliance of https://html.spec.whatwg.org/multipage/window-object.html#dom-window-close, mainly by implementing the steps related to [closing a browsing context](https://html.spec.whatwg.org/multipage/window-object.html#close-a-browsing-context) ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20865)

gterzian · 2018-06-04T10:54:35Z

@cbrewster thanks for checking the other browsers!

bors-servo · 2018-06-04T11:39:41Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: cbrewster
Pushing 65eff4f to master...

Upstreamed from servo/servo#20865 [ci skip]

highfive assigned jdm May 26, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 26, 2018

servo-wpt-sync mentioned this pull request May 26, 2018

improve spec compliance of window.close jdm/web-platform-tests#63

Merged

gterzian force-pushed the improve_spec_compliance_window_close branch from 7eff72a to 38ef920 Compare May 26, 2018 15:49

gterzian force-pushed the improve_spec_compliance_window_close branch from 38ef920 to 4160746 Compare May 26, 2018 15:55

gterzian force-pushed the improve_spec_compliance_window_close branch from 4160746 to 3c5c135 Compare May 26, 2018 16:01

gterzian force-pushed the improve_spec_compliance_window_close branch from 3c5c135 to df6b835 Compare May 26, 2018 16:10

gterzian force-pushed the improve_spec_compliance_window_close branch from df6b835 to 12195e7 Compare May 26, 2018 16:16

gterzian force-pushed the improve_spec_compliance_window_close branch from 12195e7 to b5aedac Compare May 26, 2018 16:17

gterzian force-pushed the improve_spec_compliance_window_close branch from b5aedac to caea691 Compare May 26, 2018 16:18

highfive added the S-tests-failed The changes caused existing tests to fail. label May 26, 2018

gterzian force-pushed the improve_spec_compliance_window_close branch from caea691 to 080af33 Compare May 27, 2018 02:38

highfive removed the S-tests-failed The changes caused existing tests to fail. label May 27, 2018

gterzian force-pushed the improve_spec_compliance_window_close branch from 080af33 to 89889e7 Compare May 27, 2018 02:43

highfive assigned cbrewster May 28, 2018

gterzian mentioned this pull request Jun 3, 2018

Implement Window.open and related infrastructure #20678

Merged

6 tasks

cbrewster suggested changes Jun 3, 2018

View reviewed changes

cbrewster added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 3, 2018

CYBAI reviewed Jun 4, 2018

View reviewed changes

gterzian mentioned this pull request Jun 4, 2018

History Length in #script-closeable whatwg/html#3734

Closed

gterzian force-pushed the improve_spec_compliance_window_close branch from e5df6eb to 397e5fd Compare June 4, 2018 05:52

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 4, 2018

improve spec compliance of window.close

2753e5e

gterzian force-pushed the improve_spec_compliance_window_close branch from 397e5fd to 2753e5e Compare June 4, 2018 07:03

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 Jun 4, 2018

bors-servo merged commit 2753e5e into servo:master Jun 4, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 4, 2018

gterzian deleted the improve_spec_compliance_window_close branch June 4, 2018 13:02

jdm pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2018

improve spec compliance of window.close

9069371

Upstreamed from servo/servo#20865 [ci skip]

servo-wpt-sync mentioned this pull request Jun 7, 2018

improve spec compliance of window.close web-platform-tests/wpt#11411

Merged

Uh oh!

Conversation

gterzian commented May 26, 2018 • edited by SimonSapin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

servo-wpt-sync commented May 26, 2018

Uh oh!

gterzian commented May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors-servo commented May 26, 2018

Uh oh!

bors-servo commented May 26, 2018

Uh oh!

CYBAI commented May 26, 2018

Uh oh!

gterzian commented May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

servo-wpt-sync commented May 27, 2018

Uh oh!

cbrewster commented May 28, 2018

Uh oh!

gterzian commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbrewster 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!

gterzian Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gterzian Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

servo-wpt-sync commented Jun 4, 2018

Uh oh!

gterzian commented Jun 4, 2018

Uh oh!

servo-wpt-sync commented Jun 4, 2018

Uh oh!

cbrewster commented Jun 4, 2018

Uh oh!

bors-servo commented Jun 4, 2018

Uh oh!

bors-servo commented Jun 4, 2018

gterzian commented May 26, 2018 •

edited by SimonSapin

Loading

gterzian commented May 26, 2018 •

edited

Loading

gterzian commented May 27, 2018 •

edited

Loading

gterzian commented May 29, 2018 •

edited

Loading

gterzian Jun 4, 2018 •

edited

Loading

gterzian Jun 4, 2018 •

edited

Loading