[WIP] Unfork glutin. by glennw · Pull Request #17311 · servo/servo

glennw · 2017-06-14T06:53:09Z

This builds and runs correctly on Linux with upstream glutin.

Moving to upstream glutin would have a few advantages:

Get future updates easily.
Allow us to upstream some planned optimization work on glutin (primarily swap buffers and dirty rect support).
Easily work with D3D / Wayland / Vulkan backends.

There's a number of items that would need to be checked / fixed before we could land this:

Get compiling and test on Mac.
Get compiling and test on Windows.
Get compiling and test on Android.
Confirm that window resizing and painting is working correctly on Mac.
Get the icon loading patches either upstreamed or rebased on a new glutin fork.
Get the platform_window() code working [or drop CEF for now?].

This change is

highfive · 2017-06-14T06:53:14Z

Warning

These commits modify unsafe code. Please review it carefully!

glennw · 2017-06-14T06:54:03Z

Anyone want to try this / make fixes for Mac / Windows / Android? I think they should be relatively minor.

MortimerGoro · 2017-06-14T08:51:24Z

@glennw I'll compile & test on Android. We'll need to wait for this PR to be merged: rust-windowing/glutin#868. The glutin API have changed, so I need to resubmit the PR, and we´ll need to update the glutin version here when published

jdm · 2017-06-14T14:44:23Z

For mac:

diff --git a/ports/glutin/window.rs b/ports/glutin/window.rs
index e4247c6523..b26a4b769b 100644
--- a/ports/glutin/window.rs
+++ b/ports/glutin/window.rs
@@ -86,7 +86,7 @@ fn builder_with_platform_options(mut builder: glutin::WindowBuilder) -> glutin::
         // output file.
         builder = builder.with_activation_policy(ActivationPolicy::Prohibited)
     }
-    builder.with_app_name(String::from("Servo"))
+    builder.with_title(String::from("Servo"))
 }

 #[cfg(not(target_os = "macos"))]

When resizing, the window only displays white until I let go of the mouse button.

bors-servo · 2017-06-14T15:33:58Z

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

pyfisch · 2017-06-22T17:31:13Z

Platform specific handle_keyboard_input should not be needed anymore with rust-windowing/winit#173.

ahicks92 · 2017-06-25T19:54:52Z

I want to try helping with this on Windows and am going to see if I can get it to compile here shortly.

Can we get a rebase? One of the other things that just got fixed for Windows is the ability to run the WPT suite. It would be useful if this was applied on top of that, I think, in the sense that I'm going to have to somehow have those commits involved

ahicks92 · 2017-06-25T20:19:47Z

A simple ./mach build -d does compile on Windows. Haven't dug into the diff yet.

This builds and runs correctly on Linux with upstream glutin. Moving to upstream glutin would have a few advantages: * Get future updates easily. * Allow us to upstream some planned optimization work on glutin (primarily swap buffers and dirty rect support). * Easily work with D3D / Wayland / Vulkan backends. There's a number of items that would need to be checked / fixed before we could land this: - [ ] Get compiling and test on Mac. - [ ] Get compiling and test on Windows. - [ ] Get compiling and test on Android. - [ ] Confirm that window resizing and painting is working correctly on Mac. - [ ] Get the icon loading patches either upstreamed or rebased on a new glutin fork. - [ ] Get the platform_window() code working [or drop CEF for now?].

jdm · 2017-06-25T23:03:12Z

I have pushed my rebased and amended version of this branch to https://github.com/jdm/servo/tree/unfork-glutin .

jdm · 2017-06-25T23:04:53Z

It currently fails ./mach test-tidy because of many duplicate crate versions, and etc/ci/check_no_panic.sh yields ports/glutin/window.rs:331: panic!("TODO: Support platform_window again!");.

ahicks92 · 2017-06-26T00:59:50Z

@jdm
If providing the URL is for me, rebasing isn't the issue. I'm trying to figure out how to coordinate with @glennw without making everyone's lives difficult.

Is rebasing mine and opening a PR against them, whereupon they rebase theirs (presumably to a later point off master than mine) and then I rebase mine on theirs again going to work? This sort of workflow doesn't seem to be documented by anyone. I thought I was fairly good with git until today.

jdm · 2017-06-26T01:22:28Z

I know; but providing a branch makes it extremely easy for @glennw to reset this PR's branch to an already working rebased copy.

jdm · 2017-06-30T20:09:26Z

Rebased commit has been pushed.

bors-servo · 2017-06-30T22:42:07Z

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

ahicks92 · 2017-07-01T00:18:19Z

I will take another more serious look at this hopefully this weekend.

paulrouget · 2017-07-04T01:07:41Z

ports/glutin/window.rs

            }
-            Event::KeyboardInput(element_state, _scan_code, Some(virtual_key_code)) => {
-                self.handle_keyboard_input(element_state, _scan_code, virtual_key_code);
+            Event::WindowEvent { event: glutin::WindowEvent::KeyboardInput(state, scan_code, Some(virtual_key_code), _), .. } => {


Shouldn't we use the new mods parameter and not rely on key_modifiers?

paulrouget · 2017-07-04T01:09:38Z

ports/glutin/window.rs

-                        }
-                    }
+                    let mouse_pos = self.mouse_pos.get();
+                    self.handle_mouse(mouse_button, element_state, mouse_pos.x, mouse_pos.y);


We need pos. mouse_pos is updated on mouse move. But it's possible that the click happened without a prior mouse move event. For example, if the mouse changes position when servo was not focused.

paulrouget · 2017-07-04T01:10:04Z

ports/glutin/window.rs

-                    self.mouse_pos.set(Point2D::new(x, y));
-                    self.event_queue.borrow_mut().push(
-                        WindowEvent::MouseWindowMoveEventClass(TypedPoint2D::new(x as f32, y as f32)));
-                };


Same comment as for click. pos is necessary.

MortimerGoro · 2017-08-01T16:39:12Z

Glutin has changed the API again. There is not a clear way to rebase the Android PR: rust-windowing/glutin#868

Asked tomaka about the issue. Will change the code & rebase again when we I get his feedback.

emilio · 2017-08-28T11:57:48Z

Any update here? It'd be quite nice to get this landed eventually.

nox · 2017-10-09T10:26:01Z

Ping on this.

paulrouget · 2017-10-09T14:52:36Z

FYI, I built a Servo port (with tab support) that uses the upstream glutin: https://github.com/paulrouget/servoshell - (see the src/platform/glutin).

glennw · 2017-10-17T04:13:28Z

I'm not going to have time to work on this in the near future - closing for now.

highfive assigned nox Jun 14, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 14, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Jun 14, 2017

jdm added the S-fails-travis label Jun 25, 2017

glennw force-pushed the glutin-unfork branch from 8625e0f to 789d57b Compare June 28, 2017 18:02

jdm removed the S-needs-rebase There are merge conflict errors. label Jun 30, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Jun 30, 2017

paulrouget reviewed Jul 4, 2017

View reviewed changes

pyfisch mentioned this pull request Aug 1, 2017

[DONT MERGE] Experimental (browser-like) keyboard input rust-windowing/winit#249

Closed

glennw closed this Oct 17, 2017

nox mentioned this pull request Oct 17, 2017

Unfork glutin #18918

Closed

stuartnelson3 mentioned this pull request Nov 5, 2017

Stn/unfork glutin #19120

Closed

3 tasks

emilio mentioned this pull request Dec 8, 2017

Wrong keycodes reported for non-standard layouts. Fixed upstream servo/glutin#139

Closed

Uh oh!

Conversation

glennw commented Jun 14, 2017 • edited by jdm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 14, 2017

Uh oh!

glennw commented Jun 14, 2017

Uh oh!

MortimerGoro commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdm commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors-servo commented Jun 14, 2017

Uh oh!

pyfisch commented Jun 22, 2017

Uh oh!

ahicks92 commented Jun 25, 2017

Uh oh!

ahicks92 commented Jun 25, 2017

Uh oh!

jdm commented Jun 25, 2017

Uh oh!

jdm commented Jun 25, 2017

Uh oh!

ahicks92 commented Jun 26, 2017

Uh oh!

jdm commented Jun 26, 2017

Uh oh!

jdm commented Jun 30, 2017

Uh oh!

bors-servo commented Jun 30, 2017

Uh oh!

ahicks92 commented Jul 1, 2017

Uh oh!

paulrouget Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

paulrouget Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

paulrouget Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

MortimerGoro commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio commented Aug 28, 2017

Uh oh!

nox commented Oct 9, 2017

Uh oh!

paulrouget commented Oct 9, 2017

Uh oh!

glennw commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

glennw commented Jun 14, 2017 •

edited by jdm

Loading

MortimerGoro commented Jun 14, 2017 •

edited

Loading

jdm commented Jun 14, 2017 •

edited

Loading

MortimerGoro commented Aug 1, 2017 •

edited

Loading