Issue 23009/separate angle and disable vsync

[glowe]

The --angle and --disable-vsync options were declared as global options, but only used in the Glutin embedding for desktop builds. Moving them to the Glutin embedding code makes them easier to update in the future.

I modified opts::from_cmdline_args to accept a getopts::Options (as prescribed in the issue) and augmented opts::ArgumentParsingResult to include an opts::Matches and content-process String when appropriate. I could use some feedback on this last bit. I could have changed the function to return opts::Matches and have the embedding code look for the presence of content-process, but I felt that the approach I went with was closer to the original design.

The other aspect I'm not sure about is moving disable-vsync from a global debug option to a plain embedder option. This changes the command line interface for glutin, which is maybe bad. However I wasn't sure whether it was worth preserving the original behavior given the complexity of injecting debug options into opts::from_cmdline_args.

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes partially fix #23009 – there are 2 more options to deal with, but I'm not sure if we should handle them yet.
• These changes do not require tests because this is a refactoring and I'm hoping that the existing tests cover these changes.

r? @jdm

[jdm]

I like these changes, and I appreciate the care you have taken in describing your reasoning for them! I think moving the disable-vsync flag to be a non-debug option is fine in this case. I've found several other flags that are glutin-only that we can convert as well: device_pixels_per_px, clean_shutdown, use_msaa, and no_native_titlebar.

[glowe]

I extracted clean_shutdown, use_msaa, and no_native_titlebar. The device_pixels_per_px is a bit more difficult, because it's used in the Constellation pipeline:

servo/components/constellation/pipeline.rs

Line 612 in dc8be8f

self.opts.device_pixels_per_px,

[jdm]

We can file a followup to remove the use from constellation by passing it as an argument to the constructor instead, if you'd like.

[jdm]

This looks great!

[glowe]

We can file a followup to remove the use from constellation by passing it as an argument to the constructor instead, if you'd like.

We can definitely make that change in this PR. However, I suspect that's going to force a modification of servo::Servo::new, which would be a public interface change. If so, does that mean we need to bump the version of libservo?

[jdm]

libservo isn't published anywhere so we don't need to follow any semver rules.

[jdm]

This will require updating

servo/ports/libmlservo/src/lib.rs

Line 193 in 6a54d60

simpleservo::init(opts, gl.gl_wrapper, wakeup, callbacks).expect("error initializing Servo");

and

servo/ports/libsimpleservo/jniapi/src/lib.rs

Line 117 in 6a54d60

simpleservo::init(opts, egl_init.gl_wrapper, wakeup, callbacks)

and

servo/ports/libsimpleservo/capi/src/lib.rs

Line 320 in 6a54d60

simpleservo::init(opts, gl, wakeup, callbacks).unwrap();

.

However, I just noticed

servo/ports/libsimpleservo/api/src/lib.rs

Line 49 in 6a54d60

pub density: f32,

which is the value that is consulted when device_pixels_per_px is None in this code. So I think we don't actually need to add this new argument; the device_pixels_per_px value is an override that is only meaningful in the glutin port, and the other ports already have a supported API for reporting the DPI factor, so we can revert this change to the init method.

[glowe]

Thanks for catching this. Removed.

[jdm]

@bors-servo r+
Looks great! Thanks for tackling this work!

[bors-servo]

📌 Commit bf47d85 has been approved by jdm

[glowe]

Looks great! Thanks for tackling this work!

Thank you for your guidance. It was fun!

[bors-servo]

⌛ Testing commit bf47d85 with merge 0b02cd3...

[bors-servo]

💔 Test failed - status-taskcluster

[glowe]

💔 Test failed - status-taskcluster

Looks like these errors are caused by me failing to update some things in libsimpleservo, however mach build -d built cleanly on my machine. I assume I didn't catch this issue, because mach build -d doesn't compile this crate? Is there an alternate invocation I can run that would catch these types of errors before submitting again?

[CYBAI]

@glowe I guess you're using macOS? maybe it's easier to fix them and push to the PR and ask homu to try 🤔?

error[E0433]: failed to resolve: use of undeclared type or module `Options`
   --> ports/libsimpleservo/api/src/lib.rs:173:33
    |
173 |         opts::from_cmdline_args(Options::new(), &args);
    |                                 ^^^^^^^ use of undeclared type or module `Options`

error[E0061]: this function takes 3 parameters but 2 parameters were supplied
   --> ports/libsimpleservo/api/src/lib.rs:207:17
    |
207 |     let servo = Servo::new(embedder_callbacks, window_callbacks.clone());
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 3 parameters

error: aborting due to 2 previous errors

[jdm]

mach build --libsimpleservo is what you're looking for.

[glowe]

Thanks for the help again. I believe I've addressed the libsimpleservo build errors.

[jdm]

@bors-servo r+
Thanks!

[bors-servo]

📌 Commit 0ee3004 has been approved by jdm

[bors-servo]

⌛ Testing commit 0ee3004 with merge 2ad6e94...

[bors-servo]

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 2ad6e94 to master...