Enable screen.width/height/availWidth/availHeight #18183

shinglyu · 2017-08-22T07:59:17Z

Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget

Since glutin don't have availWidth and availHeight information, I let them fallback to width and height. If that's not acceptable in terms of webcompat, I can remove that part.

There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine.

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

There are tests for these changes in wpt cssom-view
These changes do not require tests because _____

This change is

highfive · 2017-08-22T07:59:21Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@fitzgen: components/script/dom/webidls/Screen.webidl, components/script/dom/screen.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs
@KiChjang: components/script/dom/webidls/Screen.webidl, components/script/dom/screen.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs

paulrouget · 2017-08-22T09:21:50Z

components/script/dom/screen.rs

+    fn AvailHeight(&self) -> Finite<f64> {
+        // FIXME: Not spec complient because glutin don't have corresponding API
+        // Fallback to window.screen.height behavior
+        self.Height()


The FIXME should be in /ports/, not in servo. Code in /components/ are backend independent.

Here you should call self.avail_screen_size().height, and implement the associated message (GetAvailScreenSize).

In ports/glutin/window.rs, implement fn avail_screen_size(), and there, add the FIXME and use glutin::get_primary_monitor().get_dimensions();.

paulrouget

Move fallback to /ports/.

jdm · 2017-08-22T17:26:07Z

The throwing tests no longer exist upstream, so that's fine.

shinglyu · 2017-08-24T03:39:04Z

@paulrouget I create a separate path for availWidth/availHeight all the way down to WindowEvent and moved the fallback to ports/glutin. Thanks!

paulrouget · 2017-08-24T03:44:44Z

components/compositing/compositor.rs

+             ShutdownState::NotShuttingDown) => {
+                let screen_size = self.window.screen_size(top_level_browsing_context_id);
+                if let Err(e) = send.send(screen_size) {
+                    warn!("Sending response to get client window failed ({}).", e);


"to get screen size"

paulrouget · 2017-08-24T03:44:55Z

components/compositing/compositor.rs

+             ShutdownState::NotShuttingDown) => {
+                let screen_size = self.window.screen_avail_size(top_level_browsing_context_id);
+                if let Err(e) = send.send(screen_size) {
+                    warn!("Sending response to get client window failed ({}).", e);


"to get screen avail size"

paulrouget · 2017-08-24T03:52:13Z

ports/glutin/window.rs

+        let (width, height) = glutin::get_primary_monitor().get_dimensions();
+        Size2D::new(width, height)
+    }
+


In the headless case, you probably want to return context.width/height for both screen_size and screen_avail_size.

paulrouget

You also need to fix CEF (try ./mach build-cef).

paulrouget · 2017-08-24T07:30:27Z

Looks good to me.

Can someone review this?

shinglyu · 2017-08-24T07:48:39Z

Maybe @jdm, who suggested this solution?

asajeffrey

Looks good, some minor nits.

asajeffrey · 2017-08-24T08:45:01Z

ports/cef/window.rs

+                browser.get_host()
+                       .get_client()
+                       .get_render_handler()
+                       .get_screen_info((*browser).clone(), &mut screen_info);


Can this just be browser.clone()?

asajeffrey · 2017-08-24T08:45:01Z

ports/cef/window.rs

+                    is_monochrome: 0 as i32,
+                    rect: cef_rect_t::zero(),
+                    available_rect: cef_rect_t::zero(),
+                };


Not very nice having to do this. Is there an implementation of Default::default() we could use to make this slightly better?

asajeffrey · 2017-08-24T08:45:01Z

tests/wpt/metadata-css/cssom-view-1_dev/html/cssom-view-window-screen-interface.htm.ini

-
-  [window.screen.availHeight >= 0 && window.screen.availHeight <= window.screen.height]
-    expected: FAIL
-


Yay for passing tests!

asajeffrey · 2017-08-24T08:45:01Z

components/compositing/compositor_thread.rs

+    /// Get the screen size
+    GetScreenSize(TopLevelBrowsingContextId, IpcSender<(Size2D<u32>)>),
+    /// Get the available screen size
+    GetScreenAvailSize(TopLevelBrowsingContextId, IpcSender<(Size2D<u32>)>),


This isn't really an issue with this PR, but it would be nice if we could give these typed units, since I'm not sure what the unit is (device pixels?). A comment with the unit at least would be nice.

Are you suggesting we use Typed* for all compositor message? Will that mean more bytes passing through IPC? or is it just a type system thing and has no runtime impact?

Typed units do not have a runtime impact.

shinglyu · 2017-08-28T02:55:13Z

@asajeffrey Changing to Typed units seems like a good first bug for contributors. I'm going to create a followup bug for that and hand it to a new volunteer I'm mentoring here in Taiwan. So I only added a comment about the units.

shinglyu · 2017-11-12T08:28:41Z

Sorry, new job, new team, new house, so didn't have time to check github for a while :'( Just pushed a fix and I'm testing it locally now.

shinglyu · 2017-11-12T09:00:47Z

@bors-servo: try

bors-servo · 2017-11-12T09:00:52Z

⌛ Trying commit e7cbe84 with merge db31197...


        Auto merge of #18183 - shinglyu:availheight, r=<try>

@paulrouget

Enable screen.width/height/availWidth/availHeight  Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part. There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18062 (github issue number if applicable).  - [x] There are tests for these changes in wpt cssom-view - [ ] 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/18183)

bors-servo · 2017-11-12T09:36:19Z

💔 Test failed - linux-rel-wpt

shinglyu · 2017-11-13T20:18:03Z

The merge commit db31197 bors tested didn't contain the removed test expectation files in the original commit e7cbe84, so there are a few unexpected passes. Ideas?

jdm · 2017-11-13T20:22:45Z

@shinglyu tests/wpt/metadata-css/ doesn't exist any more, and the merge algorithm apparently decided that that wasn't important.

mbrubeck · 2017-11-13T20:23:07Z

The merge commit seems to have the relevant test expectation removals:

https://github.com/servo/servo/tree/db311974f63a2dc17c821f2c318c2e5379a275cd/tests/wpt/metadata

(diffs on merge commits aren't very clear)

shinglyu · 2017-11-13T20:23:46Z

Ah OK, let me do another rebase.


        Enable screen.availHeight/availWidth/Height/Width

shinglyu · 2017-11-13T20:34:24Z

Removed the test expectations that was moved to metadata/, trying again.

@bors-servo: try

bors-servo · 2017-11-13T20:34:28Z

⌛ Trying commit c120234 with merge 4a8e0ac...


        Auto merge of #18183 - shinglyu:availheight, r=<try>

@paulrouget

Enable screen.width/height/availWidth/availHeight  Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part. There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18062 (github issue number if applicable).  - [x] There are tests for these changes in wpt cssom-view - [ ] 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/18183)

bors-servo · 2017-11-13T21:36:53Z

☀️ 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
State: approved= try=True

asajeffrey · 2017-11-13T22:55:42Z

Yay, it landed!

jdm · 2017-11-13T23:06:22Z

@bors-servo: r=asajeffrey

bors-servo · 2017-11-13T23:06:23Z

📌 Commit c120234 has been approved by asajeffrey

bors-servo · 2017-11-13T23:06:27Z

⌛ Testing commit c120234 with merge 504ad4d...


        Auto merge of #18183 - shinglyu:availheight, r=asajeffrey

@paulrouget

Enable screen.width/height/availWidth/availHeight  Support screen.width/height/availWidth/availHeight using information from glutin. r?@paulrouget Since glutin don't have `availWidth` and `availHeight` information, I let them fallback to `width` and `height`. If that's not acceptable in terms of webcompat, I can remove that part. There are some test failures on wpt css about mutating screen.width/height should throw exception, but I can't reproduce that behavior on other major browser, so I keep them disabled. Also there are some media query issues, but I believe that's due to some test harness problem on my test machine. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18062 (github issue number if applicable).  - [x] There are tests for these changes in wpt cssom-view - [ ] 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/18183)

bors-servo · 2017-11-14T00:02:10Z

☀️ 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: asajeffrey
Pushing 504ad4d to master...

highfive assigned paulrouget Aug 22, 2017

highfive added the S-awaiting-review label Aug 22, 2017

paulrouget reviewed Aug 22, 2017

View changes

paulrouget requested changes Aug 22, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 24, 2017

shinglyu force-pushed the shinglyu:availheight branch from 927a008 to e0346fe Aug 24, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 24, 2017

paulrouget reviewed Aug 24, 2017

View changes

paulrouget requested changes Aug 24, 2017

View changes

shinglyu force-pushed the shinglyu:availheight branch 2 times, most recently from 24e76b3 to 3558b9c Aug 24, 2017

paulrouget approved these changes Aug 24, 2017

View changes

asajeffrey reviewed Aug 24, 2017

View changes

asajeffrey self-assigned this Aug 24, 2017

asajeffrey added S-awaiting-answer and removed S-awaiting-review labels Aug 24, 2017

shinglyu force-pushed the shinglyu:availheight branch from 3558b9c to e259da4 Aug 25, 2017

highfive added the S-awaiting-review label Aug 25, 2017

shinglyu force-pushed the shinglyu:availheight branch from e259da4 to fc3b396 Aug 25, 2017

asajeffrey added the S-needs-squash label Aug 29, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Nov 12, 2017

highfive added the S-tests-failed label Nov 12, 2017

shinglyu force-pushed the shinglyu:availheight branch from e7cbe84 to ac56bf5 Nov 13, 2017

highfive removed the S-tests-failed label Nov 13, 2017

Enable screen.availHeight/availWidth/Height/Width

Loading status checks…

c120234

shinglyu force-pushed the shinglyu:availheight branch from ac56bf5 to c120234 Nov 13, 2017

highfive removed the S-awaiting-review label Nov 13, 2017

highfive added the S-awaiting-merge label Nov 13, 2017

bors-servo merged commit c120234 into servo:master Nov 14, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 14, 2017

servo / servo

Enable screen.width/height/availWidth/availHeight #18183

Enable screen.width/height/availWidth/availHeight #18183

shinglyu commented Aug 22, 2017 •

edited by SimonSapin

highfive commented Aug 22, 2017

This comment has been minimized.

paulrouget left a comment

jdm commented Aug 22, 2017

shinglyu commented Aug 24, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget left a comment

paulrouget commented Aug 24, 2017

shinglyu commented Aug 24, 2017

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shinglyu commented Aug 28, 2017

shinglyu commented Nov 12, 2017

shinglyu commented Nov 12, 2017

bors-servo commented Nov 12, 2017

bors-servo commented Nov 12, 2017

shinglyu commented Nov 13, 2017

jdm commented Nov 13, 2017

mbrubeck commented Nov 13, 2017

shinglyu commented Nov 13, 2017

shinglyu commented Nov 13, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 13, 2017

asajeffrey commented Nov 13, 2017

jdm commented Nov 13, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 14, 2017


		[window.screen.availHeight >= 0 && window.screen.availHeight <= window.screen.height]
		expected: FAIL

servo / servo

Join GitHub today

Enable screen.width/height/availWidth/availHeight #18183

Enable screen.width/height/availWidth/availHeight #18183

Conversation

shinglyu commented Aug 22, 2017 • edited by SimonSapin

highfive commented Aug 22, 2017

This comment has been minimized.

paulrouget left a comment

jdm commented Aug 22, 2017

shinglyu commented Aug 24, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget left a comment

paulrouget commented Aug 24, 2017

shinglyu commented Aug 24, 2017

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shinglyu commented Aug 28, 2017

shinglyu commented Nov 12, 2017

shinglyu commented Nov 12, 2017

bors-servo commented Nov 12, 2017

bors-servo commented Nov 12, 2017

shinglyu commented Nov 13, 2017

jdm commented Nov 13, 2017

mbrubeck commented Nov 13, 2017

shinglyu commented Nov 13, 2017

shinglyu commented Nov 13, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 13, 2017

asajeffrey commented Nov 13, 2017

jdm commented Nov 13, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 14, 2017

shinglyu commented Aug 22, 2017 •

edited by SimonSapin