{"action":"created","issue":{"url":"https://api.github.com/repos/servo/servo/issues/20480","repository_url":"https://api.github.com/repos/servo/servo","labels_url":"https://api.github.com/repos/servo/servo/issues/20480/labels{/name}","comments_url":"https://api.github.com/repos/servo/servo/issues/20480/comments","events_url":"https://api.github.com/repos/servo/servo/issues/20480/events","html_url":"https://github.com/servo/servo/pull/20480","id":310155399,"number":20480,"title":"refactor(net): removes direct ui invocation from filemgr thread","user":{"login":"kwonoj","id":1210596,"avatar_url":"https://avatars2.githubusercontent.com/u/1210596?v=4","gravatar_id":"","url":"https://api.github.com/users/kwonoj","html_url":"https://github.com/kwonoj","followers_url":"https://api.github.com/users/kwonoj/followers","following_url":"https://api.github.com/users/kwonoj/following{/other_user}","gists_url":"https://api.github.com/users/kwonoj/gists{/gist_id}","starred_url":"https://api.github.com/users/kwonoj/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kwonoj/subscriptions","organizations_url":"https://api.github.com/users/kwonoj/orgs","repos_url":"https://api.github.com/users/kwonoj/repos","events_url":"https://api.github.com/users/kwonoj/events{/privacy}","received_events_url":"https://api.github.com/users/kwonoj/received_events","type":"User","site_admin":false},"labels":[{"id":152310043,"url":"https://api.github.com/repos/servo/servo/labels/S-awaiting-review","name":"S-awaiting-review","color":"d4c5f9","default":false},{"id":152311003,"url":"https://api.github.com/repos/servo/servo/labels/S-needs-rebase","name":"S-needs-rebase","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":{"login":"Manishearth","id":1617736,"avatar_url":"https://avatars2.githubusercontent.com/u/1617736?v=4","gravatar_id":"","url":"https://api.github.com/users/Manishearth","html_url":"https://github.com/Manishearth","followers_url":"https://api.github.com/users/Manishearth/followers","following_url":"https://api.github.com/users/Manishearth/following{/other_user}","gists_url":"https://api.github.com/users/Manishearth/gists{/gist_id}","starred_url":"https://api.github.com/users/Manishearth/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Manishearth/subscriptions","organizations_url":"https://api.github.com/users/Manishearth/orgs","repos_url":"https://api.github.com/users/Manishearth/repos","events_url":"https://api.github.com/users/Manishearth/events{/privacy}","received_events_url":"https://api.github.com/users/Manishearth/received_events","type":"User","site_admin":false},"assignees":[{"login":"Manishearth","id":1617736,"avatar_url":"https://avatars2.githubusercontent.com/u/1617736?v=4","gravatar_id":"","url":"https://api.github.com/users/Manishearth","html_url":"https://github.com/Manishearth","followers_url":"https://api.github.com/users/Manishearth/followers","following_url":"https://api.github.com/users/Manishearth/following{/other_user}","gists_url":"https://api.github.com/users/Manishearth/gists{/gist_id}","starred_url":"https://api.github.com/users/Manishearth/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Manishearth/subscriptions","organizations_url":"https://api.github.com/users/Manishearth/orgs","repos_url":"https://api.github.com/users/Manishearth/repos","events_url":"https://api.github.com/users/Manishearth/events{/privacy}","received_events_url":"https://api.github.com/users/Manishearth/received_events","type":"User","site_admin":false}],"milestone":null,"comments":18,"created_at":"2018-03-30T20:00:34Z","updated_at":"2018-04-18T07:39:03Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/servo/servo/pulls/20480","html_url":"https://github.com/servo/servo/pull/20480","diff_url":"https://github.com/servo/servo/pull/20480.diff","patch_url":"https://github.com/servo/servo/pull/20480.patch"},"body":"<!-- Please describe your changes on the following line: -->\r\n- relates to https://github.com/servo/servo/issues/20428.\r\n\r\nThis 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.\r\n\r\nProbably point of review / need to be updated are\r\n\r\n1. Communication between components \r\nCurrently it's wired as like below:\r\n```\r\n+----------------+                    +---------------+              +---------+\r\n|                |  constellationMsg  |               | embedderMsg  |         |\r\n| filemgr_thread +------------------->+ constellation +------------->+ browser |\r\n|                |                    |               |              |         |\r\n+-----+----------+                    +---------------+              +----+----+\r\n      ^                                                                   |\r\n      +-------------------------------------------------------------------+\r\n                                filelist: Vec(String)\r\n```\r\n- is this feasible approach?\r\n- does organization of message / fn (where to put consteallation / embedder msg & fns) are legit?\r\n\r\n2. Removal of `filemanger_thread::UIProvider`\r\n- 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?\r\n\r\n3. Overall fn organization\r\n- 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.\r\n\r\n4. Invoking tfd in a separate thread\r\n- 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.\r\n\r\nand of course, a lot of other codes need to follow better practices.\r\n\r\n\r\n---\r\n<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->\r\n- [x] `./mach build -d` does not report any errors\r\n- [x] `./mach test-tidy` does not report any errors\r\n- [x] These changes fix #20428 (github issue number if applicable).\r\n\r\n<!-- Either: -->\r\n- [ ] There are tests for these changes OR\r\n- [ ] These changes do not require tests because _____\r\n- Manually verified file picker `<input type=files>` can pick up files.\r\n\r\n<!-- 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.-->\r\n\r\n<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/servo/servo/20480)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/servo/servo/issues/comments/382293672","html_url":"https://github.com/servo/servo/pull/20480#issuecomment-382293672","issue_url":"https://api.github.com/repos/servo/servo/issues/20480","id":382293672,"user":{"login":"bors-servo","id":4368172,"avatar_url":"https://avatars1.githubusercontent.com/u/4368172?v=4","gravatar_id":"","url":"https://api.github.com/users/bors-servo","html_url":"https://github.com/bors-servo","followers_url":"https://api.github.com/users/bors-servo/followers","following_url":"https://api.github.com/users/bors-servo/following{/other_user}","gists_url":"https://api.github.com/users/bors-servo/gists{/gist_id}","starred_url":"https://api.github.com/users/bors-servo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bors-servo/subscriptions","organizations_url":"https://api.github.com/users/bors-servo/orgs","repos_url":"https://api.github.com/users/bors-servo/repos","events_url":"https://api.github.com/users/bors-servo/events{/privacy}","received_events_url":"https://api.github.com/users/bors-servo/received_events","type":"User","site_admin":false},"created_at":"2018-04-18T07:39:03Z","updated_at":"2018-04-18T07:39:03Z","author_association":"CONTRIBUTOR","body":":pushpin: Commit f96b5f5 has been approved by `paulrouget`\n\n<!-- @bors-servo r=paulrouget f96b5f51b19ecfadaa5f1f3a74eda5f9babf1deb -->"}}
{"actor":{"display_login":"bors-servo"}}