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

unrendered content uses doc's background color #1210

Merged
merged 1 commit into from Nov 24, 2013

Conversation

@ibnc
Copy link

ibnc commented Nov 10, 2013

fixes #140

This shouldn't be merged until the appropriate change is made in rust layers (servo/rust-layers#37). I'm making a pull request anyway to start getting some feedback.

@ibnc
Copy link
Author

ibnc commented Nov 11, 2013

Thanks for the feedback on the weekend!

SetUnRenderedColor(id, color) => {
match compositor_layer {
Some(ref mut layer) => {
layer.unrendered_color = color;

This comment has been minimized.

@eschweic

eschweic Nov 11, 2013

This isn't quite correct in the case of iframes. This will set the base compositor layer correctly, but if an iframe has unrendered content, the background color will be sent to the root's compositor layer instead the iframe's.

Instead, you should add logic to compositor_layer.rs that will recursively look for the iframe with the correct id that was passed in via this message, and only set the background color if the ids match; otherwise, recurse on the layer's children. Then, call that method here rather than setting the layer's color directly.

You might also need to set recomposite to true so the updates will immediately be displayed to the screen.

scene.background_color.r = layer.unrendered_color.r;
scene.background_color.g = layer.unrendered_color.g;
scene.background_color.b = layer.unrendered_color.b;
scene.background_color.a = layer.unrendered_color.a;

This comment has been minimized.

@eschweic

eschweic Nov 11, 2013

Ah, I see, right now setting the clear color will only work with the root layer anyway. Getting an unrendered color to appear for iframes is going to take a bit more work. I would suggest leaving this as is, but creating an issue.

@metajack
Copy link
Contributor

metajack commented Nov 13, 2013

@ibnc Can you file the issue that @eschweic recommended and we'll get this merged.

bors-servo pushed a commit that referenced this pull request Nov 13, 2013
fixes #140

This shouldn't be merged until the appropriate change is made in rust layers (servo/rust-layers#37). I'm making a pull request anyway to start getting some feedback.
@ibnc
Copy link
Author

ibnc commented Nov 13, 2013

@metajack, totally can open that issue.

@jdm How exactly do I do that?

@jdm
Copy link
Member

jdm commented Nov 13, 2013

Set up a branch on rust-layers that is pointing at servo/rust-layers@390b1d3, then stage the modified submodule and amend this commit.

@metajack
Copy link
Contributor

metajack commented Nov 13, 2013

Also, once you've got your rust-layers update commit, can you squash all three into a single one?

@ibnc
Copy link
Author

ibnc commented Nov 13, 2013

@jdm, I think I know what you mean, but I also think I'm a little lost. I'm running git branch -f sbc 390b1d3e318fbac385b3ba16f377b25b79b8af29 and am getting an error saying "Not a valid branch point."

@metajack, Will do. Should I rebase from upstream as well?

@metajack
Copy link
Contributor

metajack commented Nov 13, 2013

@ibnc You can if you want, though I think this branch is still mergeable.

To do the rust-layers update, just switch into src/support/layers/rust-layers and git checkout master and git pull I think. That should pull in your changes. Then back in the servo repo you'll need to git add src/support/layers/rust-layers to stage the submodule pointer update.

@ibnc
Copy link
Author

ibnc commented Nov 13, 2013

@metajack, should I just cherry pick the commit, servo/rust-layers@390b1d3, from rust-layers, or should I just go ahead and update from the current master branch?

@ibnc
Copy link
Author

ibnc commented Nov 13, 2013

Ah, I think I broke this.

getting "No rule to make target layers.rc', needed bylibrustlayers.dummy'. Stop." when I try to compile now.

@ibnc
Copy link
Author

ibnc commented Nov 14, 2013

It seems the layers.rc was replaced by lib.rs for rustpkg, I presume. Where do I reconfigure this to use lib.rs?

@jdm
Copy link
Member

jdm commented Nov 14, 2013

Does it work if you run the configure script from your build directory again?

@ibnc
Copy link
Author

ibnc commented Nov 14, 2013

I ran it again without a cleaning up. I'm going to do a clean build to see if I get the error again.

@kmcallister
Copy link
Contributor

kmcallister commented Nov 14, 2013

@ibnc: Sorry, you'll need my rustpkg updates to Servo before you can build it with master rust-layers. I'm about to submit a PR for that.

@ibnc
Copy link
Author

ibnc commented Nov 14, 2013

@kmcallister ah, okay. I'll sit tight then

@metajack
Copy link
Contributor

metajack commented Nov 14, 2013

You'll also need to rebase now unfortunately. It should be a straightforward one.

I just approved @kmcallister's PR so it will hopefully land shortly.

@ibnc
Copy link
Author

ibnc commented Nov 22, 2013

I've rebased, and I am getting an issue with rustpkg and rust-geom saying:

note: Couldn't infer any crates to build.
Try naming a crate 'main.rs', 'lib.rs', 'test.rs', or 'bench.rs'.
task '<unnamed>' failed at 'Unhandled condition: missing_pkg_files: package_id::PkgId{path: std::path::posix::Path{repr: ~[114u8, 117u8, 115u8, 116u8, 45u8, 103u8, 101u8, 111u8, 109u8], sepidx: None}, short_name: ~"rust-geom", version: NoVersion}',  servo_dev/src/compiler/rust/src/libstd/condition.rs:131

The thing is there is an lib.rs file under src/support/geom/rust-geom.

@metajack
Copy link
Contributor

metajack commented Nov 22, 2013

I'll pull your branch and try tomorrow.

@ibnc
Copy link
Author

ibnc commented Nov 23, 2013

@metajack any luck?

  uses html's background color, or body's.
@ibnc
Copy link
Author

ibnc commented Nov 24, 2013

Rebased.

I still can't quite figure out why rustpkg isn't building rust-geom, but no one else seems to be having the issue; so, I figure it's my local environment in some way, and that the tests should pass with bors.

If everything looks good, then would you mind trying to merge @metajack?

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on 9886135 Nov 24, 2013

saw approval from jdm
at ibnc@9886135

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 24, 2013

merging ibnc/servo/resizing_background_color = 9886135 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 24, 2013

ibnc/servo/resizing_background_color = 9886135 merged ok, testing candidate = 010b8cd

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 24, 2013

fast-forwarding master to auto = 010b8cd

bors-servo pushed a commit that referenced this pull request Nov 24, 2013
fixes #140

This shouldn't be merged until the appropriate change is made in rust layers (servo/rust-layers#37). I'm making a pull request anyway to start getting some feedback.
@bors-servo bors-servo closed this Nov 24, 2013
@bors-servo bors-servo merged commit 9886135 into servo:master Nov 24, 2013
1 check passed
1 check passed
default all tests passed
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.

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