refactor(net): removes direct ui invocation from filemgr thread #20480

kwonoj · 2018-03-30T20:00:34Z

relates to #20428.

This PR tries to refactor net::filemanager_thread implementation, removes direct ui-related code invocation but ask constellation. I believe overall organization might need to be refactored still as I took my own liberty to wire up dots and dots between components, which could be non-recommended practices.

Probably point of review / need to be updated are

Communication between components
Currently it's wired as like below:

+----------------+                    +---------------+              +---------+
|                |  constellationMsg  |               | embedderMsg  |         |
| filemgr_thread +------------------->+ constellation +------------->+ browser |
|                |                    |               |              |         |
+-----+----------+                    +---------------+              +----+----+
      ^                                                                   |
      +-------------------------------------------------------------------+
                                filelist: Vec(String)

is this feasible approach?
does organization of message / fn (where to put consteallation / embedder msg & fns) are legit?

Removal of filemanger_thread::UIProvider

As filemanager_thread no longer need to aware actual ui, this PR removes UIProvider completely and let invoke tinyfiledialog directly in message listener. Maybe UIProvider itself still being needed?

Overall fn organization

To reduce duplicated code it takes single msg with boolean flag to distinguish selecting multiple files, may feasible / or better to create explicit paths between two.

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.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #20428 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____
Manually verified file picker <input type=files> can pick up files.

This change is

highfive · 2018-03-30T20:00:38Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@cbrewster: components/constellation/constellation.rs
@paulrouget: components/constellation/constellation.rs, components/servo/lib.rs, ports/servo/browser.rs, components/compositing/compositor_thread.rs
@fitzgen: components/script_traits/lib.rs
@KiChjang: components/net/Cargo.toml, components/net/filemanager_thread.rs, components/net/resource_thread.rs, components/net/lib.rs, components/net/tests/resource_thread.rs and 4 more

bors-servo · 2018-04-02T06:52:31Z

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

paulrouget · 2018-04-02T10:03:59Z

I see you call open_file_dialog from a different thread to not block the main thread. Does that work well on the 3 platforms? (I can try on Windows and Mac if that helps).

kwonoj · 2018-04-02T16:12:58Z

@paulrouget I just give a quick try (open tfd for single / multiple dialog, check if it unblocks main) on linux / windows / macOS, and all platform seems working as expected. (Does not block / no immediate panic or unexpected behavior).

paulrouget

Could we short-circuit the constellation and directly use the embedder channel (EmbedderProxy)?

paulrouget · 2018-04-03T06:40:13Z

components/compositing/compositor_thread.rs

@@ -141,6 +142,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 file dialog to select files


Can you add a comment about what that boolean is for?

paulrouget · 2018-04-03T06:40:13Z

components/net/filemanager_thread.rs

-                       ui: &UI)
-        where UI: UIProvider,
-    {
+    #[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))]


I don't think we have to make a distinction between the platforms here. Let send a message to the embedder no matter the platform, and let the embedder decide what to do depending on the platform.

paulrouget · 2018-04-03T06:40:13Z

components/constellation/constellation.rs

@@ -1076,6 +1077,9 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
            FromCompositorMsg::SetCursor(cursor) => {
                self.handle_set_cursor_msg(cursor)
            }
+            FromCompositorMsg::OpenFileSelectDialog(patterns, multiple_files, sender) => {


Having to use FromCompositorMsg is unfortunate. Maybe a Request::Script might be better? Or even its own type of request.

kwonoj · 2018-04-03T15:25:36Z

Could we short-circuit the constellation and directly use the embedder channel (EmbedderProxy)?

@paulrouget I haven't tried but it may possible, mostly I tried follow steps in original issue #20428 's network code should send a message to the constellation, which can talk to the embedder and receive the final selection to return to the network code to let constellation handles it. May I update PR as suggested?

paulrouget · 2018-04-03T16:13:31Z

So far, only the constellation talks to the embedder via EmbedderMsg. So maybe let's keep it the way you did.

kwonoj · 2018-04-03T18:10:48Z

@paulrouget appreciate for feedback, I tried to update PR as suggested

create FileManagerMsg and let it handled via Script::Request
move out platform-aware logic from filethread

If there are things need to be corrected, I'll update per suggestions.

paulrouget · 2018-04-10T04:52:43Z

Sorry. Was away for a while. I'll look at this again soon.

kwonoj · 2018-04-10T04:54:40Z

no worries. 👍

paulrouget · 2018-04-10T06:35:20Z

I had a quick chat with @cbrewster and we agree that it's fine if the script thread has access to the EmbedderProxy. We could short-circuit the constellation.

paulrouget

So I'd really like to see how that would work if we were to share the embedder proxy with the script thread. Sorry, changed my mind again :|

paulrouget · 2018-04-10T07:36:10Z

components/compositing/compositor_thread.rs

@@ -141,6 +142,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 file dialog to select files. Set boolean flag to true allows to select multiple files.
+    GetSelectedFiles(Vec<FilterPattern>, bool, IpcSender<Option<Vec<String>>>),


Maybe rename that to PickFiles, RequestFiles or SelectFiles?

paulrouget · 2018-04-10T07:36:10Z

components/constellation/constellation.rs

+                self.embedder_proxy.send(msg);
+            }
+        }
+    }


If we are forwarding a message, maybe we want to keep the same name.

Also, I talked to @cbrewster and we agree that maybe it's better to give access to the EmbedderProxy to each script thread.

paulrouget · 2018-04-10T07:36:10Z

ports/servo/browser.rs

+fn platform_get_selected_files(_patterns: Vec<FilterPattern>,
+                               _multiple_files: bool,
+                               sender: IpcSender<Option<Vec<String>>>) {
+    sender.send(None);


Maybe warn!("File picker not implemented") first?

kwonoj · 2018-04-10T14:16:14Z

I'll try to update short-curcuit as suggested. Due to personal schedules I may need some time to wrap things up, once done will ping via @-mention here.

kwonoj · 2018-04-14T21:18:19Z

@paulrouget Updated PR as suggested,

now filemanager_thread access to embedderProxy directly
refactored EmbedderMsg name as suggested.

paulrouget

This feels a lot better! I think we made the right decision to use the embedder proxy.

A few neats and then I think we good to go.

Thank you.

paulrouget · 2018-04-16T05:59:26Z

components/constellation/constellation.rs

@@ -546,7 +546,8 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
          STF: ScriptThreadFactory<Message=Message>
 {
    /// Create a new constellation thread.
-    pub fn start(state: InitialConstellationState) -> (Sender<FromCompositorMsg>, IpcSender<SWManagerMsg>) {
+    pub fn start(state: InitialConstellationState)
+        -> (Sender<FromCompositorMsg>, IpcSender<SWManagerMsg>) {


Unnecessary change.

paulrouget · 2018-04-16T05:59:26Z

components/net/filemanager_thread.rs

                thread::Builder::new().name("select files".to_owned()).spawn(move || {
-                    store.select_files(filter, sender, origin, opt_test_paths, ui);
+                    store.select_files(filter, sender, origin, opt_test_paths, embedder );


Neat: Trailing space.

paulrouget · 2018-04-16T05:59:26Z

components/net/filemanager_thread.rs

+                            embedder_proxy: EmbedderProxy) -> Option<Vec<String>> {
+        if opts::get().headless {
+            return None;
+        }


Can you let the embedder take the decision to return None when we run in headless mode?

Guess so, will try for it.

paulrouget · 2018-04-16T05:59:26Z

components/net/filemanager_thread.rs

-                       ui: &UI)
-        where UI: UIProvider,
-    {
+    fn get_selected_files(&self,


Neat: Maybe rename get_selected_files to query_files_from_embedder.

paulrouget · 2018-04-16T05:59:26Z

components/net/Cargo.toml

 [[test]]
 name = "main"
 path = "tests/main.rs"
+


Neat: extra line.

paulrouget · 2018-04-16T05:59:26Z

components/net/tests/fetch.rs

@@ -118,7 +118,8 @@ fn test_fetch_blob() {
    use ipc_channel::ipc;
    use net_traits::blob_url_store::BlobBuf;

-    let context = new_fetch_context(None);
+    //let (sender, _receiver) = ipc::channel().unwrap();


Remove this comment.

paulrouget · 2018-04-16T05:59:26Z

components/net/tests/filemanager_thread.rs

 #[test]
 fn test_filemanager() {
-    let filemanager = FileManager::new();
+    let filemanager = FileManager::new(create_embedder_proxy());
+    PREFS.set("dom.testing.htmlinputelement.select_files.enabled", PrefValue::Boolean(true));


I'm wondering why this was not necessary before?

Previously there was mock UI provider in filemanager thread does inject test files instead (https://github.com/servo/servo/pull/20480/files#diff-5a65a0caa6da8aa6e23da0a4101c6a86L19) regardless of testing prefs. Now changed code path removes UI provider from thread and always asks to embedder message, so setting up test to mock fixture for file selection.

paulrouget · 2018-04-16T05:59:26Z

components/script_traits/script_msg.rs

+pub enum FileManagerMsg {
+    /// Requesting to open file select dialog
+    OpenFileSelectDialog(Vec<FilterPattern>, bool, IpcSender<Option<Vec<String>>>)
+}


I might have missed something, but is that message actually used somewhere?

No, I guess it's my mistake leftover while refactoring. Thanks for catching.

paulrouget · 2018-04-16T05:59:26Z

components/net/filemanager_thread.rs

+        let msg = EmbedderMsg::SelectFiles(patterns, multiple_files, ipc_sender);
+
+        embedder_proxy.send(msg);
+        ipc_receiver.recv().unwrap()


Catch the error here:

match ipc_receiver.recv() { Err(e) => warn!("Failed to receive files from emebdder ({}).", e), ... }

bors-servo · 2018-04-27T19:14:56Z

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


        refactor(resourcethread): expose constellationmsg channel


        refactor(filemanager): send constellationmsg to select file


        refactor(compositor): expose new embeddermsg


        feat(browser): implement select file dialog


        refactor(filemanager): remove awareness to ui behavior


        test(net): update test cases


        refactor(constellation): replace constellationmsg to filemanagermsg


        refactor(filemanager): use filemanagermsg to request embedder


        refactor(filemanager): uses embedderproxy directly


        refactor(embedder): rename file select message


        test(net): update test cases

KiChjang · 2018-04-29T07:45:55Z

@bors-servo r=paulrouget

bors-servo · 2018-04-29T07:45:55Z

📌 Commit d94def6 has been approved by paulrouget

bors-servo · 2018-04-29T07:57:48Z

⌛ Testing commit d94def6 with merge bf66767...


        Auto merge of #20480 - kwonoj:refactor-file-manager, r=paulrouget

refactor(net): removes direct ui invocation from filemgr thread  - relates to #20428. This PR tries to refactor `net::filemanager_thread` implementation, removes direct ui-related code invocation but ask constellation. I believe overall organization might need to be refactored still as I took my own liberty to wire up dots and dots between components, which could be non-recommended practices. Probably point of review / need to be updated are 1. Communication between components Currently it's wired as like below: ``` +----------------+ +---------------+ +---------+ | | constellationMsg | | embedderMsg | | | filemgr_thread +------------------->+ constellation +------------->+ browser | | | | | | | +-----+----------+ +---------------+ +----+----+ ^ | +-------------------------------------------------------------------+ filelist: Vec(String) ``` - is this feasible approach? - does organization of message / fn (where to put consteallation / embedder msg & fns) are legit? 2. Removal of `filemanger_thread::UIProvider` - As filemanager_thread no longer need to aware actual ui, this PR removes `UIProvider` completely and let invoke tinyfiledialog directly in message listener. Maybe UIProvider itself still being needed? 3. Overall fn organization - To reduce duplicated code it takes single msg with boolean flag to distinguish selecting multiple files, may feasible / or better to create explicit paths between two. 4. 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. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20428 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ - Manually verified file picker `<input type=files>` can pick up files.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20480)

bors-servo · 2018-04-29T09:14:51Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: paulrouget
Pushing bf66767 to master...

highfive assigned Manishearth Mar 30, 2018

highfive added the S-awaiting-review label Mar 30, 2018

highfive added the S-needs-rebase label Apr 2, 2018

kwonoj force-pushed the kwonoj:refactor-file-manager branch from b758fd0 to 1d4287c Apr 2, 2018

paulrouget requested changes Apr 3, 2018

View changes

kwonoj force-pushed the kwonoj:refactor-file-manager branch from 1d4287c to 2c38b48 Apr 3, 2018

kwonoj force-pushed the kwonoj:refactor-file-manager branch 2 times, most recently from c45873d to 61a2b73 Apr 4, 2018

kwonoj mentioned this pull request Apr 6, 2018

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

Merged

3 of 6 tasks complete

kwonoj force-pushed the kwonoj:refactor-file-manager branch from 61a2b73 to 3b2a71c Apr 9, 2018

paulrouget mentioned this pull request Apr 10, 2018

The embedder should be handling prompts and alerts #19992

Closed

paulrouget requested changes Apr 10, 2018

View changes

kwonoj force-pushed the kwonoj:refactor-file-manager branch 3 times, most recently from fcb7bcc to c9b656f Apr 11, 2018

kwonoj force-pushed the kwonoj:refactor-file-manager branch from c9b656f to 62dd3dc Apr 14, 2018

paulrouget requested changes Apr 16, 2018

View changes

kwonoj force-pushed the kwonoj:refactor-file-manager branch from 62dd3dc to 2db008e Apr 16, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 27, 2018

highfive added the S-needs-rebase label Apr 27, 2018

kwonoj added 11 commits Mar 30, 2018

refactor(resourcethread): expose constellationmsg channel

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

1c465bc

refactor(filemanager): send constellationmsg to select file

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

b396a2f

refactor(compositor): expose new embeddermsg

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

2071245

feat(browser): implement select file dialog

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

4125b54

refactor(filemanager): remove awareness to ui behavior

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

f69b124

test(net): update test cases

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

c4c0d26

refactor(constellation): replace constellationmsg to filemanagermsg

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

52b9e4f

refactor(filemanager): use filemanagermsg to request embedder

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

7cec47b

refactor(filemanager): uses embedderproxy directly

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

2fab947

refactor(embedder): rename file select message

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

9ff11b9

test(net): update test cases

Verified

This commit was signed with a verified signature.

kwonoj OJ Kwon

GPG key ID: 6C23A45602A44DA6 Learn about signing commits

Loading status checks…

d94def6

kwonoj force-pushed the kwonoj:refactor-file-manager branch from e98c4ff to d94def6 Apr 27, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Apr 27, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Apr 29, 2018

bors-servo merged commit d94def6 into servo:master Apr 29, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 29, 2018

kwonoj deleted the kwonoj:refactor-file-manager branch Apr 30, 2018

gatoWololo mentioned this pull request Jun 25, 2019

Remove use of tinyfiledialogs from script code #23057

Closed

servo / servo

Join GitHub today

refactor(net): removes direct ui invocation from filemgr thread #20480

refactor(net): removes direct ui invocation from filemgr thread #20480

Conversation

kwonoj commented Mar 30, 2018 • edited

highfive commented Mar 30, 2018

bors-servo commented Apr 2, 2018

paulrouget commented Apr 2, 2018

kwonoj commented Apr 2, 2018

paulrouget left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kwonoj commented Apr 3, 2018

paulrouget commented Apr 3, 2018

kwonoj commented Apr 3, 2018

paulrouget commented Apr 10, 2018

kwonoj commented Apr 10, 2018

paulrouget commented Apr 10, 2018

paulrouget left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kwonoj commented Apr 10, 2018

kwonoj commented Apr 14, 2018

paulrouget left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Apr 27, 2018

KiChjang commented Apr 29, 2018

bors-servo commented Apr 29, 2018

bors-servo commented Apr 29, 2018

bors-servo commented Apr 29, 2018

kwonoj commented Mar 30, 2018 •

edited

paulrouget left a comment •

edited

paulrouget left a comment •

edited