Clipboard refactoring

[mmiecz]

This PR removes clipboard handling from the constellation. Instead, now embedder handles it.

---

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

• There are tests for these changes OR
• These changes do not require tests because it is enough to test manually in input box, if copying and pasting still works .

---

This change is 

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/script/clipboard_provider.rs, components/constellation/constellation.rs
• @cbrewster: components/constellation/constellation.rs
• @paulrouget: ports/glutin/browser.rs, ports/glutin/main2.rs, ports/glutin/Cargo.toml, components/constellation/constellation.rs
• @KiChjang: components/script/clipboard_provider.rs, components/script_traits/script_msg.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[jdm]

error: unused extern crate
 --> ports/glutin/main2.rs:5:1
  |
5 | extern crate clipboard;
  | ^^^^^^^^^^^^^^^^^^^^^^^ help: remove it
  |
  = note: `-D unused-extern-crates` implied by `-D warnings`

[jdm]

Looks good!

[jdm]

Let's remove this.

[mmiecz]

Done, removed.

[jdm]

If you build ./mach build --libsimpleservo, you should find some other places where you need to handle the new messages.

[jdm]

Sorry for the delay; last week was very busy. One last change which will allow us to provide different clipboard solutions on other platforms like android/magic leap/hololens!

[jdm]

Since libsimpleservo/api is a platform-agnostic embedding API, we should not add a platform-specific dependency like clipboard to it.

[jdm]

Instead of directly manipulating the clipboard for the GetClipboardContents and SetClipboardContents messages, let's add new callbacks to HostTrait (get_clipboard_contents() can return an Option and set_clipboard_contents can accept a String) and call those instead. See how EmbedderMsg::HistoryChanged is handled earlier.

[mmiecz]

Ok thanks for the tips @jdm :). I've removed direct handling of clipboard from libsimpleservo, and added methods to HostTrait as mentioned.

I am not sure if I'm doing implementation right though?:

 fn get_clipboard_contents(&self) -> Option<String> {
        debug!("get_clipboard_contents");
        let raw_contents = (self.0.get_clipboard_contents)();
        if raw_contents.is_null() {
            return None;
        }
        let c_str = unsafe { CStr::from_ptr(raw_contents) };
        let contents_str = c_str.to_str().expect("Can't create str");
        Some(contents_str.to_owned())
    }

    fn set_clipboard_contents(&self, contents: String) {
        debug!("set_clipboard_contents");
        let contents = CString::new(contents).expect("Can't create string");
        let contents_ptr = contents.as_ptr();
        mem::forget(contents);
        (self.0.set_clipboard_contents)(contents_ptr);
    }

I guess this two methods should call some external ( platform specific ) function for getting and setting system clipboard. This is just for capi, but this should be done for jniapi also, right? Call java get/setClipboardContents from jniapi/lib/src.rs?

[jdm]

For jniapi and mlservo you can leave stub implementations, unless you are familiar enough with android that you're confident about the JNI work :)

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 775ff3b has been approved by jdm

[bors-servo]

⌛ Testing commit 775ff3b with merge 1601e75...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

More warnings from other platforms:

�[0m�[1m�[38;5;9merror�[0m�[0m�[1m: unused variable: `contents`�[0m
�[0m   �[0m�[0m�[1m�[38;5;12m--> �[0m�[0mports/libsimpleservo/jniapi/src/lib.rs:484:38�[0m
�[0m    �[0m�[0m�[1m�[38;5;12m|�[0m
�[0m�[1m�[38;5;12m484�[0m�[0m �[0m�[0m�[1m�[38;5;12m| �[0m�[0m    fn set_clipboard_contents(&self, contents: String) {}�[0m
�[0m    �[0m�[0m�[1m�[38;5;12m| �[0m�[0m                                     �[0m�[0m�[1m�[38;5;9m^^^^^^^^�[0m�[0m �[0m�[0m�[1m�[38;5;9mhelp: consider prefixing with an underscore: `_contents`�[0m
�[0m    �[0m�[0m�[1m�[38;5;12m|�[0m
�[0m    �[0m�[0m�[1m�[38;5;12m= �[0m�[0m�[1mnote�[0m�[0m: `-D unused-variables` implied by `-D warnings`�[0m

error: unused variable: `contents`
   --> ports/libmlservo/src/lib.rs:386:38
    |
386 |     fn set_clipboard_contents(&self, contents: String) {}
    |                                      ^^^^^^^^ help: consider prefixing with an underscore: `_contents`
    |
    = note: `-D unused-variables` implied by `-D warnings`

[mmiecz]

Can I build/test other platforms somehow on my Linux machine to see and fix such failures before issuing a PR? :)

[jdm]

The jni one you could build by following https://github.com/servo/servo/wiki/Building-for-Android, but it's not necessarily worth it. You can see all of the failures from the previous build by looking at https://treeherder.allizom.org/#/jobs?repo=servo-auto .

[jdm]

The libmlservo can only be built by people with access to the Magic Leap SDK, which is restricted to a small set of developers right now.

[mmiecz]

Thanks @jdm for explanation. I've fixed the unused arguments warnings.

[jdm]

@bors-servo r+
Thanks!

[bors-servo]

📌 Commit 7fb2bbe has been approved by jdm

[bors-servo]

⌛ Testing commit 7fb2bbe with merge e382266...

[bors-servo]

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing e382266 to master...