Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract OS clipboard integration into embedding layer #23440

Closed
jdm opened this issue May 22, 2019 · 5 comments
Closed

Extract OS clipboard integration into embedding layer #23440

jdm opened this issue May 22, 2019 · 5 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented May 22, 2019

Rather than implicitly supporting the OS clipboard everywhere by creating a ClipboardContext in the constellation, this is really something that should be supported separately by each Servo embedder.

We should:

  • remove the GetClipboardContents/SetClipboardContents constellation messages
  • add GetClipboardContents and SetClipboardContents messages to EmbedderMsg
  • make the code that uses the clipboard provider in textinput.rs use Window's send_to_embedder method instead
  • add new methods for getting/setting the clipboard contents to HostTrait
  • call these new methods from the embedding API's handle_servo_events
  • make the code that processes the new embedder clipboard messages call these new methods
  • move the constellation's ClipboardContext creation to the Browser structure
  • handle the new embedder clipboard messages in the glutin port's handle_servo_events by using the stored ClipboardContext

Code:

  • components/script/clipboard_provider.rs
  • components/script/textinput.rs
  • components/constellation/constellation.rs
  • components/embedder_traits/lib.rs
  • components/script_traits/script_msg.rs
  • ports/glutin/browser.rs
  • ports/libsimpleservo/api/src.rs

There are no automated tests for this; it's enough to manually test that copying and pasting inside of a <input> inside the desktop Servo build still works before and after these changes.

@mmiecz
Copy link
Contributor

@mmiecz mmiecz commented May 27, 2019

Hi, this looks like good first task to get to know Servo! @jdm can you assign this one to me, please?

@CYBAI CYBAI added the C-assigned label May 27, 2019
@mmiecz
Copy link
Contributor

@mmiecz mmiecz commented Jun 2, 2019

I've been looking into this task for some time, and I am not really sure how to call Windows send_to_embedder method from textinput.rs.
Are there any similar use-cases where I can look and see how I should approach this?

@jdm
Copy link
Member Author

@jdm jdm commented Jun 2, 2019

@mmiecz The implementation of send_to_embedder sends a ScriptMsg::ForwardToEmbedder message to the constellation. The current implementation of the clipboard provider uses a channel to the constellation, so you should be able to send the ForwardToEmbedder message directly in clipboard_provider.rs instead.

@jdm
Copy link
Member Author

@jdm jdm commented Jun 12, 2019

@mmiecz Are you still working on this?

@mmiecz
Copy link
Contributor

@mmiecz mmiecz commented Jun 12, 2019

@jdm Yes, I’ve done the sending to embedder, and removing the constellation part, and the clipboard is functional. I am not at home right now, but I will issue a PR soon

@mmiecz mmiecz mentioned this issue Jun 13, 2019
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jul 2, 2019
Clipboard refactoring

<!-- Please describe your changes on the following line: -->
This PR removes clipboard handling from the constellation. Instead, now embedder handles it.

---
<!-- 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 #23440 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it is enough to test manually in input box, if copying and pasting still works .

<!-- 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/23564)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 2, 2019
Clipboard refactoring

<!-- Please describe your changes on the following line: -->
This PR removes clipboard handling from the constellation. Instead, now embedder handles it.

---
<!-- 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 #23440 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it is enough to test manually in input box, if copying and pasting still works .

<!-- 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/23564)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 3, 2019
Clipboard refactoring

<!-- Please describe your changes on the following line: -->
This PR removes clipboard handling from the constellation. Instead, now embedder handles it.

---
<!-- 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 #23440 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it is enough to test manually in input box, if copying and pasting still works .

<!-- 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/23564)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 3, 2019
Clipboard refactoring

<!-- Please describe your changes on the following line: -->
This PR removes clipboard handling from the constellation. Instead, now embedder handles it.

---
<!-- 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 #23440 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it is enough to test manually in input box, if copying and pasting still works .

<!-- 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/23564)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.