Closed Bug 1269901 Opened 4 years ago Closed 4 years ago

Remove margin and padding caches on the style structs

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(3 files, 1 obsolete file)

Dealing with them from servo is a pain, and I'm pretty sure they aren't necessary.
Attached patch Refactor Helpers. v1 (obsolete) — Splinter Review
Attachment #8748390 - Flags: review?(dbaron)
Attachment #8748392 - Flags: review?(dbaron)
Attachment #8748393 - Flags: review?(dbaron)
Stupid implicit bool conversions. :-( :-(
Attachment #8748748 - Flags: review?(dbaron)
Attachment #8748390 - Attachment is obsolete: true
Attachment #8748390 - Flags: review?(dbaron)
Attachment #8748392 - Attachment description: Remove mCachedPadding. v1 → Part 2 - Remove mCachedPadding. v1
Attachment #8748393 - Attachment description: Remove mCachedMargin. v1 → Part 3 - Remove mCachedMargin. v1
Talos looks good. This should be green now, double-checking on linux64.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3142aba4027c
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
Attachment #8748748 - Flags: review?(dbaron) → review+
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
Attachment #8748392 - Flags: review?(dbaron) → review+
Attachment #8748393 - Flags: review?(dbaron) → review+
(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.
You need to log in before you can comment on or make changes to this bug.