style: Remove -servo-display-for-hypothetical-box from longhand #19709

CYBAI · 2018-01-06T14:24:55Z

This is a sub-PR of #19015
r? emilio

For the fn set_original_display inside properties.mako.rs, I removed is_item_or_root first to see how the tests result is. If it's needed, I'll add it back.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Move -servo-display-for-hypothetical-box outside of mako. #19697
These changes do not require tests

This change is

highfive · 2018-01-06T14:24:58Z

Heads up! This PR modifies the following files:

@bholley: components/style/values/specified/box.rs, components/style/properties/gecko.mako.rs, components/style/style_adjuster.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs
@canaltinova: components/style/values/specified/box.rs, components/style/properties/gecko.mako.rs, components/style/style_adjuster.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs
@emilio: components/style/values/specified/box.rs, components/style/properties/gecko.mako.rs, components/layout/construct.rs, components/style/style_adjuster.rs, components/style/properties/longhand/box.mako.rs and 1 more

highfive · 2018-01-06T14:25:00Z

Warning

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

tigercosmos · 2018-01-06T17:17:13Z

components/style/style_adjuster.rs

-                blockified_display,
-                is_item_or_root,
-            );
+            if let Some(box_) = self.style.get_box_if_mutated() {


What's the reason using box_ instead of box?

box is a keyword.

emilio · 2018-01-06T17:49:55Z

components/style/properties/gecko.mako.rs

@@ -3114,33 +3114,24 @@ fn static_assert() {
                                            gecko_enum_prefix="StyleDisplay",
                                            gecko_strip_moz_prefix=False) %>

-    pub fn set_display(&mut self, v: longhands::display::computed_value::T) {
+    fn match_display_keyword(
+        &mut self,


no need for self argument.

I found we still need self argument because we'd like to call it from other methods inside this impl.
However, the mut is unnecessary. I'll remove it.

You can use Self::foo instead.

Ah! Yes!! I totally forgot this way! Thanks a lot!

emilio · 2018-01-06T17:50:19Z

components/style/properties/gecko.mako.rs

+        self.gecko.mOriginalDisplay = self.match_display_keyword(v);
+    }
+
+    pub fn get_original_display(&self) -> &structs::root::mozilla::StyleDisplay {


This isn't used, is it?

No, we don't use it.
Let me remove it.

emilio · 2018-01-06T17:51:56Z

components/style/style_adjuster.rs

-                is_item_or_root,
-            );
+            if let Some(box_) = self.style.get_box_if_mutated() {
+                box_.set_original_display(blockified_display);


How is this ok in any way? This is completely changing the output.

Before this change: original_display: display, display: blockified_display.
After this change: original_display: blockified_display, display: display.

That looks pretty wrong.

Also, it's not ok to just do it if the display struct has mutated, this needs to mutate it unconditionally.

Before this change: original_display: display, display: blockified_display.
After this change: original_display: blockified_display, display: display.

I think we still need is_item_or_root inside set_original_display of servo part.

With the current patch, it will set both display and original_display directly.
Thus, with passing blockified_display, I think the after this change will be original_display: blockified_display, display: blockified_display.

Let me try to fix it.

emilio · 2018-01-06T17:52:32Z

components/style/style_adjuster.rs

+            // Save the display value without style adjustments now,
+            // which we need for hypothetical boxes.
+            let display = box_.clone_display();
+            box_.set_original_display(display);


I think I need to follow the original condition to set the original_display here.

emilio · 2018-01-06T19:18:32Z

components/style/properties/gecko.mako.rs

@@ -3152,6 +3142,14 @@ fn static_assert() {
        self.copy_display_from(other)
    }

+    pub fn set_original_display(


This is still wrong, you don't want to set mOriginalDisplay from StyleAdjuster, you just want to set mDisplay, I'd keep the set_adjusted_display name for doing that.

Got it. Let me change it back to set_adjusted_display.
I was thinking the purpose is to set original display so I named it with set_original_display.

emilio · 2018-01-08T12:41:29Z

components/style/properties/gecko.mako.rs

+        v: longhands::display::computed_value::T,
+        _is_item_or_root: bool
+    ) {
+        self.gecko.mOriginalDisplay = Self::match_display_keyword(v);


This is still not what needs to happen?

So, to clarify, mOriginalDisplay can't change from StyleAdjuster, only mDisplay should. This does the opposite as written.

Ahh, yeah, sorry, I miss this one 😣
Please let me update it again!

emilio

Hmm, there's a more subtle issue here, which is that, as written, servo doesn't set original_display correctly, right?

Where would that happen? In gecko it happens in copy_display_from and such. In servo it doesn't neither in set_display nor in copy_display_from.

I guess we could make this setup similar to what justify-items does... But that's slightly annoying. We do need to special-case those functions for the display property.

emilio

Looks great, thanks!

emilio · 2018-01-10T14:00:27Z

components/style/properties/properties.mako.rs

@@ -1863,6 +1863,10 @@ pub mod style_structs {
                /// The font hash, used for font caching.
                pub hash: u64,
            % endif
+            % if style_struct.name == "Box":
+                /// The original font which is equivalent to `mOriginalDisplay` in gecko


nit: Comment needs updating (this is the original display).

Maybe just:

// The display value specified by the CSS stylesheets (without any style adjustments), which is needed for hypothetical layout boxes.

emilio · 2018-01-10T14:01:01Z

components/style/properties/properties.mako.rs

@@ -1893,6 +1897,14 @@ pub mod style_structs {
                            self.${longhand.ident} = longhands::${longhand.ident}::computed_value
                                                              ::T(v.into_iter().collect());
                        }
+                    % elif longhand.ident == "display":
+                        /// Set ${longhand.name}.


Maybe Set display directly? And comment on why we set both?

Yeah, it should be commented only with Set display.

Is it okay to explain like this?

/// Similar to `set_display` in `gecko`, we need to specify /// `original_display` when setting `display`; thus, we need /// to special-case this method.

Maybe just:

/// We need to keep track of the original display for hypothetical boxes, so we need to special-case this.

emilio · 2018-01-10T14:01:27Z

components/style/properties/properties.mako.rs

-                    }
-
+                    % if longhand.ident == "display":
+                        /// Set ${longhand.name} from other struct.


Ditto, let's make the comment a bit more specific, and document why the special case is needed.

emilio · 2018-01-10T14:02:04Z

@bors-servo try

bors-servo · 2018-01-10T14:02:09Z

⌛ Trying commit 500587f with merge da7fe9c...

style: Remove -servo-display-for-hypothetical-box from longhand This is a sub-PR of #19015 r? emilio For the `fn set_original_display` inside `properties.mako.rs`, I removed `is_item_or_root` first to see how the tests result is. If it's needed, I'll add it back. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19697 - [x] These changes do not require tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19709)

emilio · 2018-01-10T14:18:32Z

components/style/properties/properties.mako.rs

+                        #[inline]
+                        pub fn copy_display_from(&mut self, other: &Self) {
+                            self.display = other.display.clone();
+                            self.original_display = other.original_display.clone();


Welp, there's a bug here, this should use other.display, not other.original_display.

Ohhh! Thanks for the good catching!

emilio · 2018-01-10T14:19:09Z

components/style/properties/properties.mako.rs

@@ -1893,6 +1897,14 @@ pub mod style_structs {
                            self.${longhand.ident} = longhands::${longhand.ident}::computed_value
                                                              ::T(v.into_iter().collect());
                        }
+                    % elif longhand.ident == "display":
+                        /// Set ${longhand.name}.


Maybe just:

/// We need to keep track of the original display for hypothetical boxes, so we need to special-case this.

emilio · 2018-01-10T14:48:38Z

@bors-servo r+

bors-servo · 2018-01-10T14:48:38Z

📌 Commit b5133cf has been approved by emilio

bors-servo · 2018-01-10T15:58:15Z

⌛ Testing commit b5133cf with merge 730ff59...

style: Remove -servo-display-for-hypothetical-box from longhand This is a sub-PR of #19015 r? emilio For the `fn set_original_display` inside `properties.mako.rs`, I removed `is_item_or_root` first to see how the tests result is. If it's needed, I'll add it back. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19697 - [x] These changes do not require tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19709)

bors-servo · 2018-01-10T16:37:45Z

💔 Test failed - mac-rel-wpt3

bors-servo · 2018-01-13T10:13:37Z

💔 Test failed - linux-rel-css

emilio · 2018-01-13T10:48:29Z

Ok, that test comes from #7355, and I'm not sure what makes it pass now... Maybe needs some investigation.

CYBAI · 2018-01-13T10:58:34Z

@emilio I'll check the PR later to see if I can find some clues to share to you.
Not sure if #19757 will have same impact.

emilio · 2018-01-13T11:47:12Z

So, this is:

./mach run --pref layout.writing-mode.enabled=true tests/wpt/mozilla/tests/css/iframe/size_attributes_vertical_writing_mode.html

And it's creating an InlineBlock for the iframe out of the blue, so that seems suspicious...

…Servo. This is functionally equivalent right now (set_adjusted_display will just do set_display), but won't be after servo#19709.

style: Adjust the writing-mode fixup to call set_adjusted_display on Servo. This is functionally equivalent right now (set_adjusted_display will just do set_display), but won't be after #19709.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19758)

CYBAI · 2018-01-14T04:19:33Z

@bors-servo try

bors-servo · 2018-01-14T04:19:33Z

@CYBAI: 🔑 Insufficient privileges: and not in try users

KiChjang · 2018-01-14T05:11:23Z

@CYBAI Unfortunately, it seems that homu hasn't deployed yet, so you will not be able to use your try powers until someone deploys the changes made.

@bors-servo try

bors-servo · 2018-01-14T05:11:29Z

⌛ Trying commit 0291a75 with merge 580afb8...

style: Remove -servo-display-for-hypothetical-box from longhand This is a sub-PR of #19015 r? emilio For the `fn set_original_display` inside `properties.mako.rs`, I removed `is_item_or_root` first to see how the tests result is. If it's needed, I'll add it back. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19697 - [x] These changes do not require tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19709)

CYBAI · 2018-01-14T05:23:56Z

@KiChjang haha, after above try, I know it's not deployed yet. Thanks for the try!

bors-servo · 2018-01-14T06:05:22Z

☀️ 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
State: approved= try=True

emilio · 2018-01-14T14:27:11Z

@bors-servo r+

bors-servo · 2018-01-14T14:27:11Z

📌 Commit 0291a75 has been approved by emilio

bors-servo · 2018-01-14T14:27:17Z

⌛ Testing commit 0291a75 with merge 1b46e2e...

style: Remove -servo-display-for-hypothetical-box from longhand This is a sub-PR of #19015 r? emilio For the `fn set_original_display` inside `properties.mako.rs`, I removed `is_item_or_root` first to see how the tests result is. If it's needed, I'll add it back. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19697 - [x] These changes do not require tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19709)

bors-servo · 2018-01-14T15:18:15Z

☀️ 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: emilio
Pushing 1b46e2e to master...

highfive assigned emilio Jan 6, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 6, 2018

CYBAI force-pushed the servo-display-out-of-mako branch from 9368bb8 to d6dc633 Compare January 6, 2018 15:08

tigercosmos reviewed Jan 6, 2018

View reviewed changes

emilio requested changes Jan 6, 2018

View reviewed changes

CYBAI force-pushed the servo-display-out-of-mako branch 2 times, most recently from 89c10f2 to e13bab1 Compare January 6, 2018 18:39

emilio reviewed Jan 6, 2018

View reviewed changes

CYBAI force-pushed the servo-display-out-of-mako branch 3 times, most recently from c972443 to eb04098 Compare January 7, 2018 03:23

emilio reviewed Jan 8, 2018

View reviewed changes

CYBAI force-pushed the servo-display-out-of-mako branch from eb04098 to e2b3724 Compare January 8, 2018 13:26

emilio reviewed Jan 8, 2018

View reviewed changes

CYBAI force-pushed the servo-display-out-of-mako branch from e2b3724 to 500587f Compare January 9, 2018 14:29

emilio approved these changes Jan 10, 2018

View reviewed changes

emilio reviewed Jan 10, 2018

View reviewed changes

CYBAI force-pushed the servo-display-out-of-mako branch from 500587f to b5133cf Compare January 10, 2018 14:40

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 10, 2018

emilio mentioned this pull request Jan 13, 2018

style: Remove -servo-text-decorations-in-effect #19757

Merged

highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 13, 2018

emilio mentioned this pull request Jan 13, 2018

style: Adjust the writing-mode fixup to call set_adjusted_display on Servo. #19758

Merged

style: Remove -servo-display-for-hypothetical-box from longhand

0291a75

CYBAI force-pushed the servo-display-out-of-mako branch from 3ab8b65 to 0291a75 Compare January 13, 2018 12:55

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 13, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 14, 2018

bors-servo merged commit 0291a75 into servo:master Jan 14, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 14, 2018

CYBAI deleted the servo-display-out-of-mako branch January 14, 2018 15:21

style: Remove -servo-display-for-hypothetical-box from longhand #19709

style: Remove -servo-display-for-hypothetical-box from longhand #19709

Conversation

CYBAI commented Jan 6, 2018 • edited by emilio Loading

highfive commented Jan 6, 2018

highfive commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CYBAI Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CYBAI Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Jan 10, 2018

bors-servo commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Jan 10, 2018

bors-servo commented Jan 10, 2018

bors-servo commented Jan 10, 2018

bors-servo commented Jan 10, 2018

bors-servo commented Jan 13, 2018

emilio commented Jan 13, 2018

CYBAI commented Jan 13, 2018 • edited Loading

emilio commented Jan 13, 2018

CYBAI commented Jan 14, 2018

bors-servo commented Jan 14, 2018

KiChjang commented Jan 14, 2018

bors-servo commented Jan 14, 2018

CYBAI commented Jan 14, 2018

bors-servo commented Jan 14, 2018

emilio commented Jan 14, 2018

bors-servo commented Jan 14, 2018

bors-servo commented Jan 14, 2018

bors-servo commented Jan 14, 2018

CYBAI commented Jan 6, 2018 •

edited by emilio

Loading

CYBAI Jan 6, 2018 •

edited

Loading

CYBAI Jan 6, 2018 •

edited

Loading

CYBAI commented Jan 13, 2018 •

edited

Loading