Renderrenderrenderrender: Now for embedding!

[zmike]

A collection of commits which improves embedding integration and rendering.

@larsbergstrom

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5042

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[larsbergstrom]

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

• components/compositing/compositor.rs @ r6, r7
• components/compositing/windowing.rs @ r7
• components/servo/Cargo.lock @ r16
• ports/cef/browser.rs @ r5, r10, r11
• ports/cef/browser_host.rs @ r12, r15
• ports/cef/core.rs @ r4, r5, r14
• ports/cef/interfaces/cef_browser.rs @ r1
• ports/cef/interfaces/cef_render_handler.rs @ r3
• ports/cef/macros.rs @ r2
• ports/cef/render_handler.rs @ r7
• ports/cef/window.rs @ r4, r7, r8, r9, r12, r13, r15
• ports/glutin/window.rs @ r7
• ports/gonk/Cargo.lock @ r16
• ports/gonk/src/window.rs @ r7

---

^{ports/cef/browser.rs, line 113 [r10] (raw file):}
nit: can you outdent this line by 4 spaces to be under the l in let?

---

^{ports/cef/window.rs, line 179 [r10] (raw file):}
It would be worth a comment here explaining what you mentioned in the commit message about the platform-specific differences here (we tend to document in-tree instead of just in-commit message).

---

^{ports/cef/window.rs, line 259 [r16] (raw file):}
Please open an issue in the tracker for this

---

Comments from the review on Reviewable.io

[zmike]

Comments addressed.

[larsbergstrom]

Review status: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

• ports/cef/browser.rs @ r17
• ports/cef/window.rs @ r17

---

Comments from the review on Reviewable.io

[larsbergstrom]

@bors-servo: r+

[bors-servo]

📌 Commit 47fda5b has been approved by larsbergstrom

[bors-servo]

⌛ Testing commit 47fda5b with merge b096341...

[bors-servo]

💔 Test failed - mac1

[larsbergstrom]

failed to run custom build command for `x11 v1.1.0`
Process didn't exit successfully: `/Users/servo/buildbot/slave/mac1/build/components/servo/target/debug/build/x11-c15b07a09395e8a8/build-script-build` (exit code: 101)
--- stderr
thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: "`\"pkg-config\" \"--libs\" \"--cflags\" \"x11\"` did not exit successfully: exit code: 1\n--- stderr\nPackage x11 was not found in the pkg-config search path.\nPerhaps you should add the directory containing `x11.pc\'\nto the PKG_CONFIG_PATH environment variable\nNo package \'x11\' found\n"', /Users/larsberg/rust/src/libcore/result.rs:729
stack backtrace:
   1:        0x1043844ef - sys::backtrace::write::h6990067d88907b48zVr
   2:        0x10438cec4 - panicking::on_panic::he976cdbadffd9e1bEVv
   3:        0x104348ed5 - rt::unwind::begin_unwind_inner::ha519446d83876aa8nDv
   4:        0x104349d0c - rt::unwind::begin_unwind_fmt::h15c2d4adf8e8ecdatCv
   5:        0x10438c6bc - rust_begin_unwind
   6:        0x1043df495 - panicking::panic_fmt::haa553fbfab6eff2cNKy
   7:        0x1042fa5d3 - result::Result<T, E>::unwrap::h433998647731156453
   8:        0x1042fa376 - main::h319e68eb64359d51faa
   9:        0x104412278 - rust_try_inner
  10:        0x104412265 - rust_try
  11:        0x10438d948 - rt::lang_start::h62b79b4f7043bc7a9Pv
  12:        0x1042fcf1e - main

[larsbergstrom]

@bors-servo: retry

[bors-servo]

⌛ Testing commit 47fda5b with merge 3f207df...

[bors-servo]

💔 Test failed - mac2

[larsbergstrom]

@bors-servo: r+

[zmike]

HAHAHAHAHA

[larsbergstrom]

Wonder why I didn't see that on my linux box.

note: /usr/bin/ld: /home/servo/buildbot/slave/linux1/build/components/servo/target/debug/deps/libglutin-bf8dbe3e22b31c8a.rlib(glutin-bf8dbe3e22b31c8a.o): undefined reference to symbol 'XF86VidModeSetViewPort'
//usr/lib/x86_64-linux-gnu/libXxf86vm.so.1: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

error: aborting due to previous error
Could not compile `servo`.

[larsbergstrom]

@bjz it looks like gl_generator is not including libXxf86vm.so to the link lines - any idea what we're doing wrong here?

[larsbergstrom]

Or are we just supposed to add a linux-only #[link="Xxf86vm"] extern {} somewhere for this binding?

[larsbergstrom]

Hrm, strange, it's actually in the generated code:

./git/checkouts/glutin-666c1766c69e6584/master/src/x11/ffi.rs:#[link(name = "Xxf86vm")]

But it's not ending up in the link commandline :-/

Only shows up if we compile with the feature 'window' enabled. Maybe that's the issue? hrm.

[larsbergstrom]

Aha, on that builder it's building with SERVO_HEADLESS=1, which means that the window feature is not enabled, which means we won't get that #[link...] line, but also that we shouldn't be picking up that binding. Hrm.

[larsbergstrom]

Review status: all files reviewed, all discussions resolved, some commit checks failed.
Reviewed files:

• components/compositing/compositor.rs @ r18
• components/servo/Cargo.lock @ r24
• ports/cef/browser.rs @ r20
• ports/cef/browser_host.rs @ r18
• ports/cef/Cargo.lock @ r24
• ports/cef/lib.rs @ r21
• ports/cef/window.rs @ r21
• ports/glutin/Cargo.toml @ r21
• ports/glutin/lib.rs @ r21
• ports/glutin/window.rs @ r25
• ports/gonk/Cargo.lock @ r24
• ports/gonk/src/window.rs @ r18

---

Comments from the review on Reviewable.io

[larsbergstrom]

Aha! I believe that with the new x11, we need to set not only the xlib feature, but also the xf86vmode feature: https://github.com/Daggerbot/x11-rs/blob/master/x11/build.rs#L10

[larsbergstrom]

OK, I think if we merge servo/glutin#28 and pick up a reference to it in this PR, we're good. Servo appears to build both with/without headless mode enabled with that fix.

[zmike]

@larsbergstrom I HAVE COMPLETED THE UPDATE AND AM AWAITING TEST RESULTS

[larsbergstrom]

@bors-servo: r+

[bors-servo]

📌 Commit c884b43 has been approved by larsbergstrom

[bors-servo]

⌛ Testing commit c884b43 with merge 4a95bce...

[larsbergstrom]

@zmike I HAVE HEARD THE SIXTH TIME IS THE CHARM

[zmike]

@larsbergstrom I THINK THAT'S ONLY WHEN COUNTING SETS ON BENCH

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

[alexcrichton]

@larsbergstrom

These questions may have already been answered as this was merged, but...

Do you have an example of the latter?

Ah yes I was thinking specifically of target-specific dependencies (as you pointed out), by having all platforms but android depend on the x11 libraries (if android doesn't need them)

I think the problem here is actually that glutin has a linux-target-only dependency on X11, but on linux host cross-compiling for android target, that dependency is still getting pulled in.

Interesting! How come glutin is being compiled for this host?

[larsbergstrom]

@alexcrichton

Interesting! How come glutin is being compiled for this host?

From what I can tell, almost none of our dependencies have yet switched to platform-specific Cargo dependency links and instead use #[cfg(target_os=...)] blocks in their lib.rs file. This particular problem was that glutin just had a dependency on x11, which used to do nothing on platforms where it would not be used, but the move to pkg-config broke that. The x11 package has since been updated to make that cleaner, but I suspect we should start transitioning all of our platform-specific dependencies to use the Cargo features, if only to avoid seeing rust-core-foundation show up when cross-compiling for Android :-)

[alexcrichton]

@larsbergstrom ah ok that makes sense. Currently the main use-case I know of for platform-specific dependencies is precisely this, just avoiding compilation of native dependencies. Note that you may run into rust-lang/cargo#1007 where the current syntax may unfortunately be a little verbose.