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

Use simpleservo embedding API for Magic Leap #22871

Merged
merged 4 commits into from Mar 4, 2019

Conversation

Projects
None yet
5 participants
@jdm
Copy link
Member

jdm commented Feb 12, 2019

This removes the duplication between the two ports and makes it easier to improve all of our embeddings simultaneously.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22065
  • These changes do not require tests because no automated tests for embedded devices.

This change is Reviewable

@jdm jdm force-pushed the jdm:simpleml branch from 6fb3db3 to 1936da3 Feb 12, 2019

@highfive

This comment has been minimized.

Copy link

highfive commented Feb 12, 2019

Heads up! This PR modifies the following files:

  • @paulrouget: components/servo/Cargo.toml, components/servo/lib.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Feb 12, 2019

warning Warning warning

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

This comment has been minimized.

Copy link
Member Author

jdm commented Feb 12, 2019

r? @asajeffrey
Paul already signed off on the non-ML changes in #22065 (comment).

@highfive highfive assigned asajeffrey and unassigned ferjm Feb 12, 2019

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Feb 13, 2019

My hot take is that this is much nicer. I'll have a less-hot take tomorrow.

@asajeffrey
Copy link
Member

asajeffrey left a comment

OK, this looks much much nicer. The use of the SERVO global variable is not what I would do, but everything else is much nicer.

@@ -101,6 +84,16 @@ pub struct MLApp(*mut c_void);

const LOG_LEVEL: log::LevelFilter = log::LevelFilter::Info;

fn call<F>(f: F) -> Result<(), &'static str>

This comment has been minimized.

@asajeffrey

asajeffrey Feb 13, 2019

Member

Parameterize this over the return type?


let gl = gl_glue::egl::init().expect("EGL initialization failure");
info!("OpenGL version {}", gl.get_string(gl::VERSION));

This comment has been minimized.

@asajeffrey

asajeffrey Feb 13, 2019

Member

I think we can get rid of this info!, it was useful while debugging but not any more.

where
F: FnOnce(&mut ServoGlue) -> Result<(), &'static str>,
{
SERVO.with(|s| match s.borrow_mut().as_mut() {

This comment has been minimized.

@asajeffrey

asajeffrey Feb 13, 2019

Member

Sigh, libsimpleservo uses a global variable? This is a bit of a step back, libmlservo passed the servo instance around as a parameter.

This comment has been minimized.

@jdm

jdm Mar 1, 2019

Author Member

I'll file a followup issue to discuss this choice.

@jdm jdm force-pushed the jdm:simpleml branch from 1936da3 to 176e722 Mar 1, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Mar 1, 2019

@bors-servo r=paul,asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

📌 Commit 176e722 has been approved by paul,asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

⌛️ Testing commit 176e722 with merge 40e9b84...

bors-servo added a commit that referenced this pull request Mar 1, 2019

Auto merge of #22871 - jdm:simpleml, r=paul,asajeffrey
Use simpleservo embedding API for Magic Leap

This removes the duplication between the two ports and makes it easier to improve all of our embeddings simultaneously.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22065
- [x] These changes do not require tests because no automated tests for embedded devices.

<!-- 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/22871)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

💔 Test failed - magicleap

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Mar 4, 2019

@bors-servo r=ajeffrey,paul

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 4, 2019

⌛️ Testing commit 7b19eec with merge 5a044f4...

bors-servo added a commit that referenced this pull request Mar 4, 2019

Auto merge of #22871 - jdm:simpleml, r=ajeffrey,paul
Use simpleservo embedding API for Magic Leap

This removes the duplication between the two ports and makes it easier to improve all of our embeddings simultaneously.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22065
- [x] These changes do not require tests because no automated tests for embedded devices.

<!-- 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/22871)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 4, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Mar 4, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 4, 2019

⌛️ Testing commit 7b19eec with merge d566102...

bors-servo added a commit that referenced this pull request Mar 4, 2019

Auto merge of #22871 - jdm:simpleml, r=ajeffrey,paul
Use simpleservo embedding API for Magic Leap

This removes the duplication between the two ports and makes it easier to improve all of our embeddings simultaneously.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22065
- [x] These changes do not require tests because no automated tests for embedded devices.

<!-- 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/22871)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 4, 2019

💔 Test failed - android-mac

@jdm jdm force-pushed the jdm:simpleml branch from 7b19eec to 5b7be25 Mar 4, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Mar 4, 2019

@bors-servo r=ajeffrey,paul

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 4, 2019

📌 Commit 5b7be25 has been approved by ajeffrey,paul

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 4, 2019

⌛️ Testing commit 5b7be25 with merge cfb401e...

bors-servo added a commit that referenced this pull request Mar 4, 2019

Auto merge of #22871 - jdm:simpleml, r=ajeffrey,paul
Use simpleservo embedding API for Magic Leap

This removes the duplication between the two ports and makes it easier to improve all of our embeddings simultaneously.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22065
- [x] These changes do not require tests because no automated tests for embedded devices.

<!-- 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/22871)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 4, 2019

@bors-servo bors-servo merged commit 5b7be25 into servo:master Mar 4, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.