Auto merge of #25869 - iulianR:issue-23057-tinifiledialogs, r=jdm

Move tinyfiledialog call from script to embedder

<!-- Please describe your changes on the following line: -->

PR is based on work started in #23651. I rebased on top of master and addressed review comments. Handling the `PromptPermission` message in `libsimpleservo` is probably not ideal. Looking forward to make more changes, just let me know how I should proceed.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23057 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Cargo.lock

@@ -4762,7 +4762,6 @@ dependencies = [
  "swapper",
  "tendril",
  "time",
- "tinyfiledialogs",
  "unicode-bidi",
  "unicode-segmentation",
  "url",

components/embedder_traits/lib.rs

@@ -185,6 +185,8 @@ pub enum EmbedderMsg {
     GetSelectedBluetoothDevice(Vec<String>, IpcSender<Option<String>>),
     /// Open file dialog to select files. Set boolean flag to true allows to select multiple files.
     SelectFiles(Vec<FilterPattern>, bool, IpcSender<Option<Vec<String>>>),
+    /// Open interface to request permission specified by prompt.
+    PromptPermission(PermissionPrompt, IpcSender<PermissionRequest>),
     /// Request to present an IME to the user when an editable element is focused.
     ShowIME(InputMethodType),
     /// Request to hide the IME when the editable element is blurred.
@@ -222,6 +224,7 @@ impl Debug for EmbedderMsg {
             EmbedderMsg::Panic(..) => write!(f, "Panic"),
             EmbedderMsg::GetSelectedBluetoothDevice(..) => write!(f, "GetSelectedBluetoothDevice"),
             EmbedderMsg::SelectFiles(..) => write!(f, "SelectFiles"),
+            EmbedderMsg::PromptPermission(..) => write!(f, "PromptPermission"),
             EmbedderMsg::ShowIME(..) => write!(f, "ShowIME"),
             EmbedderMsg::HideIME => write!(f, "HideIME"),
             EmbedderMsg::Shutdown => write!(f, "Shutdown"),
@@ -299,3 +302,33 @@ pub enum MediaSessionEvent {
     /// Indicates that the position state is set.
     SetPositionState(MediaPositionState),
 }
+
+/// Enum with variants that match the DOM PermissionName enum
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub enum PermissionName {
+    Geolocation,
+    Notifications,
+    Push,
+    Midi,
+    Camera,
+    Microphone,
+    Speaker,
+    DeviceInfo,
+    BackgroundSync,
+    Bluetooth,
+    PersistentStorage,
+}
+
+/// Information required to display a permission prompt
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub enum PermissionPrompt {
+    Insecure(PermissionName),
+    Request(PermissionName),
+}
+
+/// Status for prompting user for permission.
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub enum PermissionRequest {
+    Granted,
+    Denied,
+}

components/script/Cargo.toml

@@ -29,9 +29,6 @@ phf_codegen = "0.8"
 phf_shared = "0.8"
 serde_json = "1.0"
 
-[target.'cfg(target_os = "linux")'.dependencies]
-tinyfiledialogs = "3.0"
-
 [dependencies]
 accountable-refcell = {version = "0.2.0", optional = true}
 app_units = "0.7"

components/script/dom/globalscope.rs

@@ -5,6 +5,7 @@
 use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::codegen::Bindings::BroadcastChannelBinding::BroadcastChannelMethods;
 use crate::dom::bindings::codegen::Bindings::EventSourceBinding::EventSourceBinding::EventSourceMethods;
+use crate::dom::bindings::codegen::Bindings::PermissionStatusBinding::PermissionState;
 use crate::dom::bindings::codegen::Bindings::VoidFunctionBinding::VoidFunction;
 use crate::dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
 use crate::dom::bindings::codegen::Bindings::WorkerGlobalScopeBinding::WorkerGlobalScopeMethods;
@@ -60,6 +61,7 @@ use crate::timers::{OneshotTimers, TimerCallback};
 use content_security_policy::CspList;
 use devtools_traits::{PageError, ScriptToDevtoolsControlMsg};
 use dom_struct::dom_struct;
+use embedder_traits::EmbedderMsg;
 use ipc_channel::ipc::{self, IpcSender};
 use ipc_channel::router::ROUTER;
 use js::glue::{IsWrapper, UnwrapObjectDynamic};
@@ -181,6 +183,9 @@ pub struct GlobalScope {
     /// The origin of the globalscope
     origin: MutableOrigin,
 
+    /// A map for storing the previous permission state read results.
+    permission_state_invocation_results: DomRefCell<HashMap<String, PermissionState>>,
+
     /// The microtask queue associated with this global.
     ///
     /// It is refcounted because windows in the same script thread share the
@@ -567,6 +572,7 @@ impl GlobalScope {
             timers: OneshotTimers::new(scheduler_chan),
             init_timers: Default::default(),
             origin,
+            permission_state_invocation_results: Default::default(),
             microtask_queue,
             list_auto_close_worker: Default::default(),
             event_source_tracker: DOMTracker::new(),
@@ -1693,6 +1699,12 @@ impl GlobalScope {
         }
     }
 
+    pub fn permission_state_invocation_results(
+        &self,
+    ) -> &DomRefCell<HashMap<String, PermissionState>> {
+        &self.permission_state_invocation_results
+    }
+
     pub fn track_worker(&self, closing_worker: Arc<AtomicBool>) {
         self.list_auto_close_worker
             .borrow_mut()
@@ -1898,6 +1910,14 @@ impl GlobalScope {
         &self.script_to_constellation_chan
     }
 
+    pub fn send_to_embedder(&self, msg: EmbedderMsg) {
+        self.send_to_constellation(ScriptMsg::ForwardToEmbedder(msg));
+    }
+
+    pub fn send_to_constellation(&self, msg: ScriptMsg) {
+        self.script_to_constellation_chan().send(msg).unwrap();
+    }
+
     pub fn scheduler_chan(&self) -> &IpcSender<TimerSchedulerMsg> {
         &self.scheduler_chan
     }

components/script/dom/permissions.rs

@@ -19,19 +19,13 @@ use crate::dom::promise::Promise;
 use crate::realms::{AlreadyInRealm, InRealm};
 use crate::script_runtime::JSContext;
 use dom_struct::dom_struct;
+use embedder_traits::{self, EmbedderMsg, PermissionPrompt, PermissionRequest};
+use ipc_channel::ipc;
 use js::conversions::ConversionResult;
 use js::jsapi::JSObject;
 use js::jsval::{ObjectValue, UndefinedValue};
 use servo_config::pref;
 use std::rc::Rc;
-#[cfg(target_os = "linux")]
-use tinyfiledialogs::{self, MessageBoxIcon, YesNo};
-
-#[cfg(target_os = "linux")]
-const DIALOG_TITLE: &'static str = "Permission request dialog";
-const NONSECURE_DIALOG_MESSAGE: &'static str = "feature is only safe to use in secure context,\
- but servo can't guarantee\n that the current context is secure. Do you want to proceed and grant permission?";
-const REQUEST_DIALOG_MESSAGE: &'static str = "Do you want to grant permission for";
 
 pub trait PermissionAlgorithm {
     type Descriptor;
@@ -143,7 +137,6 @@ impl Permissions {
                         // (Revoke) Step 3.
                         let globalscope = self.global();
                         globalscope
-                            .as_window()
                             .permission_state_invocation_results()
                             .borrow_mut()
                             .remove(&root_desc.name.to_string());
@@ -176,7 +169,6 @@ impl Permissions {
                         // (Revoke) Step 3.
                         let globalscope = self.global();
                         globalscope
-                            .as_window()
                             .permission_state_invocation_results()
                             .borrow_mut()
                             .remove(&root_desc.name.to_string());
@@ -259,17 +251,13 @@ impl PermissionAlgorithm for Permissions {
             // Step 3.
             PermissionState::Prompt => {
                 let perm_name = status.get_query();
-
-                let globalscope = GlobalScope::current().expect("No current global object");
+                let prompt =
+                    PermissionPrompt::Request(embedder_traits::PermissionName::from(perm_name));
 
                 // https://w3c.github.io/permissions/#request-permission-to-use (Step 3 - 4)
-                let state = prompt_user(
-                    &format!("{} {} ?", REQUEST_DIALOG_MESSAGE, perm_name.clone()),
-                    globalscope.is_headless(),
-                );
-
+                let globalscope = GlobalScope::current().expect("No current global object");
+                let state = prompt_user_from_embedder(prompt, &globalscope);
                 globalscope
-                    .as_window()
                     .permission_state_invocation_results()
                     .borrow_mut()
                     .insert(perm_name.to_string(), state);
@@ -292,7 +280,7 @@ pub fn get_descriptor_permission_state(
     env_settings_obj: Option<&GlobalScope>,
 ) -> PermissionState {
     // Step 1.
-    let settings = match env_settings_obj {
+    let globalscope = match env_settings_obj {
         Some(env_settings_obj) => DomRoot::from_ref(env_settings_obj),
         None => GlobalScope::current().expect("No current global object"),
     };
@@ -308,22 +296,20 @@ pub fn get_descriptor_permission_state(
         if pref!(dom.permissions.testing.allowed_in_nonsecure_contexts) {
             PermissionState::Granted
         } else {
-            settings
-                .as_window()
+            globalscope
                 .permission_state_invocation_results()
                 .borrow_mut()
                 .remove(&permission_name.to_string());
 
-            prompt_user(
-                &format!("The {} {}", permission_name, NONSECURE_DIALOG_MESSAGE),
-                settings.is_headless(),
+            prompt_user_from_embedder(
+                PermissionPrompt::Insecure(embedder_traits::PermissionName::from(permission_name)),
+                &globalscope,
             )
         }
     };
 
     // Step 3.
-    if let Some(prev_result) = settings
-        .as_window()
+    if let Some(prev_result) = globalscope
         .permission_state_invocation_results()
         .borrow()
         .get(&permission_name.to_string())
@@ -332,8 +318,7 @@ pub fn get_descriptor_permission_state(
     }
 
     // Store the invocation result
-    settings
-        .as_window()
+    globalscope
         .permission_state_invocation_results()
         .borrow_mut()
         .insert(permission_name.to_string(), state);
@@ -342,28 +327,6 @@ pub fn get_descriptor_permission_state(
     state
 }
 
-#[cfg(target_os = "linux")]
-fn prompt_user(message: &str, headless: bool) -> PermissionState {
-    if headless {
-        return PermissionState::Denied;
-    }
-    match tinyfiledialogs::message_box_yes_no(
-        DIALOG_TITLE,
-        message,
-        MessageBoxIcon::Question,
-        YesNo::No,
-    ) {
-        YesNo::Yes => PermissionState::Granted,
-        YesNo::No => PermissionState::Denied,
-    }
-}
-
-#[cfg(not(target_os = "linux"))]
-fn prompt_user(_message: &str, _headless: bool) -> PermissionState {
-    // TODO popup only supported on linux
-    PermissionState::Denied
-}
-
 // https://w3c.github.io/permissions/#allowed-in-non-secure-contexts
 fn allowed_in_nonsecure_contexts(permission_name: &PermissionName) -> bool {
     match *permission_name {
@@ -391,3 +354,40 @@ fn allowed_in_nonsecure_contexts(permission_name: &PermissionName) -> bool {
         PermissionName::Persistent_storage => false,
     }
 }
+
+fn prompt_user_from_embedder(prompt: PermissionPrompt, gs: &GlobalScope) -> PermissionState {
+    let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel!");
+    gs.send_to_embedder(EmbedderMsg::PromptPermission(prompt, sender));
+
+    match receiver.recv() {
+        Ok(PermissionRequest::Granted) => PermissionState::Granted,
+        Ok(PermissionRequest::Denied) => PermissionState::Denied,
+        Err(e) => {
+            warn!(
+                "Failed to receive permission state from embedder ({:?}).",
+                e
+            );
+            PermissionState::Denied
+        },
+    }
+}
+
+impl From<PermissionName> for embedder_traits::PermissionName {
+    fn from(permission_name: PermissionName) -> Self {
+        match permission_name {
+            PermissionName::Geolocation => embedder_traits::PermissionName::Geolocation,
+            PermissionName::Notifications => embedder_traits::PermissionName::Notifications,
+            PermissionName::Push => embedder_traits::PermissionName::Push,
+            PermissionName::Midi => embedder_traits::PermissionName::Midi,
+            PermissionName::Camera => embedder_traits::PermissionName::Camera,
+            PermissionName::Microphone => embedder_traits::PermissionName::Microphone,
+            PermissionName::Speaker => embedder_traits::PermissionName::Speaker,
+            PermissionName::Device_info => embedder_traits::PermissionName::DeviceInfo,
+            PermissionName::Background_sync => embedder_traits::PermissionName::BackgroundSync,
+            PermissionName::Bluetooth => embedder_traits::PermissionName::Bluetooth,
+            PermissionName::Persistent_storage => {
+                embedder_traits::PermissionName::PersistentStorage
+            },
+        }
+    }
+}

components/script/dom/window.rs

@@ -8,7 +8,6 @@ use crate::dom::bindings::codegen::Bindings::DocumentBinding::{
 };
 use crate::dom::bindings::codegen::Bindings::HistoryBinding::HistoryBinding::HistoryMethods;
 use crate::dom::bindings::codegen::Bindings::MediaQueryListBinding::MediaQueryListBinding::MediaQueryListMethods;
-use crate::dom::bindings::codegen::Bindings::PermissionStatusBinding::PermissionState;
 use crate::dom::bindings::codegen::Bindings::RequestBinding::RequestInit;
 use crate::dom::bindings::codegen::Bindings::VoidFunctionBinding::VoidFunction;
 use crate::dom::bindings::codegen::Bindings::WindowBinding::{
@@ -275,9 +274,6 @@ pub struct Window {
     #[ignore_malloc_size_of = "defined in webxr"]
     webxr_registry: webxr_api::Registry,
 
-    /// A map for storing the previous permission state read results.
-    permission_state_invocation_results: DomRefCell<HashMap<String, PermissionState>>,
-
     /// All of the elements that have an outstanding image request that was
     /// initiated by layout during a reflow. They are stored in the script thread
     /// to ensure that the element can be marked dirty when the image data becomes
@@ -476,12 +472,6 @@ impl Window {
         Worklet::new(self, WorkletGlobalScopeType::Paint)
     }
 
-    pub fn permission_state_invocation_results(
-        &self,
-    ) -> &DomRefCell<HashMap<String, PermissionState>> {
-        &self.permission_state_invocation_results
-    }
-
     pub fn pending_image_notification(&self, response: PendingImageResponse) {
         //XXXjdm could be more efficient to send the responses to the layout thread,
         //       rather than making the layout thread talk to the image cache to
@@ -2333,7 +2323,6 @@ impl Window {
             webgl_chan,
             webvr_chan,
             webxr_registry,
-            permission_state_invocation_results: Default::default(),
             pending_layout_images: Default::default(),
             unminified_js_dir: Default::default(),
             test_worklet: Default::default(),

ports/glutin/browser.rs

@@ -7,7 +7,10 @@ use crate::window_trait::{WindowPortsMethods, LINE_HEIGHT};
 use euclid::{Point2D, Vector2D};
 use keyboard_types::{Key, KeyboardEvent, Modifiers, ShortcutMatcher};
 use servo::compositing::windowing::{WebRenderDebugOption, WindowEvent};
-use servo::embedder_traits::{EmbedderMsg, FilterPattern, PromptDefinition, PromptOrigin, PromptResult};
+use servo::embedder_traits::{
+    EmbedderMsg, FilterPattern, PermissionRequest, PromptDefinition, PromptOrigin, PromptResult,
+    PermissionPrompt,
+};
 use servo::msg::constellation_msg::TopLevelBrowsingContextId as BrowserId;
 use servo::msg::constellation_msg::TraversalDirection;
 use servo::net_traits::pub_domains::is_reg_domain;
@@ -491,6 +494,10 @@ where
                         self.event_queue.push(WindowEvent::SendError(None, reason));
                     };
                 },
+                EmbedderMsg::PromptPermission(prompt, sender) => {
+                    let permission_state = prompt_user(prompt);
+                    let _ = sender.send(permission_state);
+                }
                 EmbedderMsg::ShowIME(_kind) => {
                     debug!("ShowIME received");
                 },
@@ -513,6 +520,42 @@ where
     }
 }
 
+#[cfg(target_os = "linux")]
+fn prompt_user(prompt: PermissionPrompt) -> PermissionRequest {
+    if opts::get().headless {
+        return PermissionRequest::Denied;
+    }
+
+    let message = match prompt {
+        PermissionPrompt::Request(permission_name) => {
+            format!("Do you want to grant permission for {:?}?", permission_name)
+        },
+        PermissionPrompt::Insecure(permission_name) => {
+            format!(
+                "The {:?} feature is only safe to use in secure context, but servo can't guarantee\n\
+                that the current context is secure. Do you want to proceed and grant permission?",
+                permission_name
+            )
+        },
+    };
+
+    match tinyfiledialogs::message_box_yes_no(
+        "Permission request dialog",
+        &message,
+        MessageBoxIcon::Question,
+        YesNo::No,
+    ) {
+        YesNo::Yes => PermissionRequest::Granted,
+        YesNo::No => PermissionRequest::Denied,
+    }
+}
+
+#[cfg(not(target_os = "linux"))]
+fn prompt_user(_prompt: PermissionPrompt) -> PermissionRequest {
+    // TODO popup only supported on linux
+    PermissionRequest::Denied
+}
+
 #[cfg(target_os = "linux")]
 fn platform_get_selected_devices(devices: Vec<String>) -> Option<String> {
     let picker_name = "Choose a device";

ports/libsimpleservo/api/src/lib.rs

@@ -7,7 +7,9 @@ extern crate log;
 
 pub mod gl_glue;
 
-pub use servo::embedder_traits::{MediaSessionPlaybackState, PromptResult};
+pub use servo::embedder_traits::{
+    MediaSessionPlaybackState, PermissionPrompt, PermissionRequest, PromptResult,
+};
 pub use servo::script_traits::{MediaSessionActionType, MouseButton};
 
 use getopts::Options;
@@ -615,6 +617,29 @@ impl ServoGlue {
                 EmbedderMsg::Shutdown => {
                     self.callbacks.host_callbacks.on_shutdown_complete();
                 },
+                EmbedderMsg::PromptPermission(prompt, sender) => {
+                    let message = match prompt {
+                        PermissionPrompt::Request(permission_name) => {
+                            format!("Do you want to grant permission for {:?}?", permission_name)
+                        },
+                        PermissionPrompt::Insecure(permission_name) => {
+                            format!(
+                                "The {:?} feature is only safe to use in secure context, but servo can't guarantee\n\
+                                that the current context is secure. Do you want to proceed and grant permission?",
+                                permission_name
+                            )
+                        },
+                    };
+
+                    let result = match self.callbacks.host_callbacks.prompt_yes_no(message, true) {
+                        PromptResult::Primary => PermissionRequest::Granted,
+                        PromptResult::Secondary | PromptResult::Dismissed => {
+                            PermissionRequest::Denied
+                        },
+                    };
+
+                    let _ = sender.send(result);
+                },
                 EmbedderMsg::ShowIME(..) => {
                     self.callbacks.host_callbacks.on_ime_state_changed(true);
                 },

tests/wpt/metadata/permissions/idlharness.any.js.ini

@@ -1,5 +1,9 @@
 [idlharness.any.worker.html]
-  expected: CRASH
+  [Permissions interface: operation query(object)]
+    expected: FAIL
+
+  [Permissions interface: calling query(object) on navigator.permissions with too few arguments must throw TypeError]
+    expected: FAIL
 
 [idlharness.any.html]
   [Permissions interface: operation query(object)]