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

Implement the HTML background attribute #5851

Merged
merged 2 commits into from Jul 29, 2015
Merged

Conversation

@evilpie
Copy link
Contributor

evilpie commented Apr 26, 2015

Fixes #5835

Review on Reviewable

@highfive
Copy link

highfive commented Apr 26, 2015

warning Warning warning

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

hoppipolla-critic-bot commented Apr 26, 2015

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

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.

@evilpie
Copy link
Contributor Author

evilpie commented Apr 26, 2015

I doubt you will merge it like that. get_background should probably be moved from HTMLBodyElementHelpers to something that is layout specific. If there was some way to get the document base inside legacy.rs everything would be a lot easier.

@SimonSapin
Copy link
Member

SimonSapin commented Apr 27, 2015

components/script/dom/htmlbodyelement.rs, line 86 [r1] (raw file):
Can this .clone() be avoided? Maybe the method would have to return std::cell::Ref<Option<Url>>.


components/script/dom/htmlbodyelement.rs, line 135 [r1] (raw file):
Use .ok() to get an Option from a Result.



Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

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

@SimonSapin
Copy link
Member

SimonSapin commented May 22, 2015

@Ms2ger, what do you think is the right way to access an element’s base URL from legacy.rs?

@Ms2ger
Copy link
Contributor

Ms2ger commented May 22, 2015

There shouldn't be anything in legacy.rs. We don't support base, so I guess document_from_node(element).url() or something like that.

@waddlesplash
Copy link

waddlesplash commented May 26, 2015

@evilpie Any update on this? If you're unable to continue, I might be able to spare some time to complete it...

@evilpie
Copy link
Contributor Author

evilpie commented May 26, 2015

I am going to continue working on this soon.

@SimonSapin
Copy link
Member

SimonSapin commented May 26, 2015

There shouldn't be anything in legacy.rs.

If I understand correctly, you mean that this should use the new infrastructure for presentational hints where the work happens in components/script/dom/element.rs rather than in legacy.rs, as in #5988. Is that right?

@SimonSapin
Copy link
Member

SimonSapin commented May 26, 2015

@Ms2ger

@evilpie evilpie force-pushed the evilpie:background branch from 3ab2a07 to 18939e6 May 26, 2015
@evilpie
Copy link
Contributor Author

evilpie commented May 26, 2015

Sorry, I am not quite sure how to use Ref<>.

@jdm jdm removed the S-needs-rebase label May 26, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Jun 19, 2015

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

@evilpie evilpie force-pushed the evilpie:background branch from 83b8045 to d3f92eb Jun 20, 2015
@evilpie
Copy link
Contributor Author

evilpie commented Jun 20, 2015

I rebased this again. I still not sure how to use Ref<>.

@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

@SimonSapin
Copy link
Member

SimonSapin commented Jul 29, 2015

@bors-servo r+


Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/htmlbodyelement.rs, line 102 [r1] (raw file):
On further reading, this is used to create a BackgroundImage which wants to own an Url anyway, so removing the clone here would only move it.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jul 29, 2015

📌 Commit d3f92eb has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jul 29, 2015

Testing commit d3f92eb with merge 58fa3b6...

bors-servo pushed a commit that referenced this pull request Jul 29, 2015
Implement the HTML background attribute

Fixes #5835

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

bors-servo commented Jul 29, 2015

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

@bors-servo bors-servo merged commit d3f92eb into servo:master Jul 29, 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.

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