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

Add Wrench to WebRender #585

Merged
merged 2 commits into from Nov 29, 2016
Merged

Add Wrench to WebRender #585

merged 2 commits into from Nov 29, 2016

Conversation

@vvuk
Copy link
Contributor

vvuk commented Nov 22, 2016

This moves Wrench to WebRender. It can now replay binary recordings (replaces the wr-replay tool, though I haven't removed that one), show frames from YAML, and write YAML while replaying a binary recording. The YAML stuff is very much WIP, but what's there works and can be built on top of.


This change is Reviewable

@vvuk vvuk force-pushed the vvuk:wrench branch from 2cf8db8 to dd7c9bf Nov 22, 2016
Copy link
Member

glennw left a comment

One question, otherwise r=me

]

[profile.release]
debug = true

This comment has been minimized.

Copy link
@glennw

glennw Nov 23, 2016

Member

Does this have any effect on runtime performance? Should we just enable it when profiling?

Copy link
Member

kvark left a comment

Got a few questions/comments, otherwise looks good!

panic!("Can't layout simple ascii on this platform");
}

pub trait WrenchThing {

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

I'd call it Player


impl WrenchThing for BinaryFrameReader {
fn do_frame(&mut self, wrench: &mut Wrench) -> u32 {
if self.do_frame(wrench) == false {

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

should we check for frame_num == 0 here as well?

use webrender_traits::*;

mod wrench;
use wrench::{Wrench, WrenchThing};

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

I think this is against rust style guidelines, where you are supposed to have all the use to follow all the mod things

static ref BLACK_COLOR: ColorF = ColorF::new(0.0, 0.0, 0.0, 1.0);
}

pub static mut CURRENT_FRAME_NUMBER: u32 = 0;

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

Where is this getting used? I only see assignment.


pub static mut CURRENT_FRAME_NUMBER: u32 = 0;

enum ThingKind {

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

again, I wish it had a more descriptive name, like FrameSource

}

pub struct Wrench {
pub window: glutin::Window,

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

does it all have to be public?
I know this is not much important, given that it's not a used API but just a tool
wondering just out of interest

.build().unwrap();

unsafe {
window.make_current().ok();

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

I think we should be using unwrap() here, because ok() will not crash, and our behavior would be undefined later on

}

let gl_version = unsafe {
let data = CStr::from_ptr(gl::GetString(gl::VERSION) as *const _).to_bytes().to_vec();

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

you can borrow some ideas from #584

}

fn handle_rect(&self, wrench: &mut Wrench, clip_region: &ClipRegion, item: &Yaml)
{

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

opening bracket - against the code guidelines (sorry to be pedantic!)

}

fn handle_image(&self, wrench: &mut Wrench, clip_region: &ClipRegion, item: &Yaml)
{

This comment has been minimized.

Copy link
@kvark

kvark Nov 24, 2016

Member

same here and for handle_text

@bors-servo
Copy link
Contributor

bors-servo commented Nov 28, 2016

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

@vvuk vvuk force-pushed the vvuk:wrench branch from dd7c9bf to 2366155 Nov 28, 2016
@glennw
Copy link
Member

glennw commented Nov 28, 2016

@vvuk Happy to merge this when you are ready - let me know when it's good to go.

@vvuk vvuk force-pushed the vvuk:wrench branch from 6b0e485 to 7be8661 Nov 29, 2016
@glennw
Copy link
Member

glennw commented Nov 29, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 29, 2016

📌 Commit 7be8661 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 29, 2016

Test exempted - status

@bors-servo bors-servo merged commit 7be8661 into servo:master Nov 29, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
bors-servo added a commit that referenced this pull request Nov 29, 2016
Add Wrench to WebRender

This moves Wrench to WebRender.  It can now replay binary recordings (replaces the wr-replay tool, though I haven't removed that one), show frames from YAML, and write YAML while replaying a binary recording.  The YAML stuff is very much WIP, but what's there works and can be built on top of.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/585)
<!-- Reviewable:end -->
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

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