Implement <meta name=viewport> handling #6185

luniv · 2015-05-26T04:39:08Z

Translate as according to CSS Device Adaption § 9

Note: as the PR currently stands, handling <meta name=viewport> elements always occurs. This is probably not desired for some contexts (e.g. desktop), but I'm unsure of how to conditionally handle elements based on that.

highfive · 2015-05-26T04:39:11Z

Warning

These commits modify unsafe code. Please review it carefully!

hoppipolla-critic-bot · 2015-05-26T04:39:13Z

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

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 · 2015-05-26T18:07:14Z

components/style/viewport.rs

+        let mut iter = content.chars().enumerate();
+
+        macro_rules! start_of_name {
+            () => {


I think I'd prefer passing in iter as an argument to the macro to make it clear what variable it's using.

pcwalton · 2015-05-26T18:07:27Z

components/style/viewport.rs

+        }
+
+        macro_rules! skip_whitespace {
+            () => {


pcwalton · 2015-05-26T18:07:32Z

components/style/viewport.rs

+                               -> Option<(&'a str, &'a str)>
+    {
+        macro_rules! end_of_token {
+            () => {


pcwalton · 2015-05-26T18:08:40Z

components/style/viewport.rs

+                match value.parse::<f32>() {
+                    Ok(n) if n >= 0. => ViewportLength::Length(Length::from_px(n.max(1.).min(10000.))),
+                    Ok(n) if n < 0. => return None,
+                    _ => ViewportLength::Length(Length::from_px(1.))


nit: Might be clearer as Ok(n) if n >= 0. => ..., Ok(n) => ..., Err(_) => ... ?

pcwalton · 2015-05-26T18:09:15Z

components/script/dom/htmlmetaelement.rs

+            let name = name.r().value();
+            if !name.is_empty() {
+                match &**name {
+                    "viewport" => self.translate_viewport(),


Is this supposed to be case-sensitive?

pcwalton · 2015-05-26T18:09:59Z

We should file some kind of follow-up for proper handling of dynamic removal of <meta name="viewport"> elements.

luniv · 2015-05-26T18:41:26Z

Review status: 0 of 9 files reviewed, 5 unresolved discussions, all commit checks successful.

^{components/script/dom/htmlmetaelement.rs, line 65 [r2] (raw file):}
No, it should be case-insensitive.

Comments from the review on Reviewable.io

luniv · 2015-05-27T18:41:41Z

Pushed two fixup commits to address comments

metajack · 2015-06-01T16:42:48Z

@pcwalton Waiting for updated review.

bors-servo · 2015-06-19T04:33:37Z

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

metajack · 2015-07-29T19:01:37Z

@pcwalton This has been waiting on you now for 2 months.

luniv · 2015-09-19T04:37:42Z

Rebased, and converted the two macros to functions

bors-servo · 2015-09-19T23:46:04Z

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

nox · 2015-09-22T18:18:59Z

@pcwalton Ping?

bors-servo · 2015-09-24T00:01:14Z

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

bors-servo · 2015-09-30T17:09:49Z

📌 Commit ffc1329 has been approved by mbrubeck

bors-servo · 2015-09-30T17:38:30Z

⌛ Testing commit ffc1329 with merge 3795054...


        Auto merge of #6185 - luniv:viewport-meta, r=mbrubeck

Implement <meta name=viewport> handling Translate <meta name=viewport> as according to [CSS Device Adaption § 9](http://dev.w3.org/csswg/css-device-adapt/#viewport-meta) Note: as the PR currently stands, handling `<meta name=viewport>` elements always occurs. This is probably not desired for some contexts (e.g. desktop), but I'm unsure of how to conditionally handle elements based on that.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6185)

bors-servo · 2015-09-30T17:49:52Z

💔 Test failed - linux-dev

mbrubeck · 2015-09-30T17:52:14Z

It looks like the unit tests were bit-rotted by an unrelated change:

   Compiling style_tests v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/components/servo)
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:47:13: 47:48 error: use of unstable library feature 'core_intrinsics': intrinsics are unlikely to ever be stabilized, instead they should be used through stabilized interfaces in the rest of the standard library (see issue #0)
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:47         use std::intrinsics::discriminant_value;
                                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:46:5: 60:6 note: in this expansion of if let expansion
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:47:13: 47:48 help: add #![feature(core_intrinsics)] to the crate attributes to enable
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:52:30: 52:48 error: use of unstable library feature 'core_intrinsics': intrinsics are unlikely to ever be stabilized, instead they should be used through stabilized interfaces in the rest of the standard library (see issue #0)
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:52             let a = unsafe { discriminant_value(&a.descriptor) };
                                                                                                        ^~~~~~~~~~~~~~~~~~
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:46:5: 60:6 note: in this expansion of if let expansion
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:52:30: 52:48 help: add #![feature(core_intrinsics)] to the crate attributes to enable
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:53:30: 53:48 error: use of unstable library feature 'core_intrinsics': intrinsics are unlikely to ever be stabilized, instead they should be used through stabilized interfaces in the rest of the standard library (see issue #0)
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:53             let b = unsafe { discriminant_value(&b.descriptor) };
                                                                                                        ^~~~~~~~~~~~~~~~~~
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:46:5: 60:6 note: in this expansion of if let expansion
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/viewport.rs:53:30: 53:48 help: add #![feature(core_intrinsics)] to the crate attributes to enable
error: aborting due to 3 previous errors
Could not compile `style_tests`.

luniv · 2015-09-30T18:36:36Z

That'd be my fault actually. I noticed the compiler complaining that the feature flag was unused and removed it in the final squash. I'll add it back in.

mbrubeck · 2015-09-30T18:39:07Z

That'd be my fault actually. I noticed the compiler complaining that the feature flag was unused and removed it in the final squash.

You can add it with #![cfg_attr(test, feature(...))] to avoid the unused feature warning, if it's only used in tests.

bors-servo · 2015-09-30T23:44:03Z

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


        Implement CSS Device Adaption § 9 (Viewport <META> element)

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


        Handle <meta name=viewport> elements when added to document

luniv · 2015-10-01T03:35:34Z

Rebased and fixed the missing feature flag

mbrubeck · 2015-10-01T04:31:33Z

@bors-servo r+

bors-servo · 2015-10-01T04:31:34Z

📌 Commit d0ace58 has been approved by mbrubeck

bors-servo · 2015-10-01T04:49:43Z

⌛ Testing commit d0ace58 with merge a774305...


        Auto merge of #6185 - luniv:viewport-meta, r=mbrubeck

Implement <meta name=viewport> handling Translate <meta name=viewport> as according to [CSS Device Adaption § 9](http://dev.w3.org/csswg/css-device-adapt/#viewport-meta) Note: as the PR currently stands, handling `<meta name=viewport>` elements always occurs. This is probably not desired for some contexts (e.g. desktop), but I'm unsure of how to conditionally handle elements based on that.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6185)

bors-servo · 2015-10-01T05:17:36Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label May 26, 2015

pcwalton reviewed May 26, 2015
View changes

components/style/viewport.rs Outdated

}

macro_rules! skip_whitespace {

() => {

This comment has been minimized.

Sign in to view

pcwalton May 26, 2015
Contributor

Same here.

pcwalton reviewed May 26, 2015
View changes

components/style/viewport.rs Outdated

-> Option<(&'a str, &'a str)>

{

macro_rules! end_of_token {

() => {

This comment has been minimized.

Sign in to view

pcwalton May 26, 2015
Contributor

Same here.

pcwalton reviewed May 26, 2015
View changes

components/script/dom/htmlmetaelement.rs Outdated

let name = name.r().value();

if !name.is_empty() {

match &**name {

"viewport" => self.translate_viewport(),

This comment has been minimized.

Sign in to view

pcwalton May 26, 2015
Contributor

Is this supposed to be case-sensitive?

metajack assigned pcwalton Jun 1, 2015

metajack added the S-needs-rebase label Jul 29, 2015

nox removed the S-awaiting-review label Sep 13, 2015

luniv force-pushed the luniv:viewport-meta branch from 7b44f3a to 437bd77 Sep 19, 2015

highfive added the S-awaiting-review label Sep 19, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 19, 2015

luniv force-pushed the luniv:viewport-meta branch from 43c11f5 to d8358d0 Sep 20, 2015

nox removed the S-needs-rebase label Sep 22, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 24, 2015

luniv force-pushed the luniv:viewport-meta branch from d8358d0 to f9988c3 Sep 24, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 30, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 30, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 30, 2015

luniv added 2 commits May 21, 2015

Implement CSS Device Adaption § 9 (Viewport <META> element)

61f7a0a

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

Handle <meta name=viewport> elements when added to document

Loading status checks…

d0ace58

luniv force-pushed the luniv:viewport-meta branch from ffc1329 to d0ace58 Oct 1, 2015

highfive added S-awaiting-review and removed S-tests-failed labels Oct 1, 2015

jdm removed the S-needs-rebase label Oct 1, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 1, 2015

bors-servo merged commit d0ace58 into servo:master Oct 1, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

luniv deleted the luniv:viewport-meta branch Oct 1, 2015

servo / servo

Implement <meta name=viewport> handling #6185

Implement <meta name=viewport> handling #6185

luniv commented May 26, 2015

highfive commented May 26, 2015

hoppipolla-critic-bot commented May 26, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton commented May 26, 2015

luniv commented May 26, 2015

luniv commented May 27, 2015

metajack commented Jun 1, 2015

bors-servo commented Jun 19, 2015

metajack commented Jul 29, 2015

luniv commented Sep 19, 2015

bors-servo commented Sep 19, 2015

nox commented Sep 22, 2015

bors-servo commented Sep 24, 2015

bors-servo commented Sep 30, 2015

bors-servo commented Sep 30, 2015

bors-servo commented Sep 30, 2015

mbrubeck commented Sep 30, 2015

luniv commented Sep 30, 2015

mbrubeck commented Sep 30, 2015

bors-servo commented Sep 30, 2015

luniv commented Oct 1, 2015

mbrubeck commented Oct 1, 2015

bors-servo commented Oct 1, 2015

bors-servo commented Oct 1, 2015

bors-servo commented Oct 1, 2015

servo / servo

Join GitHub today

Implement <meta name=viewport> handling #6185

Implement <meta name=viewport> handling #6185

Conversation

luniv commented May 26, 2015

highfive commented May 26, 2015

hoppipolla-critic-bot commented May 26, 2015

This comment has been minimized.

pcwalton May 26, 2015

This comment has been minimized.

pcwalton May 26, 2015

This comment has been minimized.

pcwalton May 26, 2015

This comment has been minimized.

pcwalton May 26, 2015

This comment has been minimized.

pcwalton May 26, 2015

pcwalton commented May 26, 2015

luniv commented May 26, 2015

luniv commented May 27, 2015

metajack commented Jun 1, 2015

bors-servo commented Jun 19, 2015

metajack commented Jul 29, 2015

luniv commented Sep 19, 2015

bors-servo commented Sep 19, 2015

nox commented Sep 22, 2015

bors-servo commented Sep 24, 2015

bors-servo commented Sep 30, 2015

bors-servo commented Sep 30, 2015

bors-servo commented Sep 30, 2015

mbrubeck commented Sep 30, 2015

luniv commented Sep 30, 2015

mbrubeck commented Sep 30, 2015

bors-servo commented Sep 30, 2015

luniv commented Oct 1, 2015

mbrubeck commented Oct 1, 2015

bors-servo commented Oct 1, 2015

bors-servo commented Oct 1, 2015

bors-servo commented Oct 1, 2015