Hook up new openxr context menu gesture to embedder context menu machinery

[Manishearth]

Based on #26043

Fixes #25797, #26057

servo/webxr#144 needs to land first

Currently when exited the Servo window is blurred, apparently we need to call Window.Activate on it.

r? @jdm

cc @paulrouget

[gterzian]

If I may make a general comment on the design: while this undoubtedly "works", could it perhaps contribute to what was already noted to be a somewhat complex situation on the main-thread?

Even if webxr is running off the main thread, could there be a simpler way to express this new API?

I had a look at this PR, and the related one in WebXR, and if I understand it correctly this is essentially about calling quit on the XR device if the user chooses to? And the other side seems to be about enabling the XR device to use the embedder API to show a context menu.

So in an ideal world, would it not be great if OpenXR could just call embedder.show_context_menu(items), and inside on_context_menu_closed, the embedder could just call device.quit(if that was the chosen action? Or at least something like device.handle_context_menu_choice(choice))?

Then those API could be implemented with messaging for off-the-main-thread Xr, and on the main-thread be simply synchronous call(it looks like showing the menu is natively async already?)?

What I find somewhat harder to reason about the current design is how polling the result of the menu choice is tied to waiting on a frame inside Xr, and how if it's running on the main-thread the messaging is actually to the same event-loop(with the polling happening in a kind of sub-event-loop).

Obviously, since the embedder doesn't even have a handle to Webxr at this point(only the compositor has), what I'm proposing would require a larger refactoring, so it's perhaps more suited for a follow-up. I do think it would be a good step towards simplifying and disentangling the various parts on the main-thread(or even off-main thread) embedder/device interactions.

Note: when I write "the embedder" above, I mean ServoGlue.

[gterzian]

I think this could be a threaded channel. The EmbedderProxy itself also uses a threaded channel internally.

[paulrouget]

@gterzian The API change has landed as part of #26043

what I'm proposing would require a larger refactoring, so it's perhaps more suited for a follow-up.

At the moment, we're just following the current embedder API structure. But I'd be interested to know what type of changes you'd like to see in a future embedder API. Feel free to file another issue to continue the conversation.

[gterzian]

The API change has landed as part of #26043

Yes, and I'm mainly referring to the additional integration taking place here, where the result of menu choice is handled by polling the ContextMenuFuture, which in practice will be done inside wait_for_animation_frame.

[Manishearth]

So in an ideal world, would it not be great if OpenXR could just call embedder.show_context_menu(items), and inside on_context_menu_closed, the embedder could just call device.quit(if that was the chosen action? Or at least something like device.handle_context_menu_choice(choice))?

Yes, but I don't want the webxr crate to know anything about the embedder.

What I find somewhat harder to reason about the current design is how polling the result of the menu choice is tied to waiting on a frame inside Xr, and how if it's running on the main-thread the messaging is actually to the same event-loop(with the polling happening in a kind of sub-event-loop).

The openxr device is not running on the main thread. It's tied to waiting on a frame just because that's where all the other events are being handled, I needed to pick some point on the openxr thread to poll, so I picked this one, since all the other events are being handled there, and in my eye this is basically just another device event that we're "helping".

---

I thought of the other way of doing this: Instead of polling, we can give the device access to the Quitter object (which we're not using here). However, this doesn't let the device know when the menu was closed, since it needs to start listening to the menu again, and start feeding input data back to the session. I could add more functionality to the quitter and make it into a remote-like object, but that seemed equally convoluted.

[gterzian]

Yes, but I don't want the webxr crate to know anything about the embedder.

I agree, I think the current structure wrapping the EmbedderProxy looks like the right one.

The openxr device is not running on the main thread.

Ok, thanks for pointing this out, just had a better look at the code and saw the use of xr.spawn...

I needed to pick some point on the openxr thread to poll

Ok I see.

Just a question, I now see there is actually a SessionMsg::Quit, so why not have the embedder send that message to be handled by open xr on it's own thread? Is it because it's hard to tie the menu call to a particular session?

Could you not add "something wrapping a sender of a SessionMsg::Quit" to the ShowContextMenu message, so that the embedder could just call quit on it and it would internally send the session message?

That seems like a potential way to separate this from the animation frame?

[gterzian]

Ok so I've taken a better look at the code, and I think this is is a concrete proposal that would address my concerns with regards to tying the menu to the animation frame flow:

1. OpenXrDiscovery owns a EmbedderProxy.
2. SessionBuilder.spawn takes a new first argument, an EmbedderProxy.
3. DeviceAPI has a new set_context_menu_provider(Box< dyn ContextMenuProvider>) method.
4. Inside SessionThread::new, use the EmbedderProxy and the Sender<SessionMsg> to create a ContextMenuProvider, and pass it to the device by calling set_context_menu_provider.

ContextMenuProvider looks like:

pub trait ContextMenuProvider: Send {
    /// Called from the session thread.
    fn open_context_menu(&self);
    /// Called from the main thread once the result have been received.
    /// Maybe separate the two in two different traits?
    fn handle_selection(&self, result: usize);
}

Then, when the device calls open_context_menu, it doesn't need to return anything. Instead, it internally sends a ShowContextMenu(ContextMenuProvider, Vec<String>), with the provider actually being a clone of the provider.

Then, when the embedder gets the result via on_context_menu_closed, it takes the provider and in the case of the result being ContextMenuResult::Selected(selection) it calls provider.exit_session(selection). This then internally sends a SessionMsg::Quit, if selection is 0.

Then, when the session thread receives the SessionMsg::Quit message, it quits the device.

So, not saying this is super simple either, however it does address my concern of tying the messaging into the frame loop.

[gterzian]

Yes, but I don't want the webxr crate to know anything about the embedder.

Ah yes, so then my suggestion above would have to be updated to not passing the EmbedderProxy, but rather a trait like:

pub trait ContextMenuProvider: Send {
    /// Called from the session thread.
    fn open_context_menu(&self, Box<dyn ContextMenuCallback>);
}

pub trait ContextMenuCallback {
    /// Called by the embedder,
    /// This would internally send the `SessionMsg::Quit` based on the result passed.
    fn handle_result(&self, result: ContextMenuResult)
}

and to create the ContextMenuCallback you would have to store the Sender<SessionMsg> on the device...

[Manishearth]

why not have the embedder send that message to be handled by open xr on it's own thread?

the embedder doesn't have an easy way to get a handle to the openxr device's sender

Just a question, I now see there is actually a SessionMsg::Quit, so why not have the embedder send that message to be handled by open xr on it's own thread? Is it because it's hard to tie the menu call to a particular session?

Yes.

We could pass the quitter down to the embedder via the ShowContextMenu trait, and add another function to it that allows for cancelling context menus. I didn't want to make the core webxr API dependent on openxr-specific things though.

Like, I considered exactly this design, especially since the quitter exists already;

---

Basically, I've considered this design, but I'm unconvinced that it is better -- both are kinda gnarly. I guess having one trait is better than having two. Thoughts?

[gterzian]

@Manishearth Thanks for elaborating, now that I realize the XR frame loop really does need to keep polling frames while the session is running, I think my concern about polling for the result of the menu as part of the frame loop seems less relevant. I'd say this PR can also FIX #26057

Looks good to me, perhaps you could still replace the IPC channel with a threaded one if we're indeed not crossing a process boundary?

[Manishearth]

@gterzian we're not crossing a process boundary now, unsure if this might ever change.

Still, I'm open to the Quitter-based design as well, do you still prefer it? It might be cleaner.

[gterzian]

we're not crossing a process boundary now, unsure if this might ever change.

Ok :)

Still, I'm open to the Quitter-based design as well, do you still prefer it? It might be cleaner.

My main concern was tying the polling of the menu result into the frame loop, since that concern doesn't apply to the Xr frame loop, I'm much more ambivalent.

[Manishearth]

Cool, then in the interest of laziness I'll keep it the way it is.

[Manishearth]

@bors-servo r=jdm

[bors-servo]

📌 Commit 3aba88b has been approved by jdm

[jdm]

error: unused imports: `IpcReceiver`, `self`
  --> ports\libsimpleservo\api\src\lib.rs:16:24
   |
16 | use ipc_channel::ipc::{self, IpcReceiver, IpcSender};
   |                        ^^^^  ^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

[Manishearth]

@bors-servo r=jdm

[bors-servo]

📌 Commit 1ba606b has been approved by jdm

[bors-servo]

⌛ Testing commit 1ba606b with merge 1c9191f...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

• #23290

[bors-servo]

⌛ Testing commit 1ba606b with merge 7e31a66...

[bors-servo]

💔 Test failed - status-taskcluster

[Manishearth]

@bors-servo retry

• whitespace and linebreak failures

[bors-servo]

⌛ Testing commit 1ba606b with merge b3c12ad...

[bors-servo]

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing b3c12ad to master...