Skip to content

/ servo

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

Iframes #3951

Merged
merged 4 commits into from Nov 12, 2014
Merged

Iframes #3951

merged 4 commits into from Nov 12, 2014

Conversation

@mrobinson
Copy link
Member

mrobinson commented Nov 10, 2014

This is the first step to allowing incremental iframe creation and destruction. This should eliminate task failures when an iframe is added to the frame tree lazily via script.
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Nov 10, 2014

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

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.
@mrobinson
Copy link
Member Author

mrobinson commented Nov 10, 2014

@jdm Perhaps you could review this or suggest someone for review? Thanks!
components/compositing/compositor.rs Outdated
use compositor_task::{ShutdownComplete, ChangeRenderState, RenderMsgDiscarded, ScrollTimeout};
use compositor_task::{CompositorEventListener, CompositorProxy, CompositorReceiver};
use constellation::SendableFrameTree;
use compositor_task::{

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Nov 10, 2014

Contributor

nit: I usually prefer the above style (that you removed) for imports. The reason is that I can select all the import lines and say :sort in Vim and have it look nice :)

This comment has been minimized.

Sign in to view
Copy link
@mrobinson

mrobinson Nov 11, 2014

Author Member

I've been intrigued by this all morning, is there a way to configure vim (which I also use) to sort imports that are inside {} pairs and spread across multiple lines?

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Nov 11, 2014

Contributor

Not that I know of. I think the right solution is to just have rustfmt do it, once we have that.
components/compositing/compositor.rs
epoch: Epoch(0),
id: LayerId::null(),
rect: Rect::zero(),
background_color: azure_hl::Color::new(0., 0., 0., 0.),

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Nov 10, 2014

Contributor

At some point we should have a TRANSPARENT constant for this color.
components/compositing/constellation.rs Outdated
@@ -737,21 +750,26 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {

}

fn pipeline_is_in_current_frame(&self, pipeline_id: PipelineId) -> bool {
for current_frame in self.current_frame().iter() {

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Nov 10, 2014

Contributor

self.current_frame.iter().any(|current_frame| current_frame.contains(pipeline_id))
@pcwalton
Copy link
Contributor

pcwalton commented Nov 10, 2014

Looks good to me; @jdm should give it a look over too.
@jdm
Copy link
Member

jdm commented Nov 10, 2014

Critic'd. I can't comment on the actual correctness of the compositor code, per se, but it looks reasonable to me.
@pcwalton
Copy link
Contributor

pcwalton commented Nov 10, 2014

r=me for the compositor parts once @jdm's issues are addressed.
@jdm
Copy link
Member

jdm commented Nov 11, 2014

Squash those puppies.
mrobinson added 4 commits Nov 6, 2014
@mrobinson
Rename LoadIframeUrlMsg to CreateIFrameAndLoadUrlMsg
6e06a13 
This better reflects what the message does.
@mrobinson
Small fix to variable name
a10e261 
This is a more accurate name for the script pipeline.
@mrobinson
Add some factory methods for frame tree types
750beda
@mrobinson
Send incremental frame tree updates to the compositor
Loading status checks…
fbb1e0c 
This allows the compositor to add frames after the call to SetIds,
where the initial frame tree is created. There are still some issues
preventing proper late frame creation, but this prevents crashes when
it happens.

Fixes #3738.
@mrobinson mrobinson force-pushed the mrobinson:iframes branch from ae2a338 to fbb1e0c Nov 12, 2014
@mrobinson
Copy link
Member Author

mrobinson commented Nov 12, 2014

Okay. Everything is smooshed. I'll move it to bors.
@mrobinson

This comment has been minimized.

Sign in to view
Copy link
Owner Author

mrobinson commented on fbb1e0c Nov 12, 2014

r=jdm
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented on fbb1e0c Nov 12, 2014

saw approval from jdm
at mrobinson@fbb1e0c

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 12, 2014

merging mrobinson/servo/iframes = fbb1e0c into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 12, 2014

mrobinson/servo/iframes = fbb1e0c merged ok, testing candidate = ccdd291

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 12, 2014

all tests pass:
success: http://build.servo.org/builders/linux/builds/592
success: http://build.servo.org/builders/mac1/builds/134
success: http://build.servo.org/builders/mac2/builds/133

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 12, 2014

fast-forwarding master to auto = ccdd291
bors-servo pushed a commit that referenced this pull request Nov 12, 2014
bors-servo
auto merge of #3951 : mrobinson/servo/iframes, r=jdm
Loading status checks…
ccdd291 
This is the first step to allowing incremental iframe creation and destruction. This should eliminate task failures when an iframe is added to the frame tree lazily via script.
@bors-servo bors-servo closed this Nov 12, 2014
@bors-servo bors-servo merged commit fbb1e0c into servo:master Nov 12, 2014
1 check passed
1 check passed
default all tests passed
@mrobinson mrobinson mentioned this pull request Nov 12, 2014
Closed
@mrobinson mrobinson deleted the mrobinson:iframes branch Nov 13, 2014
@neojski neojski mentioned this pull request Nov 13, 2014
Closed
@mrobinson mrobinson mentioned this pull request Nov 15, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@mrobinson @hoppipolla-critic-bot @pcwalton @jdm @bors-servo
You can’t perform that action at this time.