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

Implement CSS Device Adaption Level 1 #5361

Merged
merged 6 commits into from May 6, 2015
Merged

Implement CSS Device Adaption Level 1 #5361

merged 6 commits into from May 6, 2015

Conversation

@luniv
Copy link
Contributor

luniv commented Mar 25, 2015

Spec: http://dev.w3.org/csswg/css-device-adapt/

Currently, the actual viewport is used by the layout task as part of the reflow, and the compositor uses the zoom constraints. I'm not sure if anywhere else currently needs access to the constraints (i.e. there's no CSSOM as far as I can tell).

I did not implement sections 9 (viewport ) or 10 (handling 'auto' for 'zoom').

Review on Reviewable
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Mar 25, 2015

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

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.
@pcwalton pcwalton force-pushed the servo:master branch from 7f587f6 to b1a35f5 Mar 25, 2015
@metajack
Copy link
Contributor

metajack commented Mar 31, 2015

Assigning to @mbrubeck.
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 2, 2015

I can review this, but I'd also like feedback from @SimonSapin on the FromCss trait.
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 8, 2015

Sorry for taking so long to review this. The code looks great! I left a handful of comments in Critic. Also, this will need to be rebased, sorry.
@luniv luniv force-pushed the luniv:css-device-adapt branch from e0feabd to 88938a3 Apr 8, 2015
@mbrubeck mbrubeck removed the S-needs-rebase label Apr 8, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 9, 2015

My review is complete. This all looks good to me, but I'm still hoping that @SimonSapin can take a quick look at the code, especially for any stylistic or architectural issues.
@metajack metajack assigned SimonSapin and unassigned mbrubeck Apr 9, 2015
@metajack
Copy link
Contributor

metajack commented Apr 9, 2015

Reassigning to @SimonSapin while we wait for his feedback.
@luniv
Copy link
Contributor Author

luniv commented Apr 10, 2015

Should I squash the various fix-up commits now then?
@jdm
Copy link
Member

jdm commented Apr 10, 2015

That will break the Critic tool; best to leave that until after the review is complete.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 11, 2015

☔ The latest upstream changes (presumably #5553) made this pull request unmergeable. Please resolve the merge conflicts.
@luniv luniv force-pushed the luniv:css-device-adapt branch from c970258 to de5e586 Apr 11, 2015
@luniv
Copy link
Contributor Author

luniv commented Apr 11, 2015

Re-based to take in account #5553 (CSS rule iterators)
@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2015

☔ The latest upstream changes (presumably #5610) made this pull request unmergeable. Please resolve the merge conflicts.
@luniv luniv force-pushed the luniv:css-device-adapt branch from de5e586 to 0a37761 Apr 17, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2015

☔ The latest upstream changes (presumably #5739) made this pull request unmergeable. Please resolve the merge conflicts.
@luniv luniv force-pushed the luniv:css-device-adapt branch from 0a37761 to c7945b5 Apr 21, 2015
@SimonSapin
Copy link
Member

SimonSapin commented Apr 21, 2015

I’ve filed some minor issues on Critic.
@SimonSapin
Copy link
Member

SimonSapin commented May 6, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

📌 Commit 7f2ea20 has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

⌛ Testing commit 7f2ea20 with merge acbaf45...
bors-servo pushed a commit that referenced this pull request May 6, 2015
bors-servo
Auto merge of #5361 - luniv:css-device-adapt, r=SimonSapin
acbaf45 
Spec: http://dev.w3.org/csswg/css-device-adapt/

Currently, the actual viewport is used by the layout task as part of the reflow, and the compositor uses the zoom constraints. I'm not sure if anywhere else currently needs access to the constraints (i.e. there's no CSSOM as far as I can tell).

I did not implement sections 9 (viewport <META>) or 10 (handling 'auto' for 'zoom').

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

SimonSapin commented May 6, 2015

@luniv You may want to comment on the PR when pushing changes, we don’t get notified otherwise!
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

💔 Test failed - mac2
@jdm
Copy link
Member

jdm commented May 6, 2015 

failures:
    iframe/hide_layers1.html == iframe/hide_layers_ref.html
    viewport_rule.html == viewport_rule_ref.html

hide_layers1.html is #5950, but the other is new.
@jdm jdm added S-tests-failed and removed S-awaiting-merge labels May 6, 2015
luniv added 2 commits Apr 8, 2015
@luniv
Move parsing of @Viewport rules in a stylesheet behind --experimental.
3f006f9 
This is for parsing of the rule within a stylesheet only. <meta name=viewport> could create a viewport rule and add it to the list of stylesheets for the page (like quirks mode).
@luniv
Add a basic ref test for @Viewport
Loading status checks…
455ac53
@luniv luniv force-pushed the luniv:css-device-adapt branch from 7f2ea20 to 455ac53 May 6, 2015
@luniv
Copy link
Contributor Author

luniv commented May 6, 2015

@SimonSapin Oh OK, thanks for the heads up.

Looks like the actual HTML files for the ref test got lost in the shuffle. I've added them back (and moved adding the ref test to a separate commit)
@mbrubeck
Copy link
Contributor

mbrubeck commented May 6, 2015

Reviewed files:

  • tests/ref/viewport_rule.html @ r8
  • tests/ref/viewport_rule_ref.html @ r8

Comments from the review on Reviewable.io
@mbrubeck
Copy link
Contributor

mbrubeck commented May 6, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

📌 Commit 455ac53 has been approved by mbrubeck
bors-servo pushed a commit that referenced this pull request May 6, 2015
bors-servo
Auto merge of #5361 - luniv:css-device-adapt, r=mbrubeck
Loading status checks…
ccf1e6b 
Spec: http://dev.w3.org/csswg/css-device-adapt/

Currently, the actual viewport is used by the layout task as part of the reflow, and the compositor uses the zoom constraints. I'm not sure if anywhere else currently needs access to the constraints (i.e. there's no CSSOM as far as I can tell).

I did not implement sections 9 (viewport <META>) or 10 (handling 'auto' for 'zoom').

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

bors-servo commented May 6, 2015

⌛ Testing commit 455ac53 with merge ccf1e6b...
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2
@bors-servo bors-servo merged commit 455ac53 into servo:master May 6, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
This was referenced May 6, 2015
Merged
Merged
@luniv luniv deleted the luniv:css-device-adapt branch May 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@luniv @hoppipolla-critic-bot @metajack @mbrubeck @jdm @bors-servo @SimonSapin @highfive
You can’t perform that action at this time.