use Either type for UrlOrNone #14370

thiagopnts · 2016-11-25T21:01:10Z

Use the Either type for UrlOrNone

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14298 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-11-25T21:01:15Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/shorthand/list.mako.rs, components/style/values/mod.rs, components/style/parser.rs, components/style/values/specified/mod.rs, components/style/properties/helpers.mako.rs, components/style/values/specified/url.rs, components/style/properties/longhand/box.mako.rs
@emilio: components/style/properties/gecko.mako.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/shorthand/list.mako.rs, components/style/values/mod.rs, components/style/parser.rs, components/style/values/specified/mod.rs, components/layout/construct.rs, components/style/properties/helpers.mako.rs, components/style/values/specified/url.rs, components/style/properties/longhand/box.mako.rs

highfive · 2016-11-25T21:01:16Z

Warning

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

emilio · 2016-11-25T23:01:21Z

Hey, thanks for the patch!

Looks good, though you also need to fix the stylo build (./mach test-stylo).

From the error, should be straigh-forward, but feel free to ping me if you have any doubts!

Thanks again! :)

emilio · 2016-11-25T23:04:52Z

(Sorry, I meant to say ./mach build-geckolib, though ./mach test-stylo should pass with that too :)

wafflespeanut · 2016-11-26T03:24:56Z

components/layout/construct.rs

                let image_info = box ImageFragmentInfo::new(node,
                                                            url_value.url().map(|u| u.clone()),
                                                            &self.layout_context.shared);
                vec![Fragment::new(node, SpecificFragmentInfo::Image(image_info), self.layout_context)]
            }
-            list_style_image::T::None => {
+            Either::Second(_) => {


The problem with Either type is that, it doesn't even give you a hint of what it holds under. We should do a workaround like this,

Either::Second(_none) => {

... so that we can convince ourselves that it really holds a None type.

wafflespeanut · 2016-11-26T03:27:30Z

components/style/properties/gecko.mako.rs

        match image {
-            UrlOrNone::None => {
+            Either::Second(_) => {


ditto about _none

wafflespeanut · 2016-11-26T05:34:44Z

components/style/parser.rs

@@ -74,3 +74,7 @@ pub fn log_css_error(input: &mut Parser, position: SourcePosition, message: &str
 pub trait Parse {
    fn parse(input: &mut Parser) -> Result<Self, ()> where Self: Sized;
 }
+
+pub trait ParseWithContext {


This should be a temporary fix. We should probably replace all the types implementing the Parse method to take the context. You can do it if you like, but in case you don't, please drop a FIXME above this, like, FIXME(waffles): Merge this with Parse

What about something like (probably with the traits renamed since we probably want ParseWithContext to be the default):

pub trait Parse : Sized { fn parse(input: &mut Parser) -> Result<Self, ()>; } pub trait ParseWithContext : Sized { fn parse_with_context(input: &mut Parser, context: &ParserContext) -> Result<Self, ()>; } impl<T: Parse> ParseWithContext for T { fn parse_with_context(input: &mut Parser, context: &ParserContext) -> Result<Self, ()> { T::parse(input) } }

In any case agreed, this is kind of offtopic for this PR :)

I've changed the function signature (#14373) to include the context to avoid this problem 😄

@thiagopnts You won't be needing this trait anymore. We'll be using the context in all the parse functions. Please rebase your changes on top of master :)

Cool, I will do the changes

bors-servo · 2016-11-27T05:57:13Z

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

thiagopnts · 2016-11-30T03:23:26Z

@emilio @wafflespeanut I did the changes, let me know if everything is ok so I can squash the commits!

wafflespeanut · 2016-11-30T05:12:51Z

components/layout/construct.rs

@@ -54,6 +54,7 @@ use style::properties::{self, ServoComputedValues};
 use style::selector_parser::{PseudoElement, RestyleDamage};
 use style::servo::restyle_damage::{BUBBLE_ISIZES, RECONSTRUCT_FLOW};
 use style::stylist::Stylist;
+use style::values::{Either, None_};


I don't think the type would be necessary. It's not gonna be used anywhere anyway. Instead, you can just bind it to a name, like so

Either::Second(_none) => (),

wafflespeanut · 2016-11-30T05:13:26Z

components/style/properties/gecko.mako.rs

@@ -1089,10 +1089,10 @@ fn static_assert() {

    #[allow(non_snake_case)]
    pub fn set__moz_binding(&mut self, v: longhands::_moz_binding::computed_value::T) {
-        use properties::longhands::_moz_binding::computed_value::T as BindingValue;
+        use values::{Either, None_};


ditto here and everywhere...

wafflespeanut · 2016-11-30T05:15:06Z

components/style/values/specified/url.rs

@@ -72,6 +73,12 @@ pub struct SpecifiedUrl {
    extra_data: UrlExtraData,
 }

+impl Parse for SpecifiedUrl {
+    fn parse(context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
+        SpecifiedUrl::parse(context, input)


We could just move the parse function here.

wafflespeanut

Once my comments have been addressed, squash the commits, and we'll land it :)


        use Either type for UrlOrNone

fix test-tidy errors fix style unit test use the Parse trait instead of ParseWithContext fix stylo test and geckolib build move all specified url parse code to parse trait and remove refs to unused type

thiagopnts · 2016-12-01T16:09:48Z

@wafflespeanut done :)

wafflespeanut

Thanks for working on this! :)

wafflespeanut · 2016-12-01T16:36:27Z

@bors-servo r+

wafflespeanut · 2016-12-01T16:38:19Z

@bors-servo r+

Wut?

bors-servo · 2016-12-01T16:38:20Z

📌 Commit 206f4ea has been approved by Wafflespeanut

bors-servo · 2016-12-01T16:38:22Z

⌛ Testing commit 206f4ea with merge 8c3b88d...


        Auto merge of #14370 - thiagopnts:master, r=Wafflespeanut

use Either type for UrlOrNone Use the Either type for UrlOrNone ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #14298 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14370)

bors-servo · 2016-12-01T18:16:36Z

💔 Test failed - linux-rel-wpt

wafflespeanut · 2016-12-02T06:03:28Z

@bors-servo retry

Nightmare'ish timeouts!

bors-servo · 2016-12-02T06:03:32Z

⌛ Testing commit 206f4ea with merge 290ff5c...


        Auto merge of #14370 - thiagopnts:master, r=Wafflespeanut

use Either type for UrlOrNone Use the Either type for UrlOrNone ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #14298 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14370)

bors-servo · 2016-12-02T06:54:26Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned asajeffrey Nov 25, 2016

highfive added the S-awaiting-review label Nov 25, 2016

emilio assigned emilio and unassigned asajeffrey Nov 25, 2016

emilio added S-needs-code-changes and removed S-awaiting-review labels Nov 25, 2016

wafflespeanut reviewed Nov 26, 2016

View changes

components/style/properties/gecko.mako.rs Outdated

match image {

UrlOrNone::None => {

Either::Second(_) => {

This comment has been minimized.

Sign in to view

Copy link

wafflespeanut Nov 26, 2016
Member

ditto about _none

wafflespeanut reviewed Nov 26, 2016

View changes

emilio mentioned this pull request Nov 26, 2016

Use the ParserContext along with the Parser #14373

Merged

3 of 3 tasks complete

wafflespeanut added the S-blocked-on-external label Nov 26, 2016

highfive added the S-needs-rebase label Nov 27, 2016

wafflespeanut removed the S-blocked-on-external label Nov 27, 2016

thiagopnts force-pushed the thiagopnts:master branch from 7b14343 to 394f4f7 Nov 29, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 29, 2016

wafflespeanut reviewed Nov 30, 2016

View changes

wafflespeanut requested changes Nov 30, 2016

View changes

wafflespeanut added S-needs-code-changes S-needs-squash and removed S-awaiting-review S-needs-rebase labels Nov 30, 2016

use Either type for UrlOrNone

Loading status checks…

206f4ea

fix test-tidy errors fix style unit test use the Parse trait instead of ParseWithContext fix stylo test and geckolib build move all specified url parse code to parse trait and remove refs to unused type

thiagopnts force-pushed the thiagopnts:master branch from 3ba11a5 to 206f4ea Nov 30, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 30, 2016

wafflespeanut approved these changes Dec 1, 2016

View changes

highfive assigned wafflespeanut and unassigned emilio Dec 1, 2016

highfive removed S-awaiting-review S-needs-squash labels Dec 1, 2016

highfive added the S-awaiting-merge label Dec 1, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 1, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 2, 2016

bors-servo merged commit 206f4ea into servo:master Dec 2, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 2, 2016

bors-servo mentioned this pull request Dec 2, 2016

Reuse box-shadow code for drop-shadow filter #14359

Merged

4 of 5 tasks complete

servo / servo

use Either type for UrlOrNone #14370

use Either type for UrlOrNone #14370

thiagopnts commented Nov 25, 2016 •

edited by larsbergstrom

highfive commented Nov 25, 2016

highfive commented Nov 25, 2016

emilio commented Nov 25, 2016

emilio commented Nov 25, 2016

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.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 27, 2016

thiagopnts commented Nov 30, 2016 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wafflespeanut left a comment

thiagopnts commented Dec 1, 2016

wafflespeanut left a comment

wafflespeanut commented Dec 1, 2016 •

edited

wafflespeanut commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

wafflespeanut commented Dec 2, 2016

bors-servo commented Dec 2, 2016

bors-servo commented Dec 2, 2016

servo / servo

Join GitHub today

use Either type for UrlOrNone #14370

use Either type for UrlOrNone #14370

Conversation

thiagopnts commented Nov 25, 2016 • edited by larsbergstrom

highfive commented Nov 25, 2016

highfive commented Nov 25, 2016

emilio commented Nov 25, 2016

emilio commented Nov 25, 2016

This comment has been minimized.

wafflespeanut Nov 26, 2016 • edited

This comment has been minimized.

wafflespeanut Nov 26, 2016

This comment has been minimized.

wafflespeanut Nov 26, 2016 • edited

This comment has been minimized.

emilio Nov 26, 2016 • edited

This comment has been minimized.

wafflespeanut Nov 26, 2016

This comment has been minimized.

wafflespeanut Nov 27, 2016

This comment has been minimized.

thiagopnts Nov 28, 2016 • edited

bors-servo commented Nov 27, 2016

thiagopnts commented Nov 30, 2016 • edited

This comment has been minimized.

wafflespeanut Nov 30, 2016

This comment has been minimized.

wafflespeanut Nov 30, 2016

This comment has been minimized.

wafflespeanut Nov 30, 2016

wafflespeanut left a comment

thiagopnts commented Dec 1, 2016

wafflespeanut left a comment

wafflespeanut commented Dec 1, 2016 • edited

wafflespeanut commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

wafflespeanut commented Dec 2, 2016

bors-servo commented Dec 2, 2016

bors-servo commented Dec 2, 2016

thiagopnts commented Nov 25, 2016 •

edited by larsbergstrom

wafflespeanut Nov 26, 2016 •

edited

wafflespeanut Nov 26, 2016 •

edited

emilio Nov 26, 2016 •

edited

thiagopnts Nov 28, 2016 •

edited

thiagopnts commented Nov 30, 2016 •

edited

wafflespeanut commented Dec 1, 2016 •

edited