Auto merge of #20578 - kwonoj:refactor-bluetooth-manager, r=paulrouget

refactor(bluetooth): removes direct ui invocation from bluetooth thread

<!-- Please describe your changes on the following line: -->
Relates to #20429.

This PR is mostly blocked by https://github.com/servo/servo/pull/20480, cause both are nearly identical change in crux and based on review of prior changes this PR need to be updated. Just didn't want assigned issue hanging without visible progresses, so prepped PR to followup further.

(mostly copy-paste from pair PR)

Probably point of review / need to be updated are

1. Communication between components
Currently it's wired as like below:
```
+----------------+                    +---------------+              +---------+
|                |  constellationMsg  |               | embedderMsg  |         |
| bluetooth_thread +----------------->+ constellation +------------->+ browser |
|                |                    |               |              |         |
+-----+----------+                    +---------------+              +----+----+
      ^                                                                   |
      +-------------------------------------------------------------------+
                                device: String
```
- is this feasible approach?
- does organization of message / fn (where to put consteallation / embedder msg & fns) are legit?

2. Invoking tfd in a separate thread
- This was mainly to align behavior to previous implentation, where tfd was invoked inside of filemanager_thread so does not block main. It may possibly just let block instead.

and of course, a lot of other codes need to follow better practices.

---
<!-- 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
- [ ] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20429 (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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20578)
<!-- Reviewable:end -->

Cargo.lock

@@ -179,11 +179,12 @@ version = "0.0.1"
 dependencies = [
  "bitflags 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "bluetooth_traits 0.0.1",
+ "compositing 0.0.1",
  "device 0.0.1 (git+https://github.com/servo/devices)",
  "ipc-channel 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "servo_config 0.0.1",
  "servo_rand 0.0.1",
- "tinyfiledialogs 3.3.5 (registry+https://github.com/rust-lang/crates.io-index)",
  "uuid 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 

components/bluetooth/Cargo.toml

@@ -12,11 +12,10 @@ path = "lib.rs"
 [dependencies]
 bitflags = "1.0"
 bluetooth_traits = {path = "../bluetooth_traits"}
+compositing = {path = "../compositing"}
 device = {git = "https://github.com/servo/devices", features = ["bluetooth-test"]}
 ipc-channel = "0.10"
+log = "0.4"
 servo_config = {path = "../config"}
 servo_rand = {path = "../rand"}
 uuid = {version = "0.6", features = ["v4"]}
-
-[target.'cfg(target_os = "linux")'.dependencies]
-tinyfiledialogs = "3.0"

components/bluetooth/lib.rs

@@ -5,12 +5,13 @@
 #[macro_use]
 extern crate bitflags;
 extern crate bluetooth_traits;
+extern crate compositing;
 extern crate device;
 extern crate ipc_channel;
+#[macro_use]
+extern crate log;
 extern crate servo_config;
 extern crate servo_rand;
-#[cfg(target_os = "linux")]
-extern crate tinyfiledialogs;
 extern crate uuid;
 
 pub mod test;
@@ -20,10 +21,10 @@ use bluetooth_traits::{BluetoothDeviceMsg, BluetoothRequest, BluetoothResponse,
 use bluetooth_traits::{BluetoothError, BluetoothResponseResult, BluetoothResult};
 use bluetooth_traits::blocklist::{uuid_is_blocklisted, Blocklist};
 use bluetooth_traits::scanfilter::{BluetoothScanfilter, BluetoothScanfilterSequence, RequestDeviceoptions};
+use compositing::compositor_thread::{EmbedderMsg, EmbedderProxy};
 use device::bluetooth::{BluetoothAdapter, BluetoothDevice, BluetoothGATTCharacteristic};
 use device::bluetooth::{BluetoothGATTDescriptor, BluetoothGATTService};
 use ipc_channel::ipc::{self, IpcReceiver, IpcSender};
-#[cfg(target_os = "linux")]
 use servo_config::opts;
 use servo_config::prefs::PREFS;
 use servo_rand::Rng;
@@ -39,12 +40,6 @@ const MAXIMUM_TRANSACTION_TIME: u8 = 30;
 const CONNECTION_TIMEOUT_MS: u64 = 1000;
 // The discovery session needs some time to find any nearby devices
 const DISCOVERY_TIMEOUT_MS: u64 = 1500;
-#[cfg(target_os = "linux")]
-const DIALOG_TITLE: &'static str = "Choose a device";
-#[cfg(target_os = "linux")]
-const DIALOG_COLUMN_ID: &'static str = "Id";
-#[cfg(target_os = "linux")]
-const DIALOG_COLUMN_NAME: &'static str = "Name";
 
 bitflags! {
     struct Flags: u32 {
@@ -69,11 +64,11 @@ macro_rules! return_if_cached(
 );
 
 pub trait BluetoothThreadFactory {
-    fn new() -> Self;
+    fn new(embedder_proxy: EmbedderProxy) -> Self;
 }
 
 impl BluetoothThreadFactory for IpcSender<BluetoothRequest> {
-    fn new() -> IpcSender<BluetoothRequest> {
+    fn new(embedder_proxy: EmbedderProxy) -> IpcSender<BluetoothRequest> {
         let (sender, receiver) = ipc::channel().unwrap();
         let adapter = if Some(true) == PREFS.get("dom.bluetooth.enabled").as_boolean() {
             BluetoothAdapter::init()
@@ -81,7 +76,7 @@ impl BluetoothThreadFactory for IpcSender<BluetoothRequest> {
             BluetoothAdapter::init_mock()
         }.ok();
         thread::Builder::new().name("BluetoothThread".to_owned()).spawn(move || {
-            BluetoothManager::new(receiver, adapter).start();
+            BluetoothManager::new(receiver, adapter, embedder_proxy).start();
         }).expect("Thread spawning failed");
         sender
     }
@@ -206,10 +201,13 @@ pub struct BluetoothManager {
     cached_characteristics: HashMap<String, BluetoothGATTCharacteristic>,
     cached_descriptors: HashMap<String, BluetoothGATTDescriptor>,
     allowed_services: HashMap<String, HashSet<String>>,
+    embedder_proxy: EmbedderProxy,
 }
 
 impl BluetoothManager {
-    pub fn new(receiver: IpcReceiver<BluetoothRequest>, adapter: Option<BluetoothAdapter>) -> BluetoothManager {
+    pub fn new(receiver: IpcReceiver<BluetoothRequest>,
+               adapter: Option<BluetoothAdapter>,
+               embedder_proxy: EmbedderProxy) -> BluetoothManager {
         BluetoothManager {
             receiver: receiver,
             adapter: adapter,
@@ -222,6 +220,7 @@ impl BluetoothManager {
             cached_characteristics: HashMap::new(),
             cached_descriptors: HashMap::new(),
             allowed_services: HashMap::new(),
+            embedder_proxy: embedder_proxy,
         }
     }
 
@@ -366,10 +365,9 @@ impl BluetoothManager {
         None
     }
 
-    #[cfg(target_os = "linux")]
     fn select_device(&mut self, devices: Vec<BluetoothDevice>, adapter: &BluetoothAdapter) -> Option<String> {
         if is_mock_adapter(adapter) || opts::get().headless {
-            for device in devices {
+            for device in &devices {
                 if let Ok(address) = device.get_address() {
                     return Some(address);
                 }
@@ -382,28 +380,18 @@ impl BluetoothManager {
             dialog_rows.extend_from_slice(&[device.get_address().unwrap_or("".to_string()),
                                             device.get_name().unwrap_or("".to_string())]);
         }
-        let dialog_rows: Vec<&str> = dialog_rows.iter()
-                                                .map(|s| s.as_ref())
-                                                .collect();
-        let dialog_rows: &[&str] = dialog_rows.as_slice();
 
-        if let Some(device) = tinyfiledialogs::list_dialog(DIALOG_TITLE,
-                                                           &[DIALOG_COLUMN_ID, DIALOG_COLUMN_NAME],
-                                                           Some(dialog_rows)) {
-            // The device string format will be "Address|Name". We need the first part of it.
-            return device.split("|").next().map(|s| s.to_string());
-        }
-        None
-    }
+        let (ipc_sender, ipc_receiver) = ipc::channel().expect("Failed to create IPC channel!");
+        let msg = EmbedderMsg::GetSelectedBluetoothDevice(dialog_rows, ipc_sender);
+        self.embedder_proxy.send(msg);
 
-    #[cfg(not(target_os = "linux"))]
-    fn select_device(&mut self, devices: Vec<BluetoothDevice>, _adapter: &BluetoothAdapter) -> Option<String> {
-        for device in devices {
-            if let Ok(address) = device.get_address() {
-                return Some(address);
+        match ipc_receiver.recv() {
+            Ok(result) => result,
+            Err(e) => {
+                warn!("Failed to receive files from embedder ({}).", e);
+                None
             }
         }
-        None
     }
 
     fn generate_device_id(&mut self) -> String {

components/compositing/compositor_thread.rs

@@ -141,6 +141,8 @@ pub enum EmbedderMsg {
     LoadComplete(TopLevelBrowsingContextId),
     /// A pipeline panicked. First string is the reason, second one is the backtrace.
     Panic(TopLevelBrowsingContextId, String, Option<String>),
+    /// Open dialog to select bluetooth device.
+    GetSelectedBluetoothDevice(Vec<String>, IpcSender<Option<String>>),
     /// Servo has shut down
     Shutdown,
 }
@@ -241,6 +243,7 @@ impl Debug for EmbedderMsg {
             EmbedderMsg::LoadStart(..) => write!(f, "LoadStart"),
             EmbedderMsg::LoadComplete(..) => write!(f, "LoadComplete"),
             EmbedderMsg::Panic(..) => write!(f, "Panic"),
+            EmbedderMsg::GetSelectedBluetoothDevice(..) => write!(f, "GetSelectedBluetoothDevice"),
             EmbedderMsg::Shutdown => write!(f, "Shutdown"),
         }
     }

components/servo/lib.rs

@@ -458,7 +458,7 @@ fn create_constellation(user_agent: Cow<'static, str>,
                         webrender_api_sender: webrender_api::RenderApiSender,
                         window_gl: Rc<gl::Gl>)
                         -> (Sender<ConstellationMsg>, SWManagerSenders) {
-    let bluetooth_thread: IpcSender<BluetoothRequest> = BluetoothThreadFactory::new();
+    let bluetooth_thread: IpcSender<BluetoothRequest> = BluetoothThreadFactory::new(embedder_proxy.clone());
 
     let (public_resource_threads, private_resource_threads) =
         new_resource_threads(user_agent,

ports/servo/browser.rs

@@ -7,6 +7,7 @@ use glutin_app::keyutils::{CMD_OR_CONTROL, CMD_OR_ALT};
 use glutin_app::window::{Window, LINE_HEIGHT};
 use servo::compositing::compositor_thread::EmbedderMsg;
 use servo::compositing::windowing::{WebRenderDebugOption, WindowEvent};
+use servo::ipc_channel::ipc::IpcSender;
 use servo::msg::constellation_msg::{Key, TopLevelBrowsingContextId as BrowserId};
 use servo::msg::constellation_msg::{KeyModifiers, KeyState, TraversalDirection};
 use servo::net_traits::pub_domains::is_reg_domain;
@@ -16,6 +17,8 @@ use servo::servo_url::ServoUrl;
 use servo::webrender_api::ScrollLocation;
 use std::mem;
 use std::rc::Rc;
+#[cfg(target_os = "linux")]
+use std::thread;
 use tinyfiledialogs;
 
 pub struct Browser {
@@ -288,6 +291,9 @@ impl Browser {
                     self.shutdown_requested = true;
                 },
                 EmbedderMsg::Panic(_browser_id, _reason, _backtrace) => {
+                },
+                EmbedderMsg::GetSelectedBluetoothDevice(devices, sender) => {
+                    platform_get_selected_devices(devices, sender);
                 }
             }
         }
@@ -295,6 +301,39 @@ impl Browser {
 
 }
 
+#[cfg(target_os = "linux")]
+fn platform_get_selected_devices(devices: Vec<String>, sender: IpcSender<Option<String>>) {
+    let picker_name = "Choose a device";
+
+    thread::Builder::new().name(picker_name.to_owned()).spawn(move || {
+        let dialog_rows: Vec<&str> = devices.iter()
+            .map(|s| s.as_ref())
+            .collect();
+        let dialog_rows: Option<&[&str]> = Some(dialog_rows.as_slice());
+
+        match tinyfiledialogs::list_dialog("Choose a device", &["Id", "Name"], dialog_rows) {
+            Some(device) => {
+                // The device string format will be "Address|Name". We need the first part of it.
+                let address = device.split("|").next().map(|s| s.to_string());
+                let _ = sender.send(address);
+            },
+            None => {
+                let _ = sender.send(None);
+            }
+        }
+    }).expect("Thread spawning failed");
+}
+
+#[cfg(not(target_os = "linux"))]
+fn platform_get_selected_devices(devices: Vec<String>, sender: IpcSender<Option<String>>) {
+    for device in devices {
+        if let Some(address) = device.split("|").next().map(|s| s.to_string()) {
+            let _ = sender.send(Some(address));
+        }
+    }
+    let _ = sender.send(None);
+}
+
 fn sanitize_url(request: &str) -> Option<ServoUrl> {
     let request = request.trim();
     ServoUrl::parse(&request).ok()