1269901 - Remove margin and padding caches on the style structs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Dealing with them from servo is a pain, and I'm pretty sure they aren't necessary.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Refactor Helpers. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove mCachedPadding. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove mCachedMargin. v1

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b802a83c190&selectedJob=20321733

This is orange. I'm looking into it now.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Refactor Helpers. v2

Stupid implicit bool conversions. :-( :-(

Comment 6

[Bobby Holley (:bholley)]

Talos looks good. This should be green now, double-checking on linux64.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3142aba4027c

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8748748 [details] [diff] [review]
Part 1 - Refactor Helpers. v2

>+  bool ConvertsToLength() const {
>+    NS_FOR_CSS_SIDES(side) {
>+      if (!Get(side).ConvertsToLength())
>+        return false;
>+    }
>+    return true;
>+  }

Please add {} around the inner if-body.  (Two places in the patch, I think, but I already deleted the quoted material for the other.)

r=dbaron with that

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8748392 [details] [diff] [review]
Part 2 - Remove mCachedPadding. v1

>+  void GetPaddingNoPercentage(nsMargin& aPadding) const
>+  {
>+    MOZ_ASSERT(mPadding.ConvertsToLength());
>+    NS_FOR_CSS_SIDES(side) {
>+      aPadding.Side(side) = std::max(mPadding.Get(side).ToLength(), 0);
>+    }
>+  }

Please retain, in this function, the comment from RecalcData:
-      // Clamp negative calc() to 0.
that explains why the std::max is there.

r=dbaron with that

Comment 9

[Bobby Holley (:bholley)]

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #7)
> Please add {} around the inner if-body.  (Two places in the patch, I think,
> but I already deleted the quoted material for the other.)

I don't see a second one.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a923340a036f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1abdf346b872
https://hg.mozilla.org/integration/mozilla-inbound/rev/581279cd287e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a923340a036f
https://hg.mozilla.org/mozilla-central/rev/1abdf346b872
https://hg.mozilla.org/mozilla-central/rev/581279cd287e