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

Embedding windowing #6016

Merged
merged 8 commits into from May 15, 2015
Merged

Embedding windowing #6016

merged 8 commits into from May 15, 2015

Conversation

@zmike
Copy link
Contributor

zmike commented May 12, 2015

Depends on glutin PR #21

@glennw @larsbergstrom

Review on Reviewable

@highfive
Copy link

highfive commented May 12, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 12, 2015

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

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.

@bors-servo
Copy link
Contributor

bors-servo commented May 12, 2015

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

@zmike zmike force-pushed the zmike:embedding-windowing branch from b7d123b to 4513d45 May 12, 2015
@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 12, 2015

Reviewed files:

  • ports/cef/browser.rs @ r6
  • ports/cef/browser_host.rs @ r6
  • ports/cef/core.rs @ r4
  • ports/glutin/window.rs @ r6

Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 12, 2015

Does this also need an update to the CEF lock file for the updated glutin with landed commit? I believe you can do so with:

./mach update-cargo -p glutin

Also, waiting to approve until you resolve the URL-related changes you wanted to make.

@zmike
Copy link
Contributor Author

zmike commented May 12, 2015

I'll answer all these questions in about 4 hours when this test compile finishes.

@zmike
Copy link
Contributor Author

zmike commented May 12, 2015

We can just say this depends on #5995 now

@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 13, 2015

This is r=me once #5995 lands

@larsbergstrom larsbergstrom self-assigned this May 13, 2015
Mike Blumenkrantz
@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 13, 2015

@bors-servo
Copy link
Contributor

bors-servo commented May 13, 2015

📌 Commit 6c32998 has been approved by larsbergstrom

bors-servo pushed a commit that referenced this pull request May 13, 2015
Depends on glutin PR #21

@glennw  @larsbergstrom

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6016)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 13, 2015

Testing commit 6c32998 with merge da27b07...

@bors-servo
Copy link
Contributor

bors-servo commented May 13, 2015

💔 Test failed - mac2

@jdm
Copy link
Member

jdm commented May 13, 2015

The reftest failure is #5958. However:

browser.rs:105:54: 105:55 error: mismatched types:
 expected `*mut libc::types::common::c95::c_void`,
    found `usize`
(expected *-ptr,
    found usize) [E0308]
browser.rs:105         let mut window_handle: cef_window_handle_t = 0;
                                                                    ^
browser.rs:151:41: 151:42 error: mismatched types:
 expected `*mut libc::types::common::c95::c_void`,
    found `usize`
(expected *-ptr,
    found usize) [E0308]
browser.rs:151         if window_info.parent_window != 0 {
                                                       ^
error: aborting due to 2 previous errors
Could not compile `embedding`.
@zmike
Copy link
Contributor Author

zmike commented May 14, 2015

Argh! Well I can't compile for macos, so I guess I'll keep putting updates into the branch to see if it compiles.

@zmike
Copy link
Contributor Author

zmike commented May 14, 2015

@jdm Try now I guess?

@jdm
Copy link
Member

jdm commented May 14, 2015

Any reason why you didn't initialize it to ptr::mut_null() and check .is_null()?

@jdm
Copy link
Member

jdm commented May 14, 2015

Nevermind, this looks like it would work too.

@jdm
Copy link
Member

jdm commented May 14, 2015

@bors-servo
Copy link
Contributor

bors-servo commented May 14, 2015

📌 Commit c254f9c has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 14, 2015

Testing commit c254f9c with merge 57a5a09...

bors-servo pushed a commit that referenced this pull request May 14, 2015
Depends on glutin PR #21

@glennw  @larsbergstrom

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6016)
<!-- Reviewable:end -->
@zmike
Copy link
Contributor Author

zmike commented May 14, 2015

On linux it's an int type, not a ptr.

@bors-servo
Copy link
Contributor

bors-servo commented May 14, 2015

💔 Test failed - mac2

@jdm
Copy link
Member

jdm commented May 14, 2015

browser.rs:105:54: 105:70 error: the trait `core::default::Default` is not implemented for the type `*mut libc::types::common::c95::c_void` [E0277]
browser.rs:105         let mut window_handle: cef_window_handle_t = Default::default();
                                                                    ^~~~~~~~~~~~~~~~
@zmike
Copy link
Contributor Author

zmike commented May 14, 2015

I tried doing per-platform initialization using [cfg(target_os... lines, but, I get errors like

browser.rs:107:25: 107:26 error: expected item after attributes
browser.rs:107 #[cfg(target_os="macos")]

Suggestions?

@jdm
Copy link
Member

jdm commented May 14, 2015

There's if cfg!(...) you can use for that instead.

Mike Blumenkrantz
@zmike zmike force-pushed the zmike:embedding-windowing branch from c254f9c to 6f6945a May 15, 2015
@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 15, 2015

@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2015

📌 Commit 6f6945a has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2015

Testing commit 6f6945a with merge a97f81b...

bors-servo pushed a commit that referenced this pull request May 15, 2015
Depends on glutin PR #21

@glennw  @larsbergstrom

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6016)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2015

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

@bors-servo bors-servo merged commit 6f6945a into servo:master May 15, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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