style: Move border-image-repeat outside of mako. #19210

chansuke · 2017-11-14T05:12:37Z

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

This change is

emilio

This has a couple of build errors, but it's a good start!

Let me know if I can help fixing them somehow.

emilio · 2017-11-14T13:33:20Z

components/style/values/computed/border.rs

+    where
+        W: fmt::Write,
+    {
+        match (self.0, self.1) {


There's no need for match here, just:

let BorderImageRepeat(horizontal, vertical) = *self;

This comment still applies.

emilio · 2017-11-14T13:33:20Z

components/style/values/computed/border.rs

+
+    #[inline]
+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        match (computed) {


Thid doesn't build does it?

This is probably still not building, let me know if you need any help setting up the build environment or if I can help somehow, I'd be glad to :)

highfive · 2017-11-14T17:56:00Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

highfive · 2017-11-14T17:56:01Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/values/specified/border.rs, components/style/values/computed/border.rs, components/style/values/computed/mod.rs, components/style/properties/longhand/border.mako.rs
@canaltinova: components/style/properties/gecko.mako.rs, components/style/values/specified/border.rs, components/style/values/computed/border.rs, components/style/values/computed/mod.rs, components/style/properties/longhand/border.mako.rs
@emilio: components/layout/display_list_builder.rs, components/style/properties/gecko.mako.rs, components/style/values/specified/border.rs, components/style/values/computed/border.rs, components/style/values/computed/mod.rs and 1 more

highfive · 2017-11-14T17:56:02Z

Warning

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

chansuke · 2017-11-16T10:46:45Z

@emilio Thanks for the review!!I'll fix it:)

emilio · 2017-11-18T19:30:00Z

components/layout/display_list_builder.rs

@@ -1680,8 +1681,8 @@ impl FragmentDisplayListBuilding for Fragment {
                                                          corners.3.resolve(webrender_image.width)),
                                // TODO(gw): Support border-image-outset
                                outset: SideOffsets2D::zero(),
-                                repeat_horizontal: convert_repeat_mode(border_style_struct.border_image_repeat.0),
-                                repeat_vertical: convert_repeat_mode(border_style_struct.border_image_repeat.1),
+                                repeat_horizontal: convert_repeat_mode(border_style_struct.BorderImageRepeatKeyword.0),


This is also not compiling, I think

emilio · 2017-11-18T19:30:00Z

components/style/values/computed/border.rs

+    where
+        W: fmt::Write,
+    {
+        let BorderImageRepeat(horizontal, vertical) = *self;


Now you need to actually serialize them, keeping the code that existed.

Ok.Thank you for the review.I'll fix it

emilio · 2017-11-18T19:30:00Z

components/style/values/computed/border.rs

+
+    #[inline]
+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        match (computed) {


This is probably still not building, let me know if you need any help setting up the build environment or if I can help somehow, I'd be glad to :)

chansuke · 2017-12-02T13:57:41Z

@emilio Sorry for interval.I fixed little bit but stuck...I can't solve the build error below.

unresolved import values::specified::BorderImageRepeat
failed to resolve. Could not find BorderImageRepeat in specified
cannot find function get_initial_specified_value in module border_image_repeat

emilio · 2017-12-14T02:59:16Z

Sorry for the huge lag here @chansuke!

For the unresolved import, you need to export that value from values/specified/mod.rs.

For get_initial_specified_value error, you need to remove the vector=True bit.

Other than that it looks great!

bors-servo · 2017-12-15T23:00:34Z

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

chansuke · 2017-12-16T14:40:38Z

@emilio Thank you for your help!!I fixed the error.The remaining error is missing documentation for a method error but how can I solve this?

error: missing documentation for a method
  --> components/style/values/computed/border.rs:95:5
   |
95 | /     pub fn repeat() -> Self {
96 | |         BorderImageRepeat(RepeatKeyword::Repeat, RepeatKeyword::Repeat)
97 | |     }
   | |_____^
   |
note: lint level defined here
  --> components/style/values/mod.rs:9:9
   |
9  | #![deny(missing_docs)]
   |         ^^^^^^^^^^^^

error: missing documentation for an enum
   --> components/style/values/specified/border.rs:184:1
    |
184 | / pub enum BorderImageRepeat {
185 | |     /// `[ stretch | repeat | round | space ]{1,2}`
186 | |     Keywords(RepeatKeyword, Option<RepeatKeyword>),
187 | | }
    | |_^

error: aborting due to 2 previous errors

CYBAI · 2017-12-16T15:08:40Z

@chansuke You only need to write documentation comments for them; then the error will disappear. Documentation comments in Rust always start with /// or //!.

For example, here are documentation comments for BorderSideWidth

servo/components/style/values/specified/border.rs

Lines 21 to 32 in f7440bf

    
           /// A specified value for a single side of the `border-width` property. 
        
           #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)] 
        
           pub enum BorderSideWidth { 
        
               /// `thin` 
        
               Thin, 
        
               /// `medium` 
        
               Medium, 
        
               /// `thick` 
        
               Thick, 
        
               /// `<length>` 
        
               Length(Length), 
        
           }

Ref: Rust By Example - Documentation

chansuke · 2017-12-16T17:22:15Z

@CYBAI Thanks!

jdm · 2017-12-18T15:30:07Z

error[E0432]: unresolved import `style::properties::longhands::computed_value`

  --> components/layout/display_list_builder.rs:61:35

   |

61 | use style::properties::longhands::computed_value::RepeatKeyword;

   |                                   ^^^^^^^^^^^^^^ Could not find `computed_value` in `longhands`

error[E0433]: failed to resolve. Use of undeclared type or module `BackgroundOrigin`

    --> components/layout/display_list_builder.rs:1449:13

     |

1449 |             BackgroundOrigin::BorderBox => {}

     |             ^^^^^^^^^^^^^^^^ Use of undeclared type or module `BackgroundOrigin`

error[E0433]: failed to resolve. Use of undeclared type or module `BackgroundOrigin`

    --> components/layout/display_list_builder.rs:1450:13

     |

1450 |             BackgroundOrigin::PaddingBox => {

     |             ^^^^^^^^^^^^^^^^ Use of undeclared type or module `BackgroundOrigin`

error[E0433]: failed to resolve. Use of undeclared type or module `BackgroundOrigin`

    --> components/layout/display_list_builder.rs:1457:13

     |

1457 |             BackgroundOrigin::ContentBox => {

     |             ^^^^^^^^^^^^^^^^ Use of undeclared type or module `BackgroundOrigin`

warning: unused import: `style::values::specified::border::RepeatKeyword as BorderImageRepeatKeyword`

  --> components/layout/display_list_builder.rs:76:5

   |

76 | use style::values::specified::border::RepeatKeyword as BorderImageRepeatKeyword;

   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

   |

   = note: #[warn(unused_imports)] on by default

error[E0609]: no field `BorderImageRepeatKeyword` on type `&style::properties::style_structs::Border`

    --> components/layout/display_list_builder.rs:1707:88

     |

1707 |                             repeat_horizontal: convert_repeat_mode(border_style_struct.BorderImageRepeatKeyword.0),

     |                                                                                        ^^^^^^^^^^^^^^^^^^^^^^^^

error[E0609]: no field `BorderImageRepeatKeyword` on type `&style::properties::style_structs::Border`

    --> components/layout/display_list_builder.rs:1708:86

     |

1708 |                             repeat_vertical: convert_repeat_mode(border_style_struct.BorderImageRepeatKeyword.1),

     |                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^

error[E0609]: no field `BorderImageRepeatKeyword` on type `&style::properties::style_structs::Border`

    --> components/layout/display_list_builder.rs:1740:92

     |

1740 |                                 repeat_horizontal: convert_repeat_mode(border_style_struct.BorderImageRepeatKeyword.0),

     |                                                                                            ^^^^^^^^^^^^^^^^^^^^^^^^

error[E0609]: no field `BorderImageRepeatKeyword` on type `&style::properties::style_structs::Border`

    --> components/layout/display_list_builder.rs:1741:90

     |

1741 |                                 repeat_vertical: convert_repeat_mode(border_style_struct.BorderImageRepeatKeyword.1),

     |                                                                                          ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 8 previous errors

error: Could not compile `layout`.

bors-servo · 2017-12-21T08:47:39Z

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

chansuke · 2017-12-30T09:13:25Z

Sorry for the lag.I will rebase and fix the error.

emilio · 2018-01-01T22:48:02Z

Looks fine, but there are still Geckolib build errors (you can check them with ./mach build-geckolib or ./mach cargo-geckolib check)

bors-servo · 2018-01-02T16:00:44Z

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

chansuke · 2018-01-12T04:37:29Z

@emilio Hi.I'm blocking with these build errors below.I was wondering if you could give me some suggestions.Thanks.

error[E0432]: unresolved import `style::properties::longhands::border_image_repeat::computed_value::RepeatKeyword`
  --> components/layout/display_list/builder.rs:67:5

error[E0611]: field `0` of tuple-struct `style::values::computed::BorderImageRepeat` is private
    --> components/layout/display_list/builder.rs:1452:33

error[E0611]: field `1` of tuple-struct `style::values::computed::BorderImageRepeat` is private
    --> components/layout/display_list/builder.rs:1455:33

error[E0611]: field `0` of tuple-struct `style::values::computed::BorderImageRepeat` is private
    --> components/layout/display_list/builder.rs:1492:37

error[E0611]: field `1` of tuple-struct `style::values::computed::BorderImageRepeat` is private
    --> components/layout/display_list/builder.rs:1495:37

CYBAI · 2018-01-12T06:15:25Z

Hi @chansuke
cc @emilio

With your patch, RepeatKeyword will be initiated from style::properties::longhands::border_image_repeat::computed_value.
Thus, you will need to import it from the enum your created in specified module.

servo/components/layout/display_list/builder.rs

Line 67 in 005dc85

    
           use style::properties::longhands::border_image_repeat::computed_value::RepeatKeyword;

About other error messages which is tuple-struct style::values::computed::BorderImageRepeat is private, it means your struct fields should be public.

chansuke · 2018-01-21T06:01:18Z

@emilio I fixed build errors.I wonder if you could check this.

emilio

Looks good! Some stuff still could be cleaner, but let me know if you just want to land this and we can tidy this up in followups :)

emilio · 2018-01-21T12:26:48Z

components/style/values/specified/border.rs

+#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
+pub enum BorderImageRepeat {
+    /// `[ stretch | repeat | round | space ]{1,2}`
+    Keywords(RepeatKeyword, Option<RepeatKeyword>),


This is just an enum with a single variant, so this can be a struct BorderImageRepeat(RepeatKeyword, Option<RepeatKeyword>), right?

emilio · 2018-01-21T12:26:48Z

components/style/values/computed/inherited_box.rs

+/// An angle rounded and normalized per https://drafts.csswg.org/css-images/#propdef-image-orientation
+#[allow(missing_docs)]
+#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq)]
+pub enum Orientation {


These changes are from another unrelated commit, so can just go away.

emilio · 2018-01-21T12:26:48Z

components/style/values/computed/border.rs

+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        match (computed.0, computed.1) {
+            (horizontal, vertical) => {
+                SpecifiedBorderImageRepeat::Keywords(horizontal, Some(vertical))


This can just be:

SpecifiedBorderImageRepeat::Keywords(computed.0, Some(computed.1))

Instead of this match expression.

emilio · 2018-01-21T12:26:48Z

components/style/values/computed/border.rs

+    where
+        W: fmt::Write,
+    {
+        match (self.0, self.1) {


This comment still applies.

CYBAI · 2018-01-21T15:02:12Z

components/style/values/computed/border.rs

+            SpecifiedBorderImageRepeat::Keywords(horizontal, vertical) => {
+                BorderImageRepeat(horizontal, vertical.unwrap_or(horizontal))
+            }
+        }


I'm wondering, when we only have only one pattern, will it be better to use if let here?

CYBAI · 2018-01-21T15:02:12Z

components/style/values/specified/border.rs

+        };
+
+        let vertical = input.try(RepeatKeyword::parse).ok();
+        Ok(BorderImageRepeat::Keywords(horizontal, vertical))


IMO, the original parse method looks easier to understand for me.
Just curious if you met any difficulty when you tried to use the original method?

let first = RepeatKeyword::parse(input)?; let second = input.try(RepeatKeyword::parse).ok(); Ok(SpecifiedValue(first, second))


        style: Move border-image-repeat outside of mako.

bors-servo · 2018-02-01T12:45:56Z

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

emilio · 2018-02-01T14:26:46Z

Sorry I didn't notice the followup commit @chansuke! I rebased, fixed the nits, and fixed the initial value, which you had changed to repeat instead of stretch, and opened #19924 with it.

Thanks a lot for the patch!

chansuke · 2018-02-02T16:52:08Z

@emilio @CYBAI Thank you so much for taking the time!

emilio reviewed Nov 14, 2017

View changes

highfive assigned pcwalton Nov 14, 2017

highfive added the S-awaiting-review label Nov 14, 2017

jdm assigned emilio and unassigned pcwalton Nov 16, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 16, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 16, 2017

emilio reviewed Nov 18, 2017

View changes

highfive added the S-needs-rebase label Dec 15, 2017

chansuke force-pushed the chansuke:remove-border-image-repeat branch 3 times, most recently from c397ba9 to 64a425f Dec 16, 2017

jdm removed the S-needs-rebase label Dec 18, 2017

highfive added the S-needs-rebase label Dec 21, 2017

chansuke force-pushed the chansuke:remove-border-image-repeat branch from 64a425f to ef597cd Dec 31, 2017

emilio mentioned this pull request Jan 1, 2018

style: Move border-image-repeat outside of mako. #19668

Closed

5 of 5 tasks complete

atouchet mentioned this pull request Jan 1, 2018

Move border-image-repeat outside of mako. #19021

Closed

highfive added the S-needs-rebase label Jan 2, 2018

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 3, 2018

chansuke force-pushed the chansuke:remove-border-image-repeat branch from ef597cd to ccef94c Jan 12, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 12, 2018

chansuke force-pushed the chansuke:remove-border-image-repeat branch 3 times, most recently from dc8cc2a to bbd0807 Jan 13, 2018

emilio reviewed Jan 21, 2018

View changes

CYBAI reviewed Jan 21, 2018

View changes

chansuke force-pushed the chansuke:remove-border-image-repeat branch from bbd0807 to f3347ef Jan 22, 2018

style: Move border-image-repeat outside of mako.

Loading status checks…

0149703

chansuke force-pushed the chansuke:remove-border-image-repeat branch from f3347ef to 0149703 Jan 22, 2018

highfive added the S-needs-rebase label Feb 1, 2018

emilio mentioned this pull request Feb 1, 2018

style: Move border-image-repeat outside of mako. #19924

Merged

jdm closed this Feb 1, 2018

servo / servo

Join GitHub today

style: Move border-image-repeat outside of mako. #19210

style: Move border-image-repeat outside of mako. #19210

Conversation

chansuke commented Nov 14, 2017 • edited

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

highfive commented Nov 14, 2017

highfive commented Nov 14, 2017

highfive commented Nov 14, 2017

chansuke commented Nov 16, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

chansuke commented Dec 2, 2017

emilio commented Dec 14, 2017

bors-servo commented Dec 15, 2017

chansuke commented Dec 16, 2017

CYBAI commented Dec 16, 2017

chansuke commented Dec 16, 2017

jdm commented Dec 18, 2017

bors-servo commented Dec 21, 2017

chansuke commented Dec 30, 2017

emilio commented Jan 1, 2018

bors-servo commented Jan 2, 2018

chansuke commented Jan 12, 2018

CYBAI commented Jan 12, 2018

chansuke commented Jan 21, 2018

emilio left a comment

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.

CYBAI Jan 21, 2018 • edited

This comment has been minimized.

bors-servo commented Feb 1, 2018

emilio commented Feb 1, 2018

chansuke commented Feb 2, 2018

chansuke commented Nov 14, 2017 •

edited

chansuke commented Nov 16, 2017 •

edited

CYBAI Jan 21, 2018 •

edited