Closed
Bug 1269901
Opened 8 years ago
Closed 8 years ago
Remove margin and padding caches on the style structs
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(3 files, 1 obsolete file)
8.22 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
5.92 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
Dealing with them from servo is a pain, and I'm pretty sure they aren't necessary.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8748390 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8748392 -
Flags: review?(dbaron)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8748393 -
Flags: review?(dbaron)
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b802a83c190&selectedJob=20321733 This is orange. I'm looking into it now.
Assignee | ||
Comment 5•8 years ago
|
||
Stupid implicit bool conversions. :-( :-(
Attachment #8748748 -
Flags: review?(dbaron)
Assignee | ||
Updated•8 years ago
|
Attachment #8748390 -
Attachment is obsolete: true
Attachment #8748390 -
Flags: review?(dbaron)
Assignee | ||
Updated•8 years ago
|
Attachment #8748392 -
Attachment description: Remove mCachedPadding. v1 → Part 2 - Remove mCachedPadding. v1
Assignee | ||
Updated•8 years ago
|
Attachment #8748393 -
Attachment description: Remove mCachedMargin. v1 → Part 3 - Remove mCachedMargin. v1
Assignee | ||
Comment 6•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a923340a036f https://hg.mozilla.org/mozilla-central/rev/1abdf346b872 https://hg.mozilla.org/mozilla-central/rev/581279cd287e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•