stylo: store specified value of grid layout repeat() function #18206

ferjm · 2017-08-23T20:12:15Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Bug 1382369

This change is

highfive · 2017-08-23T20:12:20Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/values/specified/grid.rs, components/style/values/generics/grid.rs, components/style/gecko/conversions.rs, components/style/properties/shorthand/position.mako.rs
@canaltinova: components/style/properties/gecko.mako.rs, components/style/values/specified/grid.rs, components/style/values/generics/grid.rs, components/style/gecko/conversions.rs, components/style/properties/shorthand/position.mako.rs
@emilio: components/style/properties/gecko.mako.rs, components/style/values/specified/grid.rs, components/style/values/generics/grid.rs, components/style/gecko/conversions.rs, components/style/properties/shorthand/position.mako.rs

highfive · 2017-08-23T20:12:21Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

ferjm · 2017-08-23T20:12:49Z

Waiting for a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce7e06023cd900772347d9977cfa8c65fd294b3a

emilio

Looks fine overall, sorry for the lag in getting here.

Maybe @canaltinova also wants to take a quick look?

emilio · 2017-08-25T06:24:49Z

components/style/values/specified/grid.rs

@@ -78,6 +78,17 @@ impl Parse for TrackSize<LengthOrPercentage> {
    }
 }

+impl HasViewportPercentage for TrackListValue<LengthOrPercentage> {


You can use derive(HasViewportPercentage).

emilio · 2017-08-25T06:24:49Z

components/style/values/specified/grid.rs

+        };
+        for (pos, names) in self.line_names.iter().enumerate() {
+            prev_names.extend_from_slice(&names);
+            if (pos >= self.values.len()) || (pos == auto_idx) {


nit: no need for parentheses here.

emilio · 2017-08-25T06:24:49Z

components/style/values/generics/grid.rs

+    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
+        match *self {
+            TrackListValue::TrackSize(ref size) => TrackListValue::TrackSize(size.to_computed_value(context)),
+            _ => unreachable!("Should only compute track-size values"),


hmm... this looks somewhat suspicious... Maybe something should be refactored here? Maybe it's worth to just make it idempotent (in which case you can use derive(ToComputedValue).

emilio · 2017-08-25T06:24:49Z

components/style/values/generics/grid.rs

+impl<L: ToCss> ToCss for TrackListValue<L> {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        match *self {
+            TrackListValue::TrackSize(ref size) => size.to_css(dest),


I'm moderately sure you can derive(ToCss) here and remove this code.

ferjm · 2017-08-25T08:33:46Z

Thank you Emilio! It seems that the patch breaks some of the existing tests. I'll see if I can fix it.

bors-servo · 2017-08-28T16:19:17Z

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

ferjm · 2017-08-29T18:21:53Z

New try build with some fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ad97987b5b37a2f5cd769e3208981ea87f3976&selectedJob=126798800

ferjm · 2017-08-31T13:48:20Z

No failures on try, so I think this is good to go.

r? @canaltinova and/or @emilio

Thank you!

bors-servo · 2017-08-31T15:45:05Z

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

ferjm · 2017-09-04T14:10:44Z

The travisCI error seems to be #18346.

wafflespeanut

Sorry, didn't look into this earlier. Overall, this looks good to me. r=me after the comment

wafflespeanut · 2017-09-05T10:15:29Z

components/style/values/generics/grid.rs

+/// Track list values. Can be <track-size> or <track-repeat>
+#[derive(Clone, Debug, PartialEq, ToComputedValue, ToCss)]
+#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+pub enum TrackListValue<T> {


We could use Either here - not sure whether it's particularly useful, but that's what I did on 26540df 😄

TBH I would prefer to keep the current enum if you don't mind. Either syntax feels less clear than an explicit enum, specially since we need to do things like https://github.com/servo/servo/pull/18206/files#diff-ea526b2e68fb17c4d9af5ce82d3cc64eR899

wafflespeanut · 2017-09-05T10:15:53Z

components/style/values/specified/grid.rs

+                let vec = mem::replace(&mut current_names, vec![]);
+                names.push(vec.into_boxed_slice());
+                if let RepeatCount::Number(num) = repeat.count {
+                    auto_offset += (num.value() - 1) as u16;


This isn't obvious. Please drop a comment about this 😄

Done. I added the comment to the binding definition.


        stylo: store specified value of grid layout repeat() function

ferjm · 2017-09-11T07:32:35Z

Mochitests changes has been approved. So this is ready to merge.

@bors-servo: r=wafflespeanut

bors-servo · 2017-09-11T07:32:35Z

📌 Commit 8306946 has been approved by wafflespeanut

bors-servo · 2017-09-11T07:32:40Z

⌛ Testing commit 8306946 with merge 8129cf5...


        Auto merge of #18206 - ferjm:bug1382369.grid.repeat.function, r=waffl…

…espeanut stylo: store specified value of grid layout repeat() function - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Bug 1382369](https://bugzilla.mozilla.org/show_bug.cgi?id=1382369)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18206)

bors-servo · 2017-09-11T09:20:02Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: wafflespeanut
Pushing 8129cf5 to master...

highfive assigned emilio Aug 23, 2017

highfive added the S-awaiting-review label Aug 23, 2017

emilio reviewed Aug 25, 2017

View changes

highfive added the S-needs-rebase label Aug 28, 2017

ferjm force-pushed the ferjm:bug1382369.grid.repeat.function branch from 831bd25 to 13637ef Aug 31, 2017

highfive assigned canova and unassigned emilio Aug 31, 2017

highfive added the S-needs-rebase label Aug 31, 2017

ferjm force-pushed the ferjm:bug1382369.grid.repeat.function branch from 13637ef to 23fcbc7 Sep 1, 2017

wafflespeanut added S-fails-tidy S-fails-travis and removed S-fails-tidy labels Sep 3, 2017

ferjm removed S-fails-travis S-needs-rebase labels Sep 4, 2017

wafflespeanut approved these changes Sep 5, 2017

View changes

wafflespeanut reviewed Sep 5, 2017

View changes

stylo: store specified value of grid layout repeat() function

Loading status checks…

8306946

ferjm force-pushed the ferjm:bug1382369.grid.repeat.function branch from 23fcbc7 to 8306946 Sep 6, 2017

highfive unassigned canova Sep 11, 2017

highfive assigned wafflespeanut Sep 11, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 11, 2017

bors-servo merged commit 8306946 into servo:master Sep 11, 2017
2 of 3 checks passed

2 of 3 checks passed

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

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 11, 2017

bors-servo mentioned this pull request Sep 11, 2017

style: Remove a few uses of ComputedValueAsSpecified. #18429

Merged

servo / servo

stylo: store specified value of grid layout repeat() function #18206

stylo: store specified value of grid layout repeat() function #18206

ferjm commented Aug 23, 2017 •

edited by SimonSapin

highfive commented Aug 23, 2017

highfive commented Aug 23, 2017

ferjm commented Aug 23, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Aug 25, 2017

bors-servo commented Aug 28, 2017

ferjm commented Aug 29, 2017

ferjm commented Aug 31, 2017

bors-servo commented Aug 31, 2017

ferjm commented Sep 4, 2017

wafflespeanut left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

servo / servo

Join GitHub today

stylo: store specified value of grid layout repeat() function #18206

stylo: store specified value of grid layout repeat() function #18206

Conversation

ferjm commented Aug 23, 2017 • edited by SimonSapin

highfive commented Aug 23, 2017

highfive commented Aug 23, 2017

ferjm commented Aug 23, 2017

emilio left a comment

This comment has been minimized.

emilio Aug 25, 2017

This comment has been minimized.

emilio Aug 25, 2017

This comment has been minimized.

emilio Aug 25, 2017

This comment has been minimized.

emilio Aug 25, 2017

ferjm commented Aug 25, 2017

bors-servo commented Aug 28, 2017

ferjm commented Aug 29, 2017

ferjm commented Aug 31, 2017

bors-servo commented Aug 31, 2017

ferjm commented Sep 4, 2017

wafflespeanut left a comment

This comment has been minimized.

wafflespeanut Sep 5, 2017

This comment has been minimized.

ferjm Sep 6, 2017 • edited

This comment has been minimized.

wafflespeanut Sep 5, 2017 • edited

This comment has been minimized.

ferjm Sep 6, 2017

ferjm commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

ferjm commented Aug 23, 2017 •

edited by SimonSapin

ferjm Sep 6, 2017 •

edited

wafflespeanut Sep 5, 2017 •

edited