Closed Bug 1369355 Opened 3 years ago Closed 3 years ago
Clean up/optimize ns
IFrame::Get Overflow Areas Property
The comment at https://dxr.mozilla.org/mozilla-central/rev/7b8937970f9ca85db88cb2496f2112175fd847c8/layout/generic/nsFrame.cpp#8700-8701 claims: /** Create or retrieve the previously stored overflow area, if the frame does * not overflow and no creation is required return nullptr. but this is not true: the method doesn't check whether creation is required, and never returns nullptr. On looking at call sites, it turns out that only one caller requires the behavior of creating a new overflow areas property; the others already know that such a property must be present. So for those callers, we can use a simplified method that doesn't include the check-and-create-if-missing, which may help the compiler optimize better.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Actually, this also lets us clean up some ugly casts (as GetOverflowAreasProperty() is now a const method). Sorry for the churn.
One more time! It turns out the GetOrCreate... method can simply be Create..., as we only call it after already handling the case of an existing property in a separate branch.
Attachment #8873391 - Flags: review?(mats)
Attachment #8873391 - Attachment description: Simplify nsIFrame::GetOverflowAreasProperty, and provide separate GetOrCreate... variant for use when it may not already exist → Simplify nsIFrame::GetOverflowAreasProperty, and provide separate Create... method for the caller that needs to add a new property rather than examine the existing one.
Comment on attachment 8873391 [details] [diff] [review] Simplify nsIFrame::GetOverflowAreasProperty, and provide separate Create... method for the caller that needs to add a new property rather than examine the existing one. r=mats, with some nits: I think we should assert this in GetOverflowAreasProperty(): MOZ_ASSERT(mOverflow.mType == NS_FRAME_OVERFLOW_LARGE) I don't think CreateOverflowAreasProperty is needed. It's just called in one place and it seems better to inline it there to avoid the disconnect with the "mOverflow.mType = NS_FRAME_OVERFLOW_LARGE" that should always be set when creating this property. The MOZ_ASSERT(overflow) isn't needed, since "new nsOverflowAreas" is infallible.
Attachment #8873391 - Flags: review?(mats) → review+
Don't forget to update the commit message. :-)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/5600d485769b Simplify nsIFrame::GetOverflowAreasProperty, as callers already know whether a property is present. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/5600d485769bed18de1fc9e8ba8e3f7e66bc771b Bug 1369355 - Simplify nsIFrame::GetOverflowAreasProperty, as callers already know whether a property is present. r=mats
You need to log in before you can comment on or make changes to this bug.